From f4d8216501356cfd253b5555fad6c770233039e6 Mon Sep 17 00:00:00 2001 From: Brad Davis Date: Wed, 13 Sep 2017 18:06:21 -0700 Subject: [PATCH] Fix race condition accessing QString sub-properties in zones --- .../src/RenderableZoneEntityItem.cpp | 34 ++++++++------ .../src/RenderableZoneEntityItem.h | 10 ++-- libraries/entities/src/ZoneEntityItem.cpp | 47 ++++++++++++++----- libraries/entities/src/ZoneEntityItem.h | 4 +- 4 files changed, 62 insertions(+), 33 deletions(-) diff --git a/libraries/entities-renderer/src/RenderableZoneEntityItem.cpp b/libraries/entities-renderer/src/RenderableZoneEntityItem.cpp index fc15a8539b..caf9cae341 100644 --- a/libraries/entities-renderer/src/RenderableZoneEntityItem.cpp +++ b/libraries/entities-renderer/src/RenderableZoneEntityItem.cpp @@ -176,6 +176,10 @@ void ZoneEntityRenderer::doRenderUpdateSynchronousTyped(const ScenePointer& scen _lastRotation = entity->getRotation(); _lastDimensions = entity->getDimensions(); + _keyLightProperties = entity->getKeyLightProperties(); + _stageProperties = entity->getStageProperties(); + _skyboxProperties = entity->getSkyboxProperties(); + #if 0 if (_lastShapeURL != _typedEntity->getCompoundShapeURL()) { @@ -196,14 +200,14 @@ void ZoneEntityRenderer::doRenderUpdateSynchronousTyped(const ScenePointer& scen } #endif - updateKeyZoneItemFromEntity(entity); + updateKeyZoneItemFromEntity(); if (sunChanged) { - updateKeySunFromEntity(entity); + updateKeySunFromEntity(); } if (sunChanged || skyboxChanged) { - updateKeyAmbientFromEntity(entity); + updateKeyAmbientFromEntity(); } if (backgroundChanged || skyboxChanged) { updateKeyBackgroundFromEntity(entity); @@ -265,19 +269,19 @@ bool ZoneEntityRenderer::needsRenderUpdateFromTypedEntity(const TypedEntityPoint return false; } -void ZoneEntityRenderer::updateKeySunFromEntity(const TypedEntityPointer& entity) { +void ZoneEntityRenderer::updateKeySunFromEntity() { const auto& sunLight = editSunLight(); sunLight->setType(model::Light::SUN); sunLight->setPosition(_lastPosition); sunLight->setOrientation(_lastRotation); // Set the keylight - sunLight->setColor(ColorUtils::toVec3(entity->getKeyLightProperties().getColor())); - sunLight->setIntensity(entity->getKeyLightProperties().getIntensity()); - sunLight->setDirection(entity->getKeyLightProperties().getDirection()); + sunLight->setColor(ColorUtils::toVec3(_keyLightProperties.getColor())); + sunLight->setIntensity(_keyLightProperties.getIntensity()); + sunLight->setDirection(_keyLightProperties.getDirection()); } -void ZoneEntityRenderer::updateKeyAmbientFromEntity(const TypedEntityPointer& entity) { +void ZoneEntityRenderer::updateKeyAmbientFromEntity() { const auto& ambientLight = editAmbientLight(); ambientLight->setType(model::Light::AMBIENT); ambientLight->setPosition(_lastPosition); @@ -285,24 +289,24 @@ void ZoneEntityRenderer::updateKeyAmbientFromEntity(const TypedEntityPointer& en // Set the keylight - ambientLight->setAmbientIntensity(entity->getKeyLightProperties().getAmbientIntensity()); + ambientLight->setAmbientIntensity(_keyLightProperties.getAmbientIntensity()); - if (entity->getKeyLightProperties().getAmbientURL().isEmpty()) { - setAmbientURL(entity->getSkyboxProperties().getURL()); + if (_keyLightProperties.getAmbientURL().isEmpty()) { + setAmbientURL(_skyboxProperties.getURL()); } else { - setAmbientURL(entity->getKeyLightProperties().getAmbientURL()); + setAmbientURL(_keyLightProperties.getAmbientURL()); } } void ZoneEntityRenderer::updateKeyBackgroundFromEntity(const TypedEntityPointer& entity) { editBackground(); setBackgroundMode(entity->getBackgroundMode()); - setSkyboxColor(entity->getSkyboxProperties().getColorVec3()); + setSkyboxColor(_skyboxProperties.getColorVec3()); setProceduralUserData(entity->getUserData()); - setSkyboxURL(entity->getSkyboxProperties().getURL()); + setSkyboxURL(_skyboxProperties.getURL()); } -void ZoneEntityRenderer::updateKeyZoneItemFromEntity(const TypedEntityPointer& entity) { +void ZoneEntityRenderer::updateKeyZoneItemFromEntity() { /* TODO: Implement the sun model behavior / Keep this code here for reference, this is how we { // Set the stage diff --git a/libraries/entities-renderer/src/RenderableZoneEntityItem.h b/libraries/entities-renderer/src/RenderableZoneEntityItem.h index babd35c0d6..80fe393f48 100644 --- a/libraries/entities-renderer/src/RenderableZoneEntityItem.h +++ b/libraries/entities-renderer/src/RenderableZoneEntityItem.h @@ -41,9 +41,9 @@ protected: virtual void doRenderUpdateAsynchronousTyped(const TypedEntityPointer& entity) override; private: - void updateKeyZoneItemFromEntity(const TypedEntityPointer& entity); - void updateKeySunFromEntity(const TypedEntityPointer& entity); - void updateKeyAmbientFromEntity(const TypedEntityPointer& entity); + void updateKeyZoneItemFromEntity(); + void updateKeySunFromEntity(); + void updateKeyAmbientFromEntity(); void updateKeyBackgroundFromEntity(const TypedEntityPointer& entity); void updateAmbientMap(); void updateSkyboxMap(); @@ -89,6 +89,10 @@ private: bool _needAmbientUpdate{ true }; bool _needBackgroundUpdate{ true }; + KeyLightPropertyGroup _keyLightProperties; + StagePropertyGroup _stageProperties; + SkyboxPropertyGroup _skyboxProperties; + // More attributes used for rendering: QString _ambientTextureURL; NetworkTexturePointer _ambientTexture; diff --git a/libraries/entities/src/ZoneEntityItem.cpp b/libraries/entities/src/ZoneEntityItem.cpp index 1b220565cd..88e4f3c9e6 100644 --- a/libraries/entities/src/ZoneEntityItem.cpp +++ b/libraries/entities/src/ZoneEntityItem.cpp @@ -49,8 +49,10 @@ ZoneEntityItem::ZoneEntityItem(const EntityItemID& entityItemID) : EntityItem(en EntityItemProperties ZoneEntityItem::getProperties(EntityPropertyFlags desiredProperties) const { EntityItemProperties properties = EntityItem::getProperties(desiredProperties); // get the properties from our base class - - _keyLightProperties.getProperties(properties); + // Contains a QString property, must be synchronized + withReadLock([&] { + _keyLightProperties.getProperties(properties); + }); _stageProperties.getProperties(properties); @@ -58,7 +60,10 @@ EntityItemProperties ZoneEntityItem::getProperties(EntityPropertyFlags desiredPr COPY_ENTITY_PROPERTY_TO_PROPERTIES(compoundShapeURL, getCompoundShapeURL); COPY_ENTITY_PROPERTY_TO_PROPERTIES(backgroundMode, getBackgroundMode); - _skyboxProperties.getProperties(properties); + // Contains a QString property, must be synchronized + withReadLock([&] { + _skyboxProperties.getProperties(properties); + }); COPY_ENTITY_PROPERTY_TO_PROPERTIES(flyingAllowed, getFlyingAllowed); COPY_ENTITY_PROPERTY_TO_PROPERTIES(ghostingAllowed, getGhostingAllowed); @@ -88,8 +93,10 @@ bool ZoneEntityItem::setProperties(const EntityItemProperties& properties) { bool ZoneEntityItem::setSubClassProperties(const EntityItemProperties& properties) { bool somethingChanged = EntityItem::setSubClassProperties(properties); // set the properties in our base class - - _keyLightPropertiesChanged = _keyLightProperties.setProperties(properties); + // Contains a QString property, must be synchronized + withWriteLock([&] { + _keyLightPropertiesChanged = _keyLightProperties.setProperties(properties); + }); _stagePropertiesChanged = _stageProperties.setProperties(properties); @@ -101,11 +108,13 @@ bool ZoneEntityItem::setSubClassProperties(const EntityItemProperties& propertie SET_ENTITY_PROPERTY_FROM_PROPERTIES(ghostingAllowed, setGhostingAllowed); SET_ENTITY_PROPERTY_FROM_PROPERTIES(filterURL, setFilterURL); - _skyboxPropertiesChanged = _skyboxProperties.setProperties(properties); + // Contains a QString property, must be synchronized + withWriteLock([&] { + _skyboxPropertiesChanged = _skyboxProperties.setProperties(properties); + }); somethingChanged = somethingChanged || _keyLightPropertiesChanged || _stagePropertiesChanged || _skyboxPropertiesChanged; - return somethingChanged; } @@ -116,8 +125,12 @@ int ZoneEntityItem::readEntitySubclassDataFromBuffer(const unsigned char* data, int bytesRead = 0; const unsigned char* dataAt = data; - int bytesFromKeylight = _keyLightProperties.readEntitySubclassDataFromBuffer(dataAt, (bytesLeftToRead - bytesRead), args, - propertyFlags, overwriteLocalData, _keyLightPropertiesChanged); + int bytesFromKeylight; + withWriteLock([&] { + bytesFromKeylight = _keyLightProperties.readEntitySubclassDataFromBuffer(dataAt, (bytesLeftToRead - bytesRead), args, + propertyFlags, overwriteLocalData, _keyLightPropertiesChanged); + }); + somethingChanged = somethingChanged || _keyLightPropertiesChanged; bytesRead += bytesFromKeylight; dataAt += bytesFromKeylight; @@ -132,8 +145,11 @@ int ZoneEntityItem::readEntitySubclassDataFromBuffer(const unsigned char* data, READ_ENTITY_PROPERTY(PROP_COMPOUND_SHAPE_URL, QString, setCompoundShapeURL); READ_ENTITY_PROPERTY(PROP_BACKGROUND_MODE, BackgroundMode, setBackgroundMode); - int bytesFromSkybox = _skyboxProperties.readEntitySubclassDataFromBuffer(dataAt, (bytesLeftToRead - bytesRead), args, - propertyFlags, overwriteLocalData, _skyboxPropertiesChanged); + int bytesFromSkybox; + withWriteLock([&] { + bytesFromSkybox = _skyboxProperties.readEntitySubclassDataFromBuffer(dataAt, (bytesLeftToRead - bytesRead), args, + propertyFlags, overwriteLocalData, _skyboxPropertiesChanged); + }); somethingChanged = somethingChanged || _skyboxPropertiesChanged; bytesRead += bytesFromSkybox; dataAt += bytesFromSkybox; @@ -150,13 +166,18 @@ int ZoneEntityItem::readEntitySubclassDataFromBuffer(const unsigned char* data, EntityPropertyFlags ZoneEntityItem::getEntityProperties(EncodeBitstreamParams& params) const { EntityPropertyFlags requestedProperties = EntityItem::getEntityProperties(params); - requestedProperties += _keyLightProperties.getEntityProperties(params); + withReadLock([&] { + requestedProperties += _keyLightProperties.getEntityProperties(params); + }); requestedProperties += PROP_SHAPE_TYPE; requestedProperties += PROP_COMPOUND_SHAPE_URL; requestedProperties += PROP_BACKGROUND_MODE; requestedProperties += _stageProperties.getEntityProperties(params); - requestedProperties += _skyboxProperties.getEntityProperties(params); + + withReadLock([&] { + requestedProperties += _skyboxProperties.getEntityProperties(params); + }); requestedProperties += PROP_FLYING_ALLOWED; requestedProperties += PROP_GHOSTING_ALLOWED; diff --git a/libraries/entities/src/ZoneEntityItem.h b/libraries/entities/src/ZoneEntityItem.h index 093c2edb64..14e7cd2f40 100644 --- a/libraries/entities/src/ZoneEntityItem.h +++ b/libraries/entities/src/ZoneEntityItem.h @@ -63,12 +63,12 @@ public: QString getCompoundShapeURL() const; virtual void setCompoundShapeURL(const QString& url); - const KeyLightPropertyGroup& getKeyLightProperties() const { return _keyLightProperties; } + KeyLightPropertyGroup getKeyLightProperties() const { return resultWithReadLock([&] { return _keyLightProperties; }); } void setBackgroundMode(BackgroundMode value) { _backgroundMode = value; _backgroundPropertiesChanged = true; } BackgroundMode getBackgroundMode() const { return _backgroundMode; } - const SkyboxPropertyGroup& getSkyboxProperties() const { return _skyboxProperties; } + SkyboxPropertyGroup getSkyboxProperties() const { return resultWithReadLock([&] { return _skyboxProperties; }); } const StagePropertyGroup& getStageProperties() const { return _stageProperties; } bool getFlyingAllowed() const { return _flyingAllowed; }