Merge pull request #7314 from samcake/color

Fix a potentially bad access to RenderItem case from Overlay

interface/src/ui/overlays/Base3DOverlay.cpp

@@ -13,6 +13,7 @@
 
 #include <RegisteredMetaTypes.h>
 #include <SharedUtil.h>
+#include "Application.h"
 
 
 const float DEFAULT_LINE_WIDTH = 1.0f;
@@ -38,15 +39,17 @@ Base3DOverlay::Base3DOverlay(const Base3DOverlay* base3DOverlay) :
     _drawInFront(base3DOverlay->_drawInFront)
 {
 }
-
 void Base3DOverlay::setProperties(const QVariantMap& properties) {
     Overlay::setProperties(properties);
 
+    bool needRenderItemUpdate = false;
+
     auto drawInFront = properties["drawInFront"];
 
     if (drawInFront.isValid()) {
         bool value = drawInFront.toBool();
         setDrawInFront(value);
+        needRenderItemUpdate = true;
     }
 
     auto position = properties["position"];
@@ -60,16 +63,19 @@ void Base3DOverlay::setProperties(const QVariantMap& properties) {
     }
     if (position.isValid()) {
         setPosition(vec3FromVariant(position));
+        needRenderItemUpdate = true;
     }
 
     if (properties["lineWidth"].isValid()) {
         setLineWidth(properties["lineWidth"].toFloat());
+        needRenderItemUpdate = true;
     }
 
     auto rotation = properties["rotation"];
 
     if (rotation.isValid()) {
         setRotation(quatFromVariant(rotation));
+        needRenderItemUpdate = true;
     }
 
     if (properties["isSolid"].isValid()) {
@@ -100,6 +106,17 @@ void Base3DOverlay::setProperties(const QVariantMap& properties) {
     if (properties["ignoreRayIntersection"].isValid()) {
         setIgnoreRayIntersection(properties["ignoreRayIntersection"].toBool());
     }
+
+    // Communicate changes to the renderItem if needed
+    if (needRenderItemUpdate) {
+        auto itemID = getRenderItemID();
+        if (render::Item::isValidID(itemID)) {
+            render::ScenePointer scene = qApp->getMain3DScene();
+            render::PendingChanges pendingChanges;
+            pendingChanges.updateItem(itemID);
+            scene->enqueuePendingChanges(pendingChanges);
+        }
+    }
 }
 
 QVariant Base3DOverlay::getProperty(const QString& property) {

interface/src/ui/overlays/Overlays.cpp

@@ -233,19 +233,6 @@ bool Overlays::editOverlay(unsigned int id, const QVariant& properties) {
     if (thisOverlay) {
         thisOverlay->setProperties(properties.toMap());
 
-        if (thisOverlay->is3D()) {
-            auto itemID = thisOverlay->getRenderItemID();
-            if (render::Item::isValidID(itemID)) {
-                render::ScenePointer scene = qApp->getMain3DScene();
-                const render::Item& item = scene->getItem(itemID);
-                if (item.getKey() != render::payloadGetKey(thisOverlay)) {
-                    render::PendingChanges pendingChanges;
-                    pendingChanges.updateItem(itemID);
-                    scene->enqueuePendingChanges(pendingChanges);
-                }
-            }
-        }
-
         return true;
     }
     return false;

libraries/render/src/render/Scene.cpp

@@ -48,6 +48,10 @@ ItemID Scene::allocateID() {
     return _IDAllocator.fetch_add(1);
 }
 
+bool Scene::isAllocatedID(const ItemID& id) {
+    return Item::isValidID(id) && (id < _numAllocatedItems.load());
+}
+
 /// Enqueue change batch to the scene
 void Scene::enqueuePendingChanges(const PendingChanges& pendingChanges) {
     _changeQueueMutex.lock();
@@ -79,8 +83,17 @@ void Scene::processPendingChangesQueue() {
         }
         // Now we know for sure that we have enough items in the array to
         // capture anything coming from the pendingChanges
+
+        // resets and potential NEW items
         resetItems(consolidatedPendingChanges._resetItems, consolidatedPendingChanges._resetPayloads);
+
+        // Update the numItemsAtomic counter AFTER the reset changes went through
+        _numAllocatedItems.exchange(maxID);
+
+        // updates
         updateItems(consolidatedPendingChanges._updatedItems, consolidatedPendingChanges._updateFunctors);
+
+        // removes
         removeItems(consolidatedPendingChanges._removedItems);
 
      // ready to go back to rendering activities

libraries/render/src/render/Scene.h

@@ -60,18 +60,24 @@ public:
     // This call is thread safe, can be called from anywhere to allocate a new ID
     ItemID allocateID();
 
+    // Check that the ID is valid and allocated for this scene, this a threadsafe call
+    bool isAllocatedID(const ItemID& id);
+
+    // THis is the total number of allocated items, this a threadsafe call
+    size_t getNumItems() const { return _numAllocatedItems.load(); }
+
     // Enqueue change batch to the scene
     void enqueuePendingChanges(const PendingChanges& pendingChanges);
 
     // Process the penging changes equeued
     void processPendingChangesQueue();
 
+    // This next call are  NOT threadsafe, you have to call them from the correct thread to avoid any potential issues
+
     // Access a particular item form its ID
     // WARNING, There is No check on the validity of the ID, so this could return a bad Item
     const Item& getItem(const ItemID& id) const { return _items[id]; }
 
-    size_t getNumItems() const { return _items.size(); }
-
     // Access the spatialized items
     const ItemSpatialTree& getSpatialTree() const { return _masterSpatialTree; }
 
@@ -81,6 +87,7 @@ public:
 protected:
     // Thread safe elements that can be accessed from anywhere
     std::atomic<unsigned int> _IDAllocator{ 1 }; // first valid itemID will be One
+    std::atomic<unsigned int> _numAllocatedItems{ 1 }; // num of allocated items, matching the _items.size()
     std::mutex _changeQueueMutex;
     PendingChangesQueue _changeQueue;