Merge with master

libraries/entities/src/AnimationPropertyGroup.cpp

@@ -216,7 +216,6 @@ bool AnimationPropertyGroup::appendToEditPacket(OctreePacketData* packetData,
 
 
 bool AnimationPropertyGroup::decodeFromEditPacket(EntityPropertyFlags& propertyFlags, const unsigned char*& dataAt , int& processedBytes) {
-
     int bytesRead = 0;
     bool overwriteLocalData = true;
     bool somethingChanged = false;
@@ -360,3 +359,21 @@ int AnimationPropertyGroup::readEntitySubclassDataFromBuffer(const unsigned char
     READ_ENTITY_PROPERTY(PROP_ANIMATION_HOLD, bool, setHold);
     return bytesRead;
 }
+
+float AnimationPropertyGroup::getNumFrames() const {
+    return _lastFrame - _firstFrame + 1.0f;
+}
+
+float AnimationPropertyGroup::computeLoopedFrame(float frame) const {
+    float numFrames = getNumFrames();
+    if (numFrames > 1.0f) {
+        frame = getFirstFrame() + fmodf(frame - getFirstFrame(), numFrames);
+    } else {
+        frame = getFirstFrame();
+    }
+    return frame;
+}
+
+bool AnimationPropertyGroup::isValidAndRunning() const {
+    return getRunning() && (getFPS() > 0.0f) && (getNumFrames() > 1.0f) && !(getURL().isEmpty());
+}

libraries/entities/src/AnimationPropertyGroup.h

@@ -77,6 +77,10 @@ public:
                                                 EntityPropertyFlags& propertyFlags, bool overwriteLocalData,
                                                 bool& somethingChanged) override;
 
+    float getNumFrames() const;
+    float computeLoopedFrame(float frame) const;
+    bool isValidAndRunning() const;
+
     DEFINE_PROPERTY_REF(PROP_ANIMATION_URL, URL, url, QString, "");
     DEFINE_PROPERTY(PROP_ANIMATION_FPS, FPS, fps, float, 30.0f);
     DEFINE_PROPERTY(PROP_ANIMATION_FRAME_INDEX, CurrentFrame, currentFrame, float, 0.0f);

libraries/entities/src/EntityItem.h

@@ -603,7 +603,7 @@ protected:
     //
 
     // DirtyFlags are set whenever a property changes that the EntitySimulation needs to know about.
-    uint32_t _flags { 0 };   // things that have changed from EXTERNAL changes (via script or packet) but NOT from simulation
+    std::atomic_uint _flags { 0 };   // things that have changed from EXTERNAL changes (via script or packet) but NOT from simulation
 
     // these backpointers are only ever set/cleared by friends:
     EntityTreeElementPointer _element; // set by EntityTreeElement

libraries/entities/src/ModelEntityItem.cpp

@@ -82,12 +82,12 @@ bool ModelEntityItem::setProperties(const EntityItemProperties& properties) {
     SET_ENTITY_PROPERTY_FROM_PROPERTIES(jointTranslations, setJointTranslations);
     SET_ENTITY_PROPERTY_FROM_PROPERTIES(relayParentJoints, setRelayParentJoints);
 
-    bool somethingChangedInAnimations = _animationProperties.setProperties(properties);
+    withWriteLock([&] {
-
+        AnimationPropertyGroup animationProperties = _animationProperties;
-    if (somethingChangedInAnimations) {
+        animationProperties.setProperties(properties);
-        _flags |= Simulation::DIRTY_UPDATEABLE;
+        bool somethingChangedInAnimations = applyNewAnimationProperties(animationProperties);
-    }
+        somethingChanged = somethingChanged || somethingChangedInAnimations;
-    somethingChanged = somethingChanged || somethingChangedInAnimations;
+    });
 
     if (somethingChanged) {
         bool wantDebug = false;
@@ -118,12 +118,16 @@ int ModelEntityItem::readEntitySubclassDataFromBuffer(const unsigned char* data,
     READ_ENTITY_PROPERTY(PROP_TEXTURES, QString, setTextures);
     READ_ENTITY_PROPERTY(PROP_RELAY_PARENT_JOINTS, bool, setRelayParentJoints);
 
+    // grab a local copy of _animationProperties to avoid multiple locks
     int bytesFromAnimation;
-    withWriteLock([&] {
+    withReadLock([&] {
-        // Note: since we've associated our _animationProperties with our _animationLoop, the readEntitySubclassDataFromBuffer()
+        AnimationPropertyGroup animationProperties = _animationProperties;
-        // will automatically read into the animation loop
+        bytesFromAnimation = animationProperties.readEntitySubclassDataFromBuffer(dataAt, (bytesLeftToRead - bytesRead), args,
-        bytesFromAnimation = _animationProperties.readEntitySubclassDataFromBuffer(dataAt, (bytesLeftToRead - bytesRead), args,
             propertyFlags, overwriteLocalData, animationPropertiesChanged);
+        if (animationPropertiesChanged) {
+            applyNewAnimationProperties(animationProperties);
+            somethingChanged = true;
+        }
     });
 
     bytesRead += bytesFromAnimation;
@@ -131,11 +135,6 @@ int ModelEntityItem::readEntitySubclassDataFromBuffer(const unsigned char* data,
 
     READ_ENTITY_PROPERTY(PROP_SHAPE_TYPE, ShapeType, setShapeType);
 
-    if (animationPropertiesChanged) {
-        _flags |= Simulation::DIRTY_UPDATEABLE;
-        somethingChanged = true;
-    }
-
     READ_ENTITY_PROPERTY(PROP_JOINT_ROTATIONS_SET, QVector<bool>, setJointRotationsSet);
     READ_ENTITY_PROPERTY(PROP_JOINT_ROTATIONS, QVector<glm::quat>, setJointRotations);
     READ_ENTITY_PROPERTY(PROP_JOINT_TRANSLATIONS_SET, QVector<bool>, setJointTranslationsSet);
@@ -194,88 +193,38 @@ void ModelEntityItem::appendSubclassData(OctreePacketData* packetData, EncodeBit
 
 // added update function back for property fix
 void ModelEntityItem::update(const quint64& now) {
-    auto currentAnimationProperties = this->getAnimationProperties();
+    assert(_lastAnimated > 0);
-    if (_previousAnimationProperties != currentAnimationProperties) {
-        withWriteLock([&] {
-            // if we hit start animation or change the first or last frame then restart the animation
-            if ((currentAnimationProperties.getFirstFrame() != _previousAnimationProperties.getFirstFrame()) ||
-                (currentAnimationProperties.getLastFrame() != _previousAnimationProperties.getLastFrame()) ||
-                (currentAnimationProperties.getRunning() && !_previousAnimationProperties.getRunning())) {
 
-                // when we start interface and the property is are set then the current frame is initialized to -1
+    // increment timestamp before checking "hold"
-                if (_currentFrame < 0) {
-                    // don't reset _lastAnimated here because we need the timestamp from the ModelEntityItem constructor for when the properties were set
-                    _currentFrame = currentAnimationProperties.getCurrentFrame();
-                    setAnimationCurrentFrame(_currentFrame);
-                } else {
-                    _lastAnimated =  usecTimestampNow();
-                    _currentFrame = currentAnimationProperties.getFirstFrame();
-                    setAnimationCurrentFrame(currentAnimationProperties.getFirstFrame());
-                }
-            } else if (!currentAnimationProperties.getRunning() && _previousAnimationProperties.getRunning()) {
-                _currentFrame = currentAnimationProperties.getFirstFrame();
-                setAnimationCurrentFrame(_currentFrame);
-            } else if (currentAnimationProperties.getCurrentFrame() != _previousAnimationProperties.getCurrentFrame()) {
-                // don't reset _lastAnimated here because the currentFrame was set with the previous setting of _lastAnimated
-                _currentFrame = currentAnimationProperties.getCurrentFrame();
-            }
-
-        });
-        _previousAnimationProperties = this->getAnimationProperties();
-    }
-    if (!(getAnimationFirstFrame() < 0) && !(getAnimationFirstFrame() > getAnimationLastFrame())) {
-        updateFrameCount();
-    }
-    EntityItem::update(now);
-}
-
-bool ModelEntityItem::needsToCallUpdate() const {
-    return isAnimatingSomething();
-}
-
-void ModelEntityItem::updateFrameCount() {
-
-    if (_currentFrame < 0.0f) {
-        return;
-    }
-
-    if (!_lastAnimated) {
-        _lastAnimated = usecTimestampNow();
-        return;
-    }
-
-    auto now = usecTimestampNow();
-
-    // update the interval since the last animation.
     auto interval = now - _lastAnimated;
     _lastAnimated = now;
 
-    // if fps is negative then increment timestamp and return.
+    // grab a local copy of _animationProperties to avoid multiple locks
-    if (getAnimationFPS() < 0.0f) {
+    auto animationProperties = getAnimationProperties();
+
+    // bail on "hold"
+    if (animationProperties.getHold()) {
         return;
     }
 
-    int updatedFrameCount = getAnimationLastFrame() - getAnimationFirstFrame() + 1;
+    // increment animation frame
-
+    _currentFrame += (animationProperties.getFPS() * ((float)interval) / (float)USECS_PER_SECOND);
-    if (!getAnimationHold() && getAnimationIsPlaying()) {
+    if (_currentFrame > animationProperties.getLastFrame() + 1.0f) {
-        float deltaTime = (float)interval / (float)USECS_PER_SECOND;
+        if (animationProperties.getLoop()) {
-        _currentFrame += (deltaTime * getAnimationFPS());
+            _currentFrame = animationProperties.computeLoopedFrame(_currentFrame);
-        if (_currentFrame > getAnimationLastFrame() + 1) {
+        } else {
-            if (getAnimationLoop() && getAnimationFirstFrame() != getAnimationLastFrame()) {
+            _currentFrame = animationProperties.getLastFrame();
-                _currentFrame = getAnimationFirstFrame() + (int)(_currentFrame - getAnimationFirstFrame()) % updatedFrameCount;
+        }
-            } else {
+    } else if (_currentFrame < animationProperties.getFirstFrame()) {
-                _currentFrame = getAnimationLastFrame();
+        if (animationProperties.getFirstFrame() < 0.0f) {
-            }
+            _currentFrame = 0.0f;
-        } else if (_currentFrame < getAnimationFirstFrame()) {
+        } else {
-            if (getAnimationFirstFrame() < 0) {
+            _currentFrame = animationProperties.getFirstFrame();
-                _currentFrame = 0;
-            } else {
-                _currentFrame = getAnimationFirstFrame();
-            }
         }
-        // qCDebug(entities)  << "in update frame " << _currentFrame;
-        setAnimationCurrentFrame(_currentFrame);
     }
+    setAnimationCurrentFrame(_currentFrame);
+
+    EntityItem::update(now);
 }
 
 void ModelEntityItem::debugDump() const {
@@ -351,67 +300,61 @@ void ModelEntityItem::setAnimationURL(const QString& url) {
 }
 
 void ModelEntityItem::setAnimationSettings(const QString& value) {
-    // the animations setting is a JSON string that may contain various animation settings.
+    // NOTE: this method only called for old bitstream format
-    // if it includes fps, currentFrame, or running, those values will be parsed out and
-    // will over ride the regular animation settings
 
-    QJsonDocument settingsAsJson = QJsonDocument::fromJson(value.toUtf8());
+    withWriteLock([&] {
-    QJsonObject settingsAsJsonObject = settingsAsJson.object();
+        auto animationProperties = _animationProperties;
-    QVariantMap settingsMap = settingsAsJsonObject.toVariantMap();
-    if (settingsMap.contains("fps")) {
-        float fps = settingsMap["fps"].toFloat();
-        setAnimationFPS(fps);
-    }
 
-    // old settings used frameIndex
+        // the animations setting is a JSON string that may contain various animation settings.
-    if (settingsMap.contains("frameIndex")) {
+        // if it includes fps, currentFrame, or running, those values will be parsed out and
-        float currentFrame = settingsMap["frameIndex"].toFloat();
+        // will over ride the regular animation settings
-#ifdef WANT_DEBUG
+        QJsonDocument settingsAsJson = QJsonDocument::fromJson(value.toUtf8());
-        if (!getAnimationURL().isEmpty()) {
+        QJsonObject settingsAsJsonObject = settingsAsJson.object();
-            qCDebug(entities) << "ModelEntityItem::setAnimationSettings() calling setAnimationFrameIndex()...";
+        QVariantMap settingsMap = settingsAsJsonObject.toVariantMap();
-            qCDebug(entities) << "    model URL:" << getModelURL();
+        if (settingsMap.contains("fps")) {
-            qCDebug(entities) << "    animation URL:" << getAnimationURL();
+            float fps = settingsMap["fps"].toFloat();
-            qCDebug(entities) << "    settings:" << value;
+            animationProperties.setFPS(fps);
-            qCDebug(entities) << "    settingsMap[frameIndex]:" << settingsMap["frameIndex"];
-            qCDebug(entities"    currentFrame: %20.5f", currentFrame);
         }
-#endif
 
-        setAnimationCurrentFrame(currentFrame);
+        // old settings used frameIndex
-    }
+        if (settingsMap.contains("frameIndex")) {
-
+            float currentFrame = settingsMap["frameIndex"].toFloat();
-    if (settingsMap.contains("running")) {
+            animationProperties.setCurrentFrame(currentFrame);
-        bool running = settingsMap["running"].toBool();
-        if (running != getAnimationIsPlaying()) {
-            setAnimationIsPlaying(running);
         }
-    }
 
-    if (settingsMap.contains("firstFrame")) {
+        if (settingsMap.contains("running")) {
-        float firstFrame = settingsMap["firstFrame"].toFloat();
+            bool running = settingsMap["running"].toBool();
-        setAnimationFirstFrame(firstFrame);
+            if (running != animationProperties.getRunning()) {
-    }
+                animationProperties.setRunning(running);
+            }
+        }
 
-    if (settingsMap.contains("lastFrame")) {
+        if (settingsMap.contains("firstFrame")) {
-        float lastFrame = settingsMap["lastFrame"].toFloat();
+            float firstFrame = settingsMap["firstFrame"].toFloat();
-        setAnimationLastFrame(lastFrame);
+            animationProperties.setFirstFrame(firstFrame);
-    }
+        }
 
-    if (settingsMap.contains("loop")) {
+        if (settingsMap.contains("lastFrame")) {
-        bool loop = settingsMap["loop"].toBool();
+            float lastFrame = settingsMap["lastFrame"].toFloat();
-        setAnimationLoop(loop);
+            animationProperties.setLastFrame(lastFrame);
-    }
+        }
 
-    if (settingsMap.contains("hold")) {
+        if (settingsMap.contains("loop")) {
-        bool hold = settingsMap["hold"].toBool();
+            bool loop = settingsMap["loop"].toBool();
-        setAnimationHold(hold);
+            animationProperties.setLoop(loop);
-    }
+        }
 
-    if (settingsMap.contains("allowTranslation")) {
+        if (settingsMap.contains("hold")) {
-        bool allowTranslation = settingsMap["allowTranslation"].toBool();
+            bool hold = settingsMap["hold"].toBool();
-        setAnimationAllowTranslation(allowTranslation);
+            animationProperties.setHold(hold);
-    }
+        }
-    _flags |= Simulation::DIRTY_UPDATEABLE;
+
+        if (settingsMap.contains("allowTranslation")) {
+            bool allowTranslation = settingsMap["allowTranslation"].toBool();
+            animationProperties.setAllowTranslation(allowTranslation);
+        }
+        applyNewAnimationProperties(animationProperties);
+    });
 }
 
 void ModelEntityItem::setAnimationIsPlaying(bool value) {
@@ -705,8 +648,43 @@ float ModelEntityItem::getAnimationFPS() const {
 
 bool ModelEntityItem::isAnimatingSomething() const {
     return resultWithReadLock<bool>([&] {
-        return !_animationProperties.getURL().isEmpty() &&
+        return _animationProperties.isValidAndRunning();
-            _animationProperties.getRunning() &&
+    });
-            (_animationProperties.getFPS() != 0.0f);
+}
-        });
+
+bool ModelEntityItem::applyNewAnimationProperties(AnimationPropertyGroup newProperties) {
+    // call applyNewAnimationProperties() whenever trying to update _animationProperties
+    // because there is some reset logic we need to do whenever the animation "config" properties change
+    // NOTE: this private method is always called inside withWriteLock()
+
+    // if we hit start animation or change the first or last frame then restart the animation
+    if ((newProperties.getFirstFrame() != _animationProperties.getFirstFrame()) ||
+        (newProperties.getLastFrame() != _animationProperties.getLastFrame()) ||
+        (newProperties.getRunning() && !_animationProperties.getRunning())) {
+
+        // when we start interface and the property is are set then the current frame is initialized to -1
+        if (_currentFrame < 0.0f) {
+            // don't reset _lastAnimated here because we need the timestamp from the ModelEntityItem constructor for when the properties were set
+            _currentFrame = newProperties.getCurrentFrame();
+            newProperties.setCurrentFrame(_currentFrame);
+        } else {
+            _lastAnimated =  usecTimestampNow();
+            _currentFrame = newProperties.getFirstFrame();
+            newProperties.setCurrentFrame(newProperties.getFirstFrame());
+        }
+    } else if (!newProperties.getRunning() && _animationProperties.getRunning()) {
+        _currentFrame = newProperties.getFirstFrame();
+        newProperties.setCurrentFrame(_currentFrame);
+    } else if (newProperties.getCurrentFrame() != _animationProperties.getCurrentFrame()) {
+        // don't reset _lastAnimated here because the currentFrame was set with the previous setting of _lastAnimated
+        _currentFrame = newProperties.getCurrentFrame();
+    }
+
+    // finally apply the changes
+    bool somethingChanged = newProperties != _animationProperties;
+    if (somethingChanged) {
+        _animationProperties = newProperties;
+        _flags |= Simulation::DIRTY_UPDATEABLE;
+    }
+    return somethingChanged;
 }

libraries/entities/src/ModelEntityItem.h

@@ -46,10 +46,9 @@ public:
                                                 EntityPropertyFlags& propertyFlags, bool overwriteLocalData,
                                                 bool& somethingChanged) override;
 
-    // update() and needstocallupdate() added back for the entity property fix
+
     virtual void update(const quint64& now) override;
-    virtual bool needsToCallUpdate() const override;
+    bool needsToCallUpdate() const override { return isAnimatingSomething(); }
-    void updateFrameCount();
 
     virtual void debugDump() const override;
 
@@ -132,6 +131,7 @@ public:
 
 private:
     void setAnimationSettings(const QString& value); // only called for old bitstream format
+    bool applyNewAnimationProperties(AnimationPropertyGroup newProperties);
     ShapeType computeTrueShapeType() const;
 
 protected:
@@ -172,7 +172,6 @@ protected:
 
 private:
     uint64_t _lastAnimated{ 0 };
-    AnimationPropertyGroup _previousAnimationProperties;
     float _currentFrame{ -1.0f };
 };