From e36dbab838424dba86170b94388aa8ae35be9686 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Fri, 6 Apr 2018 16:52:49 -0700 Subject: [PATCH 1/6] remove unused data member --- libraries/entities-renderer/src/RenderableZoneEntityItem.h | 1 - 1 file changed, 1 deletion(-) diff --git a/libraries/entities-renderer/src/RenderableZoneEntityItem.h b/libraries/entities-renderer/src/RenderableZoneEntityItem.h index c5824abef0..2f8fd47b79 100644 --- a/libraries/entities-renderer/src/RenderableZoneEntityItem.h +++ b/libraries/entities-renderer/src/RenderableZoneEntityItem.h @@ -69,7 +69,6 @@ private: graphics::SkyboxPointer editSkybox() { return editBackground()->getSkybox(); } graphics::HazePointer editHaze() { _needHazeUpdate = true; return _haze; } - bool _needsInitialSimulation{ true }; glm::vec3 _lastPosition; glm::vec3 _lastDimensions; glm::quat _lastRotation; From 75508385cf888e23683e99f3fc82de167600379c Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Fri, 6 Apr 2018 16:53:08 -0700 Subject: [PATCH 2/6] more correct needsToCallUpdate() implementations --- libraries/entities/src/MaterialEntityItem.h | 2 +- libraries/entities/src/ModelEntityItem.cpp | 67 +++++++++------------ libraries/entities/src/PolyLineEntityItem.h | 2 - 3 files changed, 29 insertions(+), 42 deletions(-) diff --git a/libraries/entities/src/MaterialEntityItem.h b/libraries/entities/src/MaterialEntityItem.h index 30743850dd..92289cdf73 100644 --- a/libraries/entities/src/MaterialEntityItem.h +++ b/libraries/entities/src/MaterialEntityItem.h @@ -26,7 +26,7 @@ public: ALLOW_INSTANTIATION // This class can be instantiated void update(const quint64& now) override; - bool needsToCallUpdate() const override { return true; } + bool needsToCallUpdate() const override { return _retryApply; } // methods for getting/setting all properties of an entity virtual EntityItemProperties getProperties(EntityPropertyFlags desiredProperties = EntityPropertyFlags()) const override; diff --git a/libraries/entities/src/ModelEntityItem.cpp b/libraries/entities/src/ModelEntityItem.cpp index be62664ff9..6552de87d1 100644 --- a/libraries/entities/src/ModelEntityItem.cpp +++ b/libraries/entities/src/ModelEntityItem.cpp @@ -194,53 +194,43 @@ void ModelEntityItem::appendSubclassData(OctreePacketData* packetData, EncodeBit // added update function back for property fix void ModelEntityItem::update(const quint64& now) { + auto currentAnimationProperties = this->getAnimationProperties(); + 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())) { - { - auto currentAnimationProperties = this->getAnimationProperties(); - - 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 - 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 + // when we start interface and the property is are set then the current frame is initialized to -1 + 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()); } - - }); - _previousAnimationProperties = this->getAnimationProperties(); - - } - - if (isAnimatingSomething()) { - if (!(getAnimationFirstFrame() < 0) && !(getAnimationFirstFrame() > getAnimationLastFrame())) { - updateFrameCount(); + } 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 true; + return isAnimatingSomething(); } void ModelEntityItem::updateFrameCount() { @@ -713,7 +703,6 @@ float ModelEntityItem::getAnimationFPS() const { }); } - bool ModelEntityItem::isAnimatingSomething() const { return resultWithReadLock([&] { return !_animationProperties.getURL().isEmpty() && diff --git a/libraries/entities/src/PolyLineEntityItem.h b/libraries/entities/src/PolyLineEntityItem.h index 871c451c50..c76419af02 100644 --- a/libraries/entities/src/PolyLineEntityItem.h +++ b/libraries/entities/src/PolyLineEntityItem.h @@ -77,8 +77,6 @@ class PolyLineEntityItem : public EntityItem { QString getTextures() const; void setTextures(const QString& textures); - virtual bool needsToCallUpdate() const override { return true; } - virtual ShapeType getShapeType() const override { return SHAPE_TYPE_NONE; } bool pointsChanged() const { return _pointsChanged; } From b48e9495a5484d09703a53175b02aa66a3661345 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Fri, 6 Apr 2018 17:32:57 -0700 Subject: [PATCH 3/6] rollback change to MaterialEntityItem, we'll fix later --- libraries/entities/src/MaterialEntityItem.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/entities/src/MaterialEntityItem.h b/libraries/entities/src/MaterialEntityItem.h index 92289cdf73..30743850dd 100644 --- a/libraries/entities/src/MaterialEntityItem.h +++ b/libraries/entities/src/MaterialEntityItem.h @@ -26,7 +26,7 @@ public: ALLOW_INSTANTIATION // This class can be instantiated void update(const quint64& now) override; - bool needsToCallUpdate() const override { return _retryApply; } + bool needsToCallUpdate() const override { return true; } // methods for getting/setting all properties of an entity virtual EntityItemProperties getProperties(EntityPropertyFlags desiredProperties = EntityPropertyFlags()) const override; From 8e51d6609201da6055723fe11e25ba6bc8c6d841 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Thu, 19 Apr 2018 11:46:47 -0700 Subject: [PATCH 4/6] fix bug: animation should reset on start running --- .../entities/src/AnimationPropertyGroup.cpp | 19 +- .../entities/src/AnimationPropertyGroup.h | 4 + libraries/entities/src/EntityItem.h | 2 +- libraries/entities/src/ModelEntityItem.cpp | 205 ++++++++---------- libraries/entities/src/ModelEntityItem.h | 7 +- 5 files changed, 117 insertions(+), 120 deletions(-) diff --git a/libraries/entities/src/AnimationPropertyGroup.cpp b/libraries/entities/src/AnimationPropertyGroup.cpp index 82af60ed1a..43c6b7a6a5 100644 --- a/libraries/entities/src/AnimationPropertyGroup.cpp +++ b/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()); +} diff --git a/libraries/entities/src/AnimationPropertyGroup.h b/libraries/entities/src/AnimationPropertyGroup.h index 54d4ced92f..bebfe2c194 100644 --- a/libraries/entities/src/AnimationPropertyGroup.h +++ b/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); diff --git a/libraries/entities/src/EntityItem.h b/libraries/entities/src/EntityItem.h index 0557bbe5ad..a88250a133 100644 --- a/libraries/entities/src/EntityItem.h +++ b/libraries/entities/src/EntityItem.h @@ -597,7 +597,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 diff --git a/libraries/entities/src/ModelEntityItem.cpp b/libraries/entities/src/ModelEntityItem.cpp index 6552de87d1..d8eb8b1e7a 100644 --- a/libraries/entities/src/ModelEntityItem.cpp +++ b/libraries/entities/src/ModelEntityItem.cpp @@ -82,11 +82,9 @@ 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); - - if (somethingChangedInAnimations) { - _flags |= Simulation::DIRTY_UPDATEABLE; - } + AnimationPropertyGroup animationProperties = getAnimationProperties(); + animationProperties.setProperties(properties); + bool somethingChangedInAnimations = applyNewAnimationProperties(animationProperties); somethingChanged = somethingChanged || somethingChangedInAnimations; if (somethingChanged) { @@ -118,24 +116,21 @@ int ModelEntityItem::readEntitySubclassDataFromBuffer(const unsigned char* data, READ_ENTITY_PROPERTY(PROP_TEXTURES, QString, setTextures); READ_ENTITY_PROPERTY(PROP_RELAY_PARENT_JOINTS, bool, setRelayParentJoints); - int bytesFromAnimation; - withWriteLock([&] { - // Note: since we've associated our _animationProperties with our _animationLoop, the readEntitySubclassDataFromBuffer() - // will automatically read into the animation loop - bytesFromAnimation = _animationProperties.readEntitySubclassDataFromBuffer(dataAt, (bytesLeftToRead - bytesRead), args, - propertyFlags, overwriteLocalData, animationPropertiesChanged); - }); + // grab a local copy of _animationProperties to avoid multiple locks + AnimationPropertyGroup animationProperties = getAnimationProperties(); + int bytesFromAnimation = animationProperties.readEntitySubclassDataFromBuffer(dataAt, (bytesLeftToRead - bytesRead), args, + propertyFlags, overwriteLocalData, animationPropertiesChanged); + if (animationPropertiesChanged) { + applyNewAnimationProperties(animationProperties); + _flags |= Simulation::DIRTY_UPDATEABLE; + somethingChanged = true; + } bytesRead += bytesFromAnimation; dataAt += bytesFromAnimation; READ_ENTITY_PROPERTY(PROP_SHAPE_TYPE, ShapeType, setShapeType); - if (animationPropertiesChanged) { - _flags |= Simulation::DIRTY_UPDATEABLE; - somethingChanged = true; - } - READ_ENTITY_PROPERTY(PROP_JOINT_ROTATIONS_SET, QVector, setJointRotationsSet); READ_ENTITY_PROPERTY(PROP_JOINT_ROTATIONS, QVector, setJointRotations); READ_ENTITY_PROPERTY(PROP_JOINT_TRANSLATIONS_SET, QVector, setJointTranslationsSet); @@ -194,88 +189,38 @@ void ModelEntityItem::appendSubclassData(OctreePacketData* packetData, EncodeBit // added update function back for property fix void ModelEntityItem::update(const quint64& now) { - auto currentAnimationProperties = this->getAnimationProperties(); - 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())) { + assert(_lastAnimated > 0); - // when we start interface and the property is are set then the current frame is initialized to -1 - 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. + // increment timestamp before checking "hold" auto interval = now - _lastAnimated; _lastAnimated = now; - // if fps is negative then increment timestamp and return. - if (getAnimationFPS() < 0.0f) { + // grab a local copy of _animationProperties to avoid multiple locks + auto animationProperties = getAnimationProperties(); + + // bail on "hold" + if (animationProperties.getHold()) { return; } - int updatedFrameCount = getAnimationLastFrame() - getAnimationFirstFrame() + 1; - - if (!getAnimationHold() && getAnimationIsPlaying()) { - float deltaTime = (float)interval / (float)USECS_PER_SECOND; - _currentFrame += (deltaTime * getAnimationFPS()); - if (_currentFrame > getAnimationLastFrame() + 1) { - if (getAnimationLoop() && getAnimationFirstFrame() != getAnimationLastFrame()) { - _currentFrame = getAnimationFirstFrame() + (int)(_currentFrame - getAnimationFirstFrame()) % updatedFrameCount; - } else { - _currentFrame = getAnimationLastFrame(); - } - } else if (_currentFrame < getAnimationFirstFrame()) { - if (getAnimationFirstFrame() < 0) { - _currentFrame = 0; - } else { - _currentFrame = getAnimationFirstFrame(); - } + // increment animation frame + _currentFrame += (animationProperties.getFPS() * ((float)interval) / (float)USECS_PER_SECOND); + if (_currentFrame > animationProperties.getLastFrame() + 1.0f) { + if (animationProperties.getLoop()) { + _currentFrame = animationProperties.computeLoopedFrame(_currentFrame); + } else { + _currentFrame = animationProperties.getLastFrame(); + } + } else if (_currentFrame < animationProperties.getFirstFrame()) { + if (animationProperties.getFirstFrame() < 0.0f) { + _currentFrame = 0.0f; + } else { + _currentFrame = animationProperties.getFirstFrame(); } - // qCDebug(entities) << "in update frame " << _currentFrame; - setAnimationCurrentFrame(_currentFrame); } + setAnimationCurrentFrame(_currentFrame); + + EntityItem::update(now); } void ModelEntityItem::debugDump() const { @@ -351,10 +296,11 @@ void ModelEntityItem::setAnimationURL(const QString& url) { } void ModelEntityItem::setAnimationSettings(const QString& value) { + // NOTE: this method only called for old bitstream format + // the animations setting is a JSON string that may contain various animation settings. // 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()); QJsonObject settingsAsJsonObject = settingsAsJson.object(); QVariantMap settingsMap = settingsAsJsonObject.toVariantMap(); @@ -363,54 +309,48 @@ void ModelEntityItem::setAnimationSettings(const QString& value) { setAnimationFPS(fps); } + // grab a local copy of _animationProperties to avoid multiple locks + auto animationProperties = getAnimationProperties(); + // old settings used frameIndex if (settingsMap.contains("frameIndex")) { float currentFrame = settingsMap["frameIndex"].toFloat(); -#ifdef WANT_DEBUG - if (!getAnimationURL().isEmpty()) { - qCDebug(entities) << "ModelEntityItem::setAnimationSettings() calling setAnimationFrameIndex()..."; - qCDebug(entities) << " model URL:" << getModelURL(); - qCDebug(entities) << " animation URL:" << getAnimationURL(); - qCDebug(entities) << " settings:" << value; - qCDebug(entities) << " settingsMap[frameIndex]:" << settingsMap["frameIndex"]; - qCDebug(entities" currentFrame: %20.5f", currentFrame); - } -#endif - - setAnimationCurrentFrame(currentFrame); + animationProperties.setCurrentFrame(currentFrame); } if (settingsMap.contains("running")) { bool running = settingsMap["running"].toBool(); - if (running != getAnimationIsPlaying()) { - setAnimationIsPlaying(running); + if (running != animationProperties.getRunning()) { + animationProperties.setRunning(running); } } if (settingsMap.contains("firstFrame")) { float firstFrame = settingsMap["firstFrame"].toFloat(); - setAnimationFirstFrame(firstFrame); + animationProperties.setFirstFrame(firstFrame); } if (settingsMap.contains("lastFrame")) { float lastFrame = settingsMap["lastFrame"].toFloat(); - setAnimationLastFrame(lastFrame); + animationProperties.setLastFrame(lastFrame); } if (settingsMap.contains("loop")) { bool loop = settingsMap["loop"].toBool(); - setAnimationLoop(loop); + animationProperties.setLoop(loop); } if (settingsMap.contains("hold")) { bool hold = settingsMap["hold"].toBool(); - setAnimationHold(hold); + animationProperties.setHold(hold); } if (settingsMap.contains("allowTranslation")) { bool allowTranslation = settingsMap["allowTranslation"].toBool(); - setAnimationAllowTranslation(allowTranslation); + animationProperties.setAllowTranslation(allowTranslation); } + applyNewAnimationProperties(animationProperties); + // TODO? set the dirty flag inside applyNewAnimationProperties() _flags |= Simulation::DIRTY_UPDATEABLE; } @@ -705,8 +645,45 @@ float ModelEntityItem::getAnimationFPS() const { bool ModelEntityItem::isAnimatingSomething() const { return resultWithReadLock([&] { - return !_animationProperties.getURL().isEmpty() && - _animationProperties.getRunning() && - (_animationProperties.getFPS() != 0.0f); - }); + return _animationProperties.isValidAndRunning(); + }); +} + +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 + + // grab a local copy of _animationProperties to avoid multiple locks + AnimationPropertyGroup oldProperties = getAnimationProperties(); + + // if we hit start animation or change the first or last frame then restart the animation + if ((newProperties.getFirstFrame() != oldProperties.getFirstFrame()) || + (newProperties.getLastFrame() != oldProperties.getLastFrame()) || + (newProperties.getRunning() && !oldProperties.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() && oldProperties.getRunning()) { + _currentFrame = newProperties.getFirstFrame(); + newProperties.setCurrentFrame(_currentFrame); + } else if (newProperties.getCurrentFrame() != oldProperties.getCurrentFrame()) { + // don't reset _lastAnimated here because the currentFrame was set with the previous setting of _lastAnimated + _currentFrame = newProperties.getCurrentFrame(); + } + + // finally apply the changes with a lock + withWriteLock([&] { + _animationProperties = newProperties; + }); + + // return 'true' if something changed + return newProperties != oldProperties; } diff --git a/libraries/entities/src/ModelEntityItem.h b/libraries/entities/src/ModelEntityItem.h index 327606ae2f..91b44359f7 100644 --- a/libraries/entities/src/ModelEntityItem.h +++ b/libraries/entities/src/ModelEntityItem.h @@ -46,10 +46,10 @@ public: EntityPropertyFlags& propertyFlags, bool overwriteLocalData, bool& somethingChanged) override; - // update() and needstocallupdate() added back for the entity property fix + + bool applyNewAnimationProperties(AnimationPropertyGroup newProperties); virtual void update(const quint64& now) override; - virtual bool needsToCallUpdate() const override; - void updateFrameCount(); + bool needsToCallUpdate() const override { return isAnimatingSomething(); } virtual void debugDump() const override; @@ -172,7 +172,6 @@ protected: private: uint64_t _lastAnimated{ 0 }; - AnimationPropertyGroup _previousAnimationProperties; float _currentFrame{ -1.0f }; }; From 748af5a65f219139caa97b519023c8ce0be5c825 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Thu, 19 Apr 2018 12:30:16 -0700 Subject: [PATCH 5/6] worry about multi-threads when updating animation props --- libraries/entities/src/ModelEntityItem.cpp | 149 +++++++++++---------- libraries/entities/src/ModelEntityItem.h | 2 +- 2 files changed, 76 insertions(+), 75 deletions(-) diff --git a/libraries/entities/src/ModelEntityItem.cpp b/libraries/entities/src/ModelEntityItem.cpp index d8eb8b1e7a..f759de915e 100644 --- a/libraries/entities/src/ModelEntityItem.cpp +++ b/libraries/entities/src/ModelEntityItem.cpp @@ -82,10 +82,12 @@ bool ModelEntityItem::setProperties(const EntityItemProperties& properties) { SET_ENTITY_PROPERTY_FROM_PROPERTIES(jointTranslations, setJointTranslations); SET_ENTITY_PROPERTY_FROM_PROPERTIES(relayParentJoints, setRelayParentJoints); - AnimationPropertyGroup animationProperties = getAnimationProperties(); - animationProperties.setProperties(properties); - bool somethingChangedInAnimations = applyNewAnimationProperties(animationProperties); - somethingChanged = somethingChanged || somethingChangedInAnimations; + withWriteLock([&] { + AnimationPropertyGroup animationProperties = _animationProperties; + animationProperties.setProperties(properties); + bool somethingChangedInAnimations = applyNewAnimationProperties(animationProperties); + somethingChanged = somethingChanged || somethingChangedInAnimations; + }); if (somethingChanged) { bool wantDebug = false; @@ -117,14 +119,17 @@ int ModelEntityItem::readEntitySubclassDataFromBuffer(const unsigned char* data, READ_ENTITY_PROPERTY(PROP_RELAY_PARENT_JOINTS, bool, setRelayParentJoints); // grab a local copy of _animationProperties to avoid multiple locks - AnimationPropertyGroup animationProperties = getAnimationProperties(); - int bytesFromAnimation = animationProperties.readEntitySubclassDataFromBuffer(dataAt, (bytesLeftToRead - bytesRead), args, - propertyFlags, overwriteLocalData, animationPropertiesChanged); - if (animationPropertiesChanged) { - applyNewAnimationProperties(animationProperties); - _flags |= Simulation::DIRTY_UPDATEABLE; - somethingChanged = true; - } + int bytesFromAnimation; + withReadLock([&] { + AnimationPropertyGroup animationProperties = _animationProperties; + bytesFromAnimation = animationProperties.readEntitySubclassDataFromBuffer(dataAt, (bytesLeftToRead - bytesRead), args, + propertyFlags, overwriteLocalData, animationPropertiesChanged); + if (animationPropertiesChanged) { + applyNewAnimationProperties(animationProperties); + _flags |= Simulation::DIRTY_UPDATEABLE; + somethingChanged = true; + } + }); bytesRead += bytesFromAnimation; dataAt += bytesFromAnimation; @@ -298,60 +303,61 @@ void ModelEntityItem::setAnimationURL(const QString& url) { void ModelEntityItem::setAnimationSettings(const QString& value) { // NOTE: this method only called for old bitstream format - // the animations setting is a JSON string that may contain various animation settings. - // 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()); - QJsonObject settingsAsJsonObject = settingsAsJson.object(); - QVariantMap settingsMap = settingsAsJsonObject.toVariantMap(); - if (settingsMap.contains("fps")) { - float fps = settingsMap["fps"].toFloat(); - setAnimationFPS(fps); - } + withWriteLock([&] { + auto animationProperties = _animationProperties; - // grab a local copy of _animationProperties to avoid multiple locks - auto animationProperties = getAnimationProperties(); - - // old settings used frameIndex - if (settingsMap.contains("frameIndex")) { - float currentFrame = settingsMap["frameIndex"].toFloat(); - animationProperties.setCurrentFrame(currentFrame); - } - - if (settingsMap.contains("running")) { - bool running = settingsMap["running"].toBool(); - if (running != animationProperties.getRunning()) { - animationProperties.setRunning(running); + // the animations setting is a JSON string that may contain various animation settings. + // 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()); + QJsonObject settingsAsJsonObject = settingsAsJson.object(); + QVariantMap settingsMap = settingsAsJsonObject.toVariantMap(); + if (settingsMap.contains("fps")) { + float fps = settingsMap["fps"].toFloat(); + animationProperties.setFPS(fps); } - } - if (settingsMap.contains("firstFrame")) { - float firstFrame = settingsMap["firstFrame"].toFloat(); - animationProperties.setFirstFrame(firstFrame); - } + // old settings used frameIndex + if (settingsMap.contains("frameIndex")) { + float currentFrame = settingsMap["frameIndex"].toFloat(); + animationProperties.setCurrentFrame(currentFrame); + } - if (settingsMap.contains("lastFrame")) { - float lastFrame = settingsMap["lastFrame"].toFloat(); - animationProperties.setLastFrame(lastFrame); - } + if (settingsMap.contains("running")) { + bool running = settingsMap["running"].toBool(); + if (running != animationProperties.getRunning()) { + animationProperties.setRunning(running); + } + } - if (settingsMap.contains("loop")) { - bool loop = settingsMap["loop"].toBool(); - animationProperties.setLoop(loop); - } + if (settingsMap.contains("firstFrame")) { + float firstFrame = settingsMap["firstFrame"].toFloat(); + animationProperties.setFirstFrame(firstFrame); + } - if (settingsMap.contains("hold")) { - bool hold = settingsMap["hold"].toBool(); - animationProperties.setHold(hold); - } + if (settingsMap.contains("lastFrame")) { + float lastFrame = settingsMap["lastFrame"].toFloat(); + animationProperties.setLastFrame(lastFrame); + } - if (settingsMap.contains("allowTranslation")) { - bool allowTranslation = settingsMap["allowTranslation"].toBool(); - animationProperties.setAllowTranslation(allowTranslation); - } - applyNewAnimationProperties(animationProperties); - // TODO? set the dirty flag inside applyNewAnimationProperties() - _flags |= Simulation::DIRTY_UPDATEABLE; + if (settingsMap.contains("loop")) { + bool loop = settingsMap["loop"].toBool(); + animationProperties.setLoop(loop); + } + + if (settingsMap.contains("hold")) { + bool hold = settingsMap["hold"].toBool(); + animationProperties.setHold(hold); + } + + if (settingsMap.contains("allowTranslation")) { + bool allowTranslation = settingsMap["allowTranslation"].toBool(); + animationProperties.setAllowTranslation(allowTranslation); + } + applyNewAnimationProperties(animationProperties); + // TODO? set the dirty flag inside applyNewAnimationProperties() + _flags |= Simulation::DIRTY_UPDATEABLE; + }); } void ModelEntityItem::setAnimationIsPlaying(bool value) { @@ -652,14 +658,12 @@ bool ModelEntityItem::isAnimatingSomething() const { 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 - - // grab a local copy of _animationProperties to avoid multiple locks - AnimationPropertyGroup oldProperties = getAnimationProperties(); + // 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() != oldProperties.getFirstFrame()) || - (newProperties.getLastFrame() != oldProperties.getLastFrame()) || - (newProperties.getRunning() && !oldProperties.getRunning())) { + 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) { @@ -671,19 +675,16 @@ bool ModelEntityItem::applyNewAnimationProperties(AnimationPropertyGroup newProp _currentFrame = newProperties.getFirstFrame(); newProperties.setCurrentFrame(newProperties.getFirstFrame()); } - } else if (!newProperties.getRunning() && oldProperties.getRunning()) { + } else if (!newProperties.getRunning() && _animationProperties.getRunning()) { _currentFrame = newProperties.getFirstFrame(); newProperties.setCurrentFrame(_currentFrame); - } else if (newProperties.getCurrentFrame() != oldProperties.getCurrentFrame()) { + } 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 with a lock - withWriteLock([&] { - _animationProperties = newProperties; - }); - - // return 'true' if something changed - return newProperties != oldProperties; + // finally apply the changes + bool somethingChanged = newProperties != _animationProperties; + _animationProperties = newProperties; + return somethingChanged; } diff --git a/libraries/entities/src/ModelEntityItem.h b/libraries/entities/src/ModelEntityItem.h index 91b44359f7..ad6cdf4040 100644 --- a/libraries/entities/src/ModelEntityItem.h +++ b/libraries/entities/src/ModelEntityItem.h @@ -47,7 +47,6 @@ public: bool& somethingChanged) override; - bool applyNewAnimationProperties(AnimationPropertyGroup newProperties); virtual void update(const quint64& now) override; bool needsToCallUpdate() const override { return isAnimatingSomething(); } @@ -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: From 577f9a03e6586264ce714778ec76d0984e1c3d67 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Mon, 23 Apr 2018 12:07:49 -0700 Subject: [PATCH 6/6] fix bug: animation reset on 'hold' --- libraries/entities/src/ModelEntityItem.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/libraries/entities/src/ModelEntityItem.cpp b/libraries/entities/src/ModelEntityItem.cpp index f759de915e..0f59bc673d 100644 --- a/libraries/entities/src/ModelEntityItem.cpp +++ b/libraries/entities/src/ModelEntityItem.cpp @@ -126,7 +126,6 @@ int ModelEntityItem::readEntitySubclassDataFromBuffer(const unsigned char* data, propertyFlags, overwriteLocalData, animationPropertiesChanged); if (animationPropertiesChanged) { applyNewAnimationProperties(animationProperties); - _flags |= Simulation::DIRTY_UPDATEABLE; somethingChanged = true; } }); @@ -355,8 +354,6 @@ void ModelEntityItem::setAnimationSettings(const QString& value) { animationProperties.setAllowTranslation(allowTranslation); } applyNewAnimationProperties(animationProperties); - // TODO? set the dirty flag inside applyNewAnimationProperties() - _flags |= Simulation::DIRTY_UPDATEABLE; }); } @@ -685,6 +682,9 @@ bool ModelEntityItem::applyNewAnimationProperties(AnimationPropertyGroup newProp // finally apply the changes bool somethingChanged = newProperties != _animationProperties; - _animationProperties = newProperties; + if (somethingChanged) { + _animationProperties = newProperties; + _flags |= Simulation::DIRTY_UPDATEABLE; + } return somethingChanged; }