From 148944814b0b1417e7abd1c433eefd6125f526ee Mon Sep 17 00:00:00 2001
From: Andrew Meadows <andrew@highfidelity.io>
Date: Mon, 17 Sep 2018 13:28:03 -0700
Subject: [PATCH 1/4] consolidate two bad string checks into one

---
 libraries/animation/src/AnimationCache.h      |  2 +-
 .../src/RenderableModelEntityItem.cpp         | 33 ++++++++++++-------
 .../src/RenderableModelEntityItem.h           |  4 +--
 libraries/entities/src/ModelEntityItem.cpp    | 24 --------------
 libraries/entities/src/ModelEntityItem.h      |  7 +---
 5 files changed, 25 insertions(+), 45 deletions(-)

diff --git a/libraries/animation/src/AnimationCache.h b/libraries/animation/src/AnimationCache.h
index ca5ea5b072..483350e2b5 100644
--- a/libraries/animation/src/AnimationCache.h
+++ b/libraries/animation/src/AnimationCache.h
@@ -22,7 +22,7 @@
 
 class Animation;
 
-typedef QSharedPointer<Animation> AnimationPointer;
+using AnimationPointer = QSharedPointer<Animation>;
 
 class AnimationCache : public ResourceCache, public Dependency  {
     Q_OBJECT
diff --git a/libraries/entities-renderer/src/RenderableModelEntityItem.cpp b/libraries/entities-renderer/src/RenderableModelEntityItem.cpp
index 0452587439..b0c932082f 100644
--- a/libraries/entities-renderer/src/RenderableModelEntityItem.cpp
+++ b/libraries/entities-renderer/src/RenderableModelEntityItem.cpp
@@ -1068,6 +1068,13 @@ void RenderableModelEntityItem::copyAnimationJointDataToModel() {
     }
 }
 
+bool RenderableModelEntityItem::readyToAnimate() const {
+    return resultWithReadLock<bool>([&] {
+        float firstFrame = _animationProperties.getFirstFrame();
+        return (firstFrame <= 0.0f) || (firstFrame <= _animationProperties.getLastFrame());
+    });
+}
+
 using namespace render;
 using namespace render::entities;
 
@@ -1155,7 +1162,7 @@ void ModelEntityRenderer::animate(const TypedEntityPointer& entity) {
 
     const QVector<glm::quat>& rotations = frames[_lastKnownCurrentFrame].rotations;
     const QVector<glm::vec3>& translations = frames[_lastKnownCurrentFrame].translations;
-                
+
     jointsData.resize(_jointMapping.size());
     for (int j = 0; j < _jointMapping.size(); j++) {
         int index = _jointMapping[j];
@@ -1169,13 +1176,12 @@ void ModelEntityRenderer::animate(const TypedEntityPointer& entity) {
                 }
             } else if (index < animationJointNames.size()) {
                 QString jointName = fbxJoints[index].name; // Pushing this here so its not done on every entity, with the exceptions of those allowing for translation
-                
                 if (originalFbxIndices.contains(jointName)) {
                     // Making sure the joint names exist in the original model the animation is trying to apply onto. If they do, then remap and get it's translation.
                     int remappedIndex = originalFbxIndices[jointName] - 1; // JointIndeces seem to always start from 1 and the found index is always 1 higher than actual.
                     translationMat = glm::translate(originalFbxJoints[remappedIndex].translation);
                 }
-            } 
+            }
             glm::mat4 rotationMat;
             if (index < rotations.size()) {
                 rotationMat = glm::mat4_cast(fbxJoints[index].preRotation * rotations[index] * fbxJoints[index].postRotation);
@@ -1477,14 +1483,16 @@ void ModelEntityRenderer::doRenderUpdateSynchronousTyped(const ScenePointer& sce
     if (_animating) {
         DETAILED_PROFILE_RANGE(simulation_physics, "Animate");
 
-        if (!jointsMapped()) {
-            mapJoints(entity, model->getJointNames());
-        //else the joint have been mapped before but we have a new animation to load
-        } else if (_animation && (_animation->getURL().toString() != entity->getAnimationURL())) {
+        if (_animation && (_animation->getURL().toString() != entity->getAnimationURL())) { // bad check
+            // the joints have been mapped before but we have a new animation to load
+            _animation.reset();
             _jointMappingCompleted = false;
-            mapJoints(entity, model->getJointNames());
         }
-        if (!(entity->getAnimationFirstFrame() < 0) && !(entity->getAnimationFirstFrame() > entity->getAnimationLastFrame())) {
+
+        if (!_jointMappingCompleted) {
+            mapJoints(entity, model);
+        }
+        if (entity->readyToAnimate()) {
             animate(entity);
         }
         emit requestRenderUpdate();
@@ -1518,19 +1526,20 @@ void ModelEntityRenderer::doRender(RenderArgs* args) {
 #endif
 }
 
-void ModelEntityRenderer::mapJoints(const TypedEntityPointer& entity, const QStringList& modelJointNames) {
+void ModelEntityRenderer::mapJoints(const TypedEntityPointer& entity, const ModelPointer& model) {
     // if we don't have animation, or we're already joint mapped then bail early
-    if (!entity->hasAnimation() || jointsMapped()) {
+    if (!entity->hasAnimation()) {
         return;
     }
 
-    if (!_animation || _animation->getURL().toString() != entity->getAnimationURL()) {
+    if (!_animation) {
         _animation = DependencyManager::get<AnimationCache>()->getAnimation(entity->getAnimationURL());
     }
 
     if (_animation && _animation->isLoaded()) {
         QStringList animationJointNames = _animation->getJointNames();
 
+        auto modelJointNames = model->getJointNames();
         if (modelJointNames.size() > 0 && animationJointNames.size() > 0) {
             _jointMapping.resize(modelJointNames.size());
             for (int i = 0; i < modelJointNames.size(); i++) {
diff --git a/libraries/entities-renderer/src/RenderableModelEntityItem.h b/libraries/entities-renderer/src/RenderableModelEntityItem.h
index b5f9d9f833..b04766c320 100644
--- a/libraries/entities-renderer/src/RenderableModelEntityItem.h
+++ b/libraries/entities-renderer/src/RenderableModelEntityItem.h
@@ -120,6 +120,7 @@ private:
     bool needsUpdateModelBounds() const;
     void autoResizeJointArrays();
     void copyAnimationJointDataToModel();
+    bool readyToAnimate() const;
 
     void getCollisionGeometryResource();
     GeometryResource::Pointer _compoundShapeResource;
@@ -169,8 +170,7 @@ protected:
 
 private:
     void animate(const TypedEntityPointer& entity);
-    void mapJoints(const TypedEntityPointer& entity, const QStringList& modelJointNames);
-    bool jointsMapped() const { return _jointMappingCompleted; }
+    void mapJoints(const TypedEntityPointer& entity, const ModelPointer& model);
 
     // Transparency is handled in ModelMeshPartPayload
     virtual bool isTransparent() const override { return false; }
diff --git a/libraries/entities/src/ModelEntityItem.cpp b/libraries/entities/src/ModelEntityItem.cpp
index ec864aa8c8..3c4cacc9d3 100644
--- a/libraries/entities/src/ModelEntityItem.cpp
+++ b/libraries/entities/src/ModelEntityItem.cpp
@@ -634,30 +634,6 @@ bool ModelEntityItem::getAnimationHold() const {
     });
 }
 
-void ModelEntityItem::setAnimationFirstFrame(float firstFrame) { 
-    withWriteLock([&] {
-        _animationProperties.setFirstFrame(firstFrame);
-    });
-}
-
-float ModelEntityItem::getAnimationFirstFrame() const { 
-    return resultWithReadLock<float>([&] {
-        return _animationProperties.getFirstFrame();
-    });
-}
-
-void ModelEntityItem::setAnimationLastFrame(float lastFrame) { 
-    withWriteLock([&] {
-        _animationProperties.setLastFrame(lastFrame);
-    });
-}
-
-float ModelEntityItem::getAnimationLastFrame() const { 
-    return resultWithReadLock<float>([&] {
-        return _animationProperties.getLastFrame();
-    });
-}
-
 bool ModelEntityItem::getAnimationIsPlaying() const { 
     return resultWithReadLock<bool>([&] {
         return _animationProperties.getRunning();
diff --git a/libraries/entities/src/ModelEntityItem.h b/libraries/entities/src/ModelEntityItem.h
index 2716097c93..07cb97430d 100644
--- a/libraries/entities/src/ModelEntityItem.h
+++ b/libraries/entities/src/ModelEntityItem.h
@@ -83,6 +83,7 @@ public:
     // Animation related items...
     AnimationPropertyGroup getAnimationProperties() const;
 
+    // TODO: audit and remove unused Animation accessors
     bool hasAnimation() const;
     QString getAnimationURL() const;
     void setAnimationURL(const QString& url);
@@ -103,12 +104,6 @@ public:
     void setRelayParentJoints(bool relayJoints);
     bool getRelayParentJoints() const;
 
-    void setAnimationFirstFrame(float firstFrame);
-    float getAnimationFirstFrame() const;
-
-    void setAnimationLastFrame(float lastFrame);
-    float getAnimationLastFrame() const;
-
     bool getAnimationIsPlaying() const;
     float getAnimationCurrentFrame() const;
     float getAnimationFPS() const;

From 0c617793311e013d2864de43b8ed8b8041f3c628 Mon Sep 17 00:00:00 2001
From: Andrew Meadows <andrew@highfidelity.io>
Date: Mon, 17 Sep 2018 13:58:00 -0700
Subject: [PATCH 2/4] remove deprecated cruft; stub setAnimationURL() override

---
 .../src/RenderableModelEntityItem.cpp                  | 10 ++--------
 .../entities-renderer/src/RenderableModelEntityItem.h  |  8 ++++----
 libraries/entities/src/ModelEntityItem.h               |  2 +-
 3 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/libraries/entities-renderer/src/RenderableModelEntityItem.cpp b/libraries/entities-renderer/src/RenderableModelEntityItem.cpp
index b0c932082f..e7d8329b3c 100644
--- a/libraries/entities-renderer/src/RenderableModelEntityItem.cpp
+++ b/libraries/entities-renderer/src/RenderableModelEntityItem.cpp
@@ -975,14 +975,8 @@ QStringList RenderableModelEntityItem::getJointNames() const {
     return result;
 }
 
-// FIXME: deprecated; remove >= RC67
-bool RenderableModelEntityItem::getMeshes(MeshProxyList& result) {
-    auto model = getModel();
-    if (!model || !model->isLoaded()) {
-        return false;
-    }
-    BLOCKING_INVOKE_METHOD(model.get(), "getMeshes", Q_RETURN_ARG(MeshProxyList, result));
-    return !result.isEmpty();
+void RenderableModelEntityItem::setAnimationURL(const QString& url) {
+    ModelEntityItem::setAnimationURL(url);
 }
 
 scriptable::ScriptableModelBase render::entities::ModelEntityRenderer::getScriptableModel() {
diff --git a/libraries/entities-renderer/src/RenderableModelEntityItem.h b/libraries/entities-renderer/src/RenderableModelEntityItem.h
index b04766c320..3d242784b5 100644
--- a/libraries/entities-renderer/src/RenderableModelEntityItem.h
+++ b/libraries/entities-renderer/src/RenderableModelEntityItem.h
@@ -114,20 +114,20 @@ public:
     virtual int getJointIndex(const QString& name) const override;
     virtual QStringList getJointNames() const override;
 
-    bool getMeshes(MeshProxyList& result) override; // deprecated
+    void setAnimationURL(const QString& url) override;
 
 private:
     bool needsUpdateModelBounds() const;
     void autoResizeJointArrays();
     void copyAnimationJointDataToModel();
     bool readyToAnimate() const;
-
     void getCollisionGeometryResource();
+
     GeometryResource::Pointer _compoundShapeResource;
-    bool _jointMapCompleted { false };
-    bool _originalTexturesRead { false };
     std::vector<int> _jointMap;
     QVariantMap _originalTextures;
+    bool _jointMapCompleted { false };
+    bool _originalTexturesRead { false };
     bool _dimensionsInitialized { true };
     bool _needsJointSimulation { false };
 };
diff --git a/libraries/entities/src/ModelEntityItem.h b/libraries/entities/src/ModelEntityItem.h
index 07cb97430d..666c7571ed 100644
--- a/libraries/entities/src/ModelEntityItem.h
+++ b/libraries/entities/src/ModelEntityItem.h
@@ -86,7 +86,7 @@ public:
     // TODO: audit and remove unused Animation accessors
     bool hasAnimation() const;
     QString getAnimationURL() const;
-    void setAnimationURL(const QString& url);
+    virtual void setAnimationURL(const QString& url);
 
     void setAnimationCurrentFrame(float value);
     void setAnimationIsPlaying(bool value);

From da0d70af3f2b69644b95bc5eedffdf3cee8c24d2 Mon Sep 17 00:00:00 2001
From: Andrew Meadows <andrew@highfidelity.io>
Date: Mon, 17 Sep 2018 15:29:18 -0700
Subject: [PATCH 3/4] don't use string comparison when polling for changed
 animation URL

---
 .../src/RenderableModelEntityItem.cpp          | 18 ++++++++++++++++--
 .../src/RenderableModelEntityItem.h            |  3 +++
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/libraries/entities-renderer/src/RenderableModelEntityItem.cpp b/libraries/entities-renderer/src/RenderableModelEntityItem.cpp
index e7d8329b3c..5f96692416 100644
--- a/libraries/entities-renderer/src/RenderableModelEntityItem.cpp
+++ b/libraries/entities-renderer/src/RenderableModelEntityItem.cpp
@@ -976,7 +976,20 @@ QStringList RenderableModelEntityItem::getJointNames() const {
 }
 
 void RenderableModelEntityItem::setAnimationURL(const QString& url) {
+    QString oldURL = getAnimationURL();
     ModelEntityItem::setAnimationURL(url);
+    if (oldURL != getAnimationURL()) {
+        _needsAnimationReset = true;
+    }
+}
+
+bool RenderableModelEntityItem::needsAnimationReset() const {
+    return _needsAnimationReset;
+}
+
+QString RenderableModelEntityItem::getAnimationURLAndReset() {
+    _needsAnimationReset = false;
+    return getAnimationURL();
 }
 
 scriptable::ScriptableModelBase render::entities::ModelEntityRenderer::getScriptableModel() {
@@ -1477,7 +1490,8 @@ void ModelEntityRenderer::doRenderUpdateSynchronousTyped(const ScenePointer& sce
     if (_animating) {
         DETAILED_PROFILE_RANGE(simulation_physics, "Animate");
 
-        if (_animation && (_animation->getURL().toString() != entity->getAnimationURL())) { // bad check
+        if (_animation && entity->needsAnimationReset()) {
+            //(_animation->getURL().toString() != entity->getAnimationURL())) { // bad check
             // the joints have been mapped before but we have a new animation to load
             _animation.reset();
             _jointMappingCompleted = false;
@@ -1527,7 +1541,7 @@ void ModelEntityRenderer::mapJoints(const TypedEntityPointer& entity, const Mode
     }
 
     if (!_animation) {
-        _animation = DependencyManager::get<AnimationCache>()->getAnimation(entity->getAnimationURL());
+        _animation = DependencyManager::get<AnimationCache>()->getAnimation(entity->getAnimationURLAndReset());
     }
 
     if (_animation && _animation->isLoaded()) {
diff --git a/libraries/entities-renderer/src/RenderableModelEntityItem.h b/libraries/entities-renderer/src/RenderableModelEntityItem.h
index 3d242784b5..4ef34188c9 100644
--- a/libraries/entities-renderer/src/RenderableModelEntityItem.h
+++ b/libraries/entities-renderer/src/RenderableModelEntityItem.h
@@ -115,6 +115,8 @@ public:
     virtual QStringList getJointNames() const override;
 
     void setAnimationURL(const QString& url) override;
+    bool needsAnimationReset() const;
+    QString getAnimationURLAndReset();
 
 private:
     bool needsUpdateModelBounds() const;
@@ -130,6 +132,7 @@ private:
     bool _originalTexturesRead { false };
     bool _dimensionsInitialized { true };
     bool _needsJointSimulation { false };
+    bool _needsAnimationReset { false };
 };
 
 namespace render { namespace entities { 

From 3b1b64b057dd743e9f0d1da15bc0ab09df88c779 Mon Sep 17 00:00:00 2001
From: Andrew Meadows <andrew@highfidelity.io>
Date: Tue, 2 Oct 2018 18:25:40 -0700
Subject: [PATCH 4/4] fix logic in readyToAnimate()

---
 libraries/entities-renderer/src/RenderableModelEntityItem.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libraries/entities-renderer/src/RenderableModelEntityItem.cpp b/libraries/entities-renderer/src/RenderableModelEntityItem.cpp
index 5f96692416..c6337dc872 100644
--- a/libraries/entities-renderer/src/RenderableModelEntityItem.cpp
+++ b/libraries/entities-renderer/src/RenderableModelEntityItem.cpp
@@ -1078,7 +1078,7 @@ void RenderableModelEntityItem::copyAnimationJointDataToModel() {
 bool RenderableModelEntityItem::readyToAnimate() const {
     return resultWithReadLock<bool>([&] {
         float firstFrame = _animationProperties.getFirstFrame();
-        return (firstFrame <= 0.0f) || (firstFrame <= _animationProperties.getLastFrame());
+        return (firstFrame >= 0.0f) && (firstFrame <= _animationProperties.getLastFrame());
     });
 }