Merge pull request #15523 from SamGondelman/zone

Case 22466, BUGZ-102: Fix zone rendering issues

libraries/entities-renderer/src/EntityTreeRenderer.cpp

@@ -220,6 +220,7 @@ void EntityTreeRenderer::stopDomainAndNonOwnedEntities() {
 void EntityTreeRenderer::clearDomainAndNonOwnedEntities() {
     stopDomainAndNonOwnedEntities();
 
+    auto sessionUUID = getTree()->getMyAvatarSessionUUID();
     std::unordered_map<EntityItemID, EntityRendererPointer> savedEntities;
     // remove all entities from the scene
     auto scene = _viewState->getMain3DScene();
@@ -227,7 +228,7 @@ void EntityTreeRenderer::clearDomainAndNonOwnedEntities() {
         for (const auto& entry :  _entitiesInScene) {
             const auto& renderer = entry.second;
             const EntityItemPointer& entityItem = renderer->getEntity();
-            if (!(entityItem->isLocalEntity() || (entityItem->isAvatarEntity() && entityItem->getOwningAvatarID() == getTree()->getMyAvatarSessionUUID()))) {
+            if (!(entityItem->isLocalEntity() || (entityItem->isAvatarEntity() && entityItem->getOwningAvatarID() == sessionUUID))) {
                 fadeOutRenderable(renderer);
             } else {
                 savedEntities[entry.first] = entry.second;
@@ -238,7 +239,9 @@ void EntityTreeRenderer::clearDomainAndNonOwnedEntities() {
     _renderablesToUpdate = savedEntities;
     _entitiesInScene = savedEntities;
 
-    _layeredZones.clearNonLocalLayeredZones();
+    if (_layeredZones.clearDomainAndNonOwnedZones(sessionUUID)) {
+        applyLayeredZones();
+    }
 
     OctreeProcessor::clearDomainAndNonOwnedEntities();
 }
@@ -271,6 +274,9 @@ void EntityTreeRenderer::clear() {
 
     // reset the zone to the default (while we load the next scene)
     _layeredZones.clear();
+    if (!_shuttingDown) {
+        applyLayeredZones();
+    }
 
     OctreeProcessor::clear();
 }
@@ -363,6 +369,7 @@ void EntityTreeRenderer::addPendingEntities(const render::ScenePointer& scene, r
             for (const auto& processedId : processedIds) {
                 _entitiesToAdd.erase(processedId);
             }
+            forceRecheckEntities();
         }
     }
 }
@@ -537,8 +544,7 @@ void EntityTreeRenderer::handleSpaceUpdate(std::pair<int32_t, glm::vec4> proxyUp
     _spaceUpdates.emplace_back(proxyUpdate.first, proxyUpdate.second);
 }
 
-bool EntityTreeRenderer::findBestZoneAndMaybeContainingEntities(QSet<EntityItemID>& entitiesContainingAvatar) {
+void EntityTreeRenderer::findBestZoneAndMaybeContainingEntities(QSet<EntityItemID>& entitiesContainingAvatar) {
-    bool didUpdate = false;
     float radius = 0.01f; // for now, assume 0.01 meter radius, because we actually check the point inside later
     QVector<QUuid> entityIDs;
 
@@ -550,7 +556,7 @@ bool EntityTreeRenderer::findBestZoneAndMaybeContainingEntities(QSet<EntityItemI
         // FIXME - if EntityTree had a findEntitiesContainingPoint() this could theoretically be a little faster
         entityTree->evalEntitiesInSphere(_avatarPosition, radius, PickFilter(), entityIDs);
 
-        LayeredZones oldLayeredZones(std::move(_layeredZones));
+        LayeredZones oldLayeredZones(_layeredZones);
         _layeredZones.clear();
 
         // create a list of entities that actually contain the avatar's position
@@ -578,8 +584,8 @@ bool EntityTreeRenderer::findBestZoneAndMaybeContainingEntities(QSet<EntityItemI
 
             if (contains) {
                 // if this entity is a zone and visible, add it to our layered zones
-                if (isZone && entity->getVisible() && renderableForEntity(entity)) {
+                if (isZone && entity->getVisible() && renderableIdForEntity(entity) != render::Item::INVALID_ITEM_ID) {
-                    _layeredZones.insert(std::dynamic_pointer_cast<ZoneEntityItem>(entity));
+                    _layeredZones.emplace(std::dynamic_pointer_cast<ZoneEntityItem>(entity));
                 }
 
                 if ((!hasScript && isZone) || scriptHasLoaded) {
@@ -588,24 +594,16 @@ bool EntityTreeRenderer::findBestZoneAndMaybeContainingEntities(QSet<EntityItemI
             }
         }
 
-        // check if our layered zones have changed
+        if (!_layeredZones.equals(oldLayeredZones)) {
-        if ((_layeredZones.empty() && oldLayeredZones.empty()) || (!oldLayeredZones.empty() && _layeredZones.contains(oldLayeredZones))) {
-            return;
-        }
-
             applyLayeredZones();
-
+        }
-        didUpdate = true;
     });
-
-    return didUpdate;
 }
 
-bool EntityTreeRenderer::checkEnterLeaveEntities() {
+void EntityTreeRenderer::checkEnterLeaveEntities() {
     PROFILE_RANGE(simulation_physics, "EnterLeave");
     PerformanceTimer perfTimer("enterLeave");
     auto now = usecTimestampNow();
-    bool didUpdate = false;
 
     if (_tree && !_shuttingDown) {
         glm::vec3 avatarPosition = _viewState->getAvatarPosition();
@@ -623,7 +621,7 @@ bool EntityTreeRenderer::checkEnterLeaveEntities() {
             _forceRecheckEntities = false;
 
             QSet<EntityItemID> entitiesContainingAvatar;
-            didUpdate = findBestZoneAndMaybeContainingEntities(entitiesContainingAvatar);
+            findBestZoneAndMaybeContainingEntities(entitiesContainingAvatar);
 
             // Note: at this point we don't need to worry about the tree being locked, because we only deal with
             // EntityItemIDs from here. The callEntityScriptMethod() method is robust against attempting to call scripts
@@ -649,7 +647,6 @@ bool EntityTreeRenderer::checkEnterLeaveEntities() {
             }
         }
     }
-    return didUpdate;
 }
 
 void EntityTreeRenderer::leaveDomainAndNonOwnedEntities() {
@@ -696,18 +693,12 @@ bool EntityTreeRenderer::applyLayeredZones() {
     // in the expected layered order and update the scene with it
     auto scene = _viewState->getMain3DScene();
     if (scene) {
-        render::Transaction transaction;
         render::ItemIDs list;
-        for (auto& zone : _layeredZones) {
+        _layeredZones.appendRenderIDs(list, this);
-            auto id = renderableIdForEntity(zone.zone);
-            // The zone may not have been rendered yet.
-            if (id != render::Item::INVALID_ITEM_ID) {
-                list.push_back(id);
-            }
-        }
-        render::Selection selection("RankedZones", list);
-        transaction.resetSelection(selection);
 
+        render::Selection selection("RankedZones", list);
+        render::Transaction transaction;
+        transaction.resetSelection(selection);
         scene->enqueueTransaction(transaction);
     } else {
         qCWarning(entitiesrenderer) << "EntityTreeRenderer::applyLayeredZones(), Unexpected null scene, possibly during application shutdown";
@@ -1018,7 +1009,6 @@ void EntityTreeRenderer::deletingEntity(const EntityItemID& entityID) {
 }
 
 void EntityTreeRenderer::addingEntity(const EntityItemID& entityID) {
-    forceRecheckEntities(); // reset our state to force checking our inside/outsideness of entities
     checkAndCallPreload(entityID);
     auto entity = std::static_pointer_cast<EntityTree>(_tree)->findEntityByID(entityID);
     if (entity) {
@@ -1190,107 +1180,98 @@ void EntityTreeRenderer::updateEntityRenderStatus(bool shouldRenderEntities) {
 }
 
 void EntityTreeRenderer::updateZone(const EntityItemID& id) {
-    // Get in the zone!
+    if (auto zone = std::dynamic_pointer_cast<ZoneEntityItem>(getTree()->findEntityByEntityItemID(id))) {
-    auto zone = std::dynamic_pointer_cast<ZoneEntityItem>(getTree()->findEntityByEntityItemID(id));
+        _layeredZones.update(zone, _avatarPosition, this);
-    if (zone && zone->contains(_avatarPosition)) {
+        applyLayeredZones();
-        _layeredZones.update(zone);
     }
 }
 
-EntityTreeRenderer::LayeredZones::LayeredZones(LayeredZones&& other) {
+bool EntityTreeRenderer::LayeredZones::clearDomainAndNonOwnedZones(const QUuid& sessionUUID) {
-    // In a swap:
+    bool zonesChanged = false;
-    // > All iterators and references remain valid. The past-the-end iterator is invalidated.
-    bool isSkyboxLayerValid = (other._skyboxLayer != other.end());
 
-    swap(other);
+    auto it = c.begin();
-    _map.swap(other._map);
+    while (it != c.end()) {
-    _skyboxLayer = other._skyboxLayer;
+        auto zone = it->zone.lock();
-
+        if (!zone || !(zone->isLocalEntity() || (zone->isAvatarEntity() && zone->getOwningAvatarID() == sessionUUID))) {
-    if (!isSkyboxLayerValid) {
+            zonesChanged = true;
-        _skyboxLayer = end();
+            it = c.erase(it);
-    }
-}
-
-void EntityTreeRenderer::LayeredZones::clearNonLocalLayeredZones() {
-    std::set<LayeredZone> localLayeredZones;
-    std::map<QUuid, iterator> newMap;
-
-    for (auto iter = begin(); iter != end(); iter++) {
-        LayeredZone layeredZone = *iter;
-
-        if (layeredZone.zone->isLocalEntity()) {
-            bool success;
-            iterator it;
-            std::tie(it, success) = localLayeredZones.insert(layeredZone);
-
-            if (success) {
-                newMap.emplace(layeredZone.id, it);
-            }
-        }
-    }
-
-    std::set<LayeredZone>::operator=(localLayeredZones);
-    _map = newMap;
-    _skyboxLayer = empty() ? end() : begin();
-}
-
-void EntityTreeRenderer::LayeredZones::clear() {
-    std::set<LayeredZone>::clear();
-    _map.clear();
-    _skyboxLayer = end();
-}
-
-std::pair<EntityTreeRenderer::LayeredZones::iterator, bool> EntityTreeRenderer::LayeredZones::insert(const LayeredZone& layer) {
-    iterator it;
-    bool success;
-    std::tie(it, success) = std::set<LayeredZone>::insert(layer);
-
-    if (success) {
-        _map.emplace(it->id, it);
-    }
-
-    return { it, success };
-}
-
-void EntityTreeRenderer::LayeredZones::update(std::shared_ptr<ZoneEntityItem> zone) {
-    bool isVisible = zone->isVisible();
-
-    if (empty() && isVisible) {
-        // there are no zones: set this one
-        insert(zone);
-        return;
         } else {
-        LayeredZone zoneLayer(zone);
+            it++;
-
-        // find this zone's layer, if it exists
-        iterator layer = end();
-        auto it = _map.find(zoneLayer.id);
-        if (it != _map.end()) {
-            layer = it->second;
-            // if the volume changed, we need to resort the layer (reinsertion)
-            // if the visibility changed, we need to erase the layer
-            if (zoneLayer.volume != layer->volume || !isVisible) {
-                erase(layer);
-                _map.erase(it);
-                layer = end();
         }
     }
 
-        // (re)insert this zone's layer if necessary
+    if (zonesChanged) {
-        if (layer == end() && isVisible) {
+        std::make_heap(c.begin(), c.end(), comp);
-            std::tie(layer, std::ignore) = insert(zoneLayer);
-            _map.emplace(layer->id, layer);
     }
+    return zonesChanged;
+}
+
+std::pair<bool, bool> EntityTreeRenderer::LayeredZones::getZoneInteractionProperties() const {
+    auto it = c.cbegin();
+    while (it != c.cend()) {
+        auto zone = it->zone.lock();
+        if (zone && zone->isDomainEntity()) {
+            return { zone->getFlyingAllowed(), zone->getGhostingAllowed() };
+        }
+        it++;
+    }
+    return { true, true };
+}
+
+void EntityTreeRenderer::LayeredZones::remove(const std::shared_ptr<ZoneEntityItem>& zone) {
+    auto it = c.begin();
+    while (it != c.end()) {
+        if (it->zone.lock() == zone) {
+            break;
+        }
+        it++;
+    }
+    if (it != c.end()) {
+        c.erase(it);
+        std::make_heap(c.begin(), c.end(), comp);
     }
 }
 
-bool EntityTreeRenderer::LayeredZones::contains(const LayeredZones& other) {
+void EntityTreeRenderer::LayeredZones::update(std::shared_ptr<ZoneEntityItem> zone, const glm::vec3& position, EntityTreeRenderer* entityTreeRenderer) {
-    bool result = std::equal(other.begin(), other._skyboxLayer, begin());
+    // When a zone's position or visibility changes, we call this method
-    if (result) {
+    // In order to resort our zones, we first remove the changed zone, and then re-insert it if necessary
-        // if valid, set the _skyboxLayer from the other LayeredZones
+    remove(zone);
-        _skyboxLayer = std::next(begin(), std::distance(other.begin(), other._skyboxLayer));
+
+    // Only call contains if the zone is rendering
+    if (zone->isVisible() && entityTreeRenderer->renderableIdForEntity(zone) != render::Item::INVALID_ITEM_ID && zone->contains(position)) {
+        emplace(zone);
+    }
+}
+
+bool EntityTreeRenderer::LayeredZones::equals(const LayeredZones& other) const {
+    if (size() != other.size()) {
+        return false;
+    }
+
+    auto it = c.cbegin();
+    auto otherIt = other.c.cbegin();
+    while (it != c.cend()) {
+        if (*it != *otherIt) {
+            return false;
+        }
+        it++;
+        otherIt++;
+    }
+
+    return true;
+}
+
+void EntityTreeRenderer::LayeredZones::appendRenderIDs(render::ItemIDs& list, EntityTreeRenderer* entityTreeRenderer) const {
+    auto it = c.cbegin();
+    while (it != c.cend()) {
+        if (it->zone.lock()) {
+            auto id = entityTreeRenderer->renderableIdForEntityId(it->id);
+            if (id != render::Item::INVALID_ITEM_ID) {
+                list.push_back(id);
+            }
+        }
+        it++;
     }
-    return result;
 }
 
 CalculateEntityLoadingPriority EntityTreeRenderer::_calculateEntityLoadingPriorityFunc = [](const EntityItem& item) -> float {
@@ -1298,14 +1279,7 @@ CalculateEntityLoadingPriority EntityTreeRenderer::_calculateEntityLoadingPriori
 };
 
 std::pair<bool, bool> EntityTreeRenderer::getZoneInteractionProperties() {
-    for (auto& zone : _layeredZones) {
+    return _layeredZones.getZoneInteractionProperties();
-        // Only domain entities control flying allowed and ghosting allowed
-        if (zone.zone && zone.zone->isDomainEntity()) {
-            return { zone.zone->getFlyingAllowed(), zone.zone->getGhostingAllowed() };
-        }
-    }
-
-    return { true, true };
 }
 
 bool EntityTreeRenderer::wantsKeyboardFocus(const EntityItemID& id) const {

libraries/entities-renderer/src/EntityTreeRenderer.h

@@ -169,7 +169,7 @@ private:
 
     void resetEntitiesScriptEngine();
 
-    bool findBestZoneAndMaybeContainingEntities(QSet<EntityItemID>& entitiesContainingAvatar);
+    void findBestZoneAndMaybeContainingEntities(QSet<EntityItemID>& entitiesContainingAvatar);
 
     bool applyLayeredZones();
     void stopDomainAndNonOwnedEntities();
@@ -180,7 +180,7 @@ private:
     EntityItemID _currentClickingOnEntityID;
 
     QScriptValueList createEntityArgs(const EntityItemID& entityID);
-    bool checkEnterLeaveEntities();
+    void checkEnterLeaveEntities();
     void leaveDomainAndNonOwnedEntities();
     void leaveAllEntities();
     void forceRecheckEntities();
@@ -210,48 +210,38 @@ private:
 
     class LayeredZone {
     public:
-        LayeredZone(std::shared_ptr<ZoneEntityItem> zone, QUuid id, float volume) : zone(zone), id(id), volume(volume) {}
+        LayeredZone(std::shared_ptr<ZoneEntityItem> zone) : zone(zone), id(zone->getID()), volume(zone->getVolumeEstimate()) {}
-        LayeredZone(std::shared_ptr<ZoneEntityItem> zone) : LayeredZone(zone, zone->getID(), zone->getVolumeEstimate()) {}
 
-        bool operator<(const LayeredZone& r) const { return std::tie(volume, id) < std::tie(r.volume, r.id); }
+        bool operator>(const LayeredZone& r) const { return volume > r.volume; }
-        bool operator==(const LayeredZone& r) const { return id == r.id; }
+        bool operator==(const LayeredZone& r) const { return zone.lock() == r.zone.lock(); }
-        bool operator<=(const LayeredZone& r) const { return (*this < r) || (*this == r); }
+        bool operator!=(const LayeredZone& r) const { return !(*this == r); }
+        bool operator>=(const LayeredZone& r) const { return (*this > r) || (*this == r); }
 
-        std::shared_ptr<ZoneEntityItem> zone;
+        std::weak_ptr<ZoneEntityItem> zone;
         QUuid id;
         float volume;
     };
 
-    class LayeredZones : public std::set<LayeredZone> {
+    class LayeredZones : public std::priority_queue<LayeredZone, std::vector<LayeredZone>, std::greater<LayeredZone>> {
     public:
-        LayeredZones() {};
+        void clear() { *this = LayeredZones(); }
-        LayeredZones(LayeredZones&& other);
+        bool clearDomainAndNonOwnedZones(const QUuid& sessionUUID);
 
-        // avoid accidental misconstruction
+        bool equals(const LayeredZones& other) const;
-        LayeredZones(const LayeredZones&) = delete;
+        void remove(const std::shared_ptr<ZoneEntityItem>& zone);
-        LayeredZones& operator=(const LayeredZones&) = delete;
+        void update(std::shared_ptr<ZoneEntityItem> zone, const glm::vec3& position, EntityTreeRenderer* entityTreeRenderer);
-        LayeredZones& operator=(LayeredZones&&) = delete;
 
-        void clear();
+        void appendRenderIDs(render::ItemIDs& list, EntityTreeRenderer* entityTreeRenderer) const;
-        void clearNonLocalLayeredZones();
+        std::pair<bool, bool> getZoneInteractionProperties() const;
-        std::pair<iterator, bool> insert(const LayeredZone& layer);
-        void update(std::shared_ptr<ZoneEntityItem> zone);
-        bool contains(const LayeredZones& other);
-
-        std::shared_ptr<ZoneEntityItem> getZone() { return empty() ? nullptr : begin()->zone; }
-
-    private:
-        std::map<QUuid, iterator> _map;
-        iterator _skyboxLayer { end() };
     };
 
     LayeredZones _layeredZones;
-    float _avgRenderableUpdateCost { 0.0f };
-
     uint64_t _lastZoneCheck { 0 };
     const uint64_t ZONE_CHECK_INTERVAL = USECS_PER_MSEC * 100; // ~10hz
     const float ZONE_CHECK_DISTANCE = 0.001f;
 
+    float _avgRenderableUpdateCost { 0.0f };
+
     ReadWriteLockable _changedEntitiesGuard;
     std::unordered_set<EntityItemID> _changedEntities;
 

libraries/entities-renderer/src/RenderableZoneEntityItem.cpp

@@ -92,9 +92,7 @@ void ZoneEntityRenderer::doRender(RenderArgs* args) {
     }
 
     { // Sun 
-      // Need an update ?
         if (_needSunUpdate) {
-            // Do we need to allocate the light in the stage ?
             if (LightStage::isIndexInvalid(_sunIndex)) {
                 _sunIndex = _stage->addLight(_sunLight);
             } else {
@@ -107,9 +105,7 @@ void ZoneEntityRenderer::doRender(RenderArgs* args) {
     { // Ambient
         updateAmbientMap();
 
-        // Need an update ?
         if (_needAmbientUpdate) {
-            // Do we need to allocate the light in the stage ?
             if (LightStage::isIndexInvalid(_ambientIndex)) {
                 _ambientIndex = _stage->addLight(_ambientLight);
             } else {
@@ -123,7 +119,7 @@ void ZoneEntityRenderer::doRender(RenderArgs* args) {
         updateSkyboxMap();
 
         if (_needBackgroundUpdate) {
-            if (_skyboxMode == COMPONENT_MODE_ENABLED && BackgroundStage::isIndexInvalid(_backgroundIndex)) {
+            if (BackgroundStage::isIndexInvalid(_backgroundIndex)) {
                 _backgroundIndex = _backgroundStage->addBackground(_background);
             }
             _needBackgroundUpdate = false;
@@ -186,24 +182,19 @@ void ZoneEntityRenderer::doRender(RenderArgs* args) {
     }
 }
 
-void ZoneEntityRenderer::removeFromScene(const ScenePointer& scene, Transaction& transaction) {
-#if 0
-    if (_model) {
-        _model->removeFromScene(scene, transaction);
-    }
-#endif
-    Parent::removeFromScene(scene, transaction);
-}
-
 void ZoneEntityRenderer::doRenderUpdateSynchronousTyped(const ScenePointer& scene, Transaction& transaction, const TypedEntityPointer& entity) {
-    DependencyManager::get<EntityTreeRenderer>()->updateZone(entity->getID());
-
     auto position = entity->getWorldPosition();
     auto rotation = entity->getWorldOrientation();
     auto dimensions = entity->getScaledDimensions();
     bool rotationChanged = rotation != _lastRotation;
     bool transformChanged = rotationChanged || position != _lastPosition || dimensions != _lastDimensions;
 
+    auto visible = entity->getVisible();
+    if (transformChanged || visible != _lastVisible) {
+        _lastVisible = visible;
+        DependencyManager::get<EntityTreeRenderer>()->updateZone(entity->getID());
+    }
+
     auto proceduralUserData = entity->getUserData();
     bool proceduralUserDataChanged = _proceduralUserData != proceduralUserData;
 
@@ -226,25 +217,6 @@ void ZoneEntityRenderer::doRenderUpdateSynchronousTyped(const ScenePointer& scen
         _proceduralUserData = entity->getUserData();
     }
 
-#if 0
-    if (_lastShapeURL != _typedEntity->getCompoundShapeURL()) {
-        _lastShapeURL = _typedEntity->getCompoundShapeURL();
-        _model.reset();
-        _model = std::make_shared<Model>();
-        _model->setIsWireframe(true);
-        _model->init();
-        _model->setURL(_lastShapeURL);
-    }
-
-    if (_model && _model->isActive()) {
-        _model->setScaleToFit(true, _lastDimensions);
-        _model->setSnapModelToRegistrationPoint(true, _entity->getRegistrationPoint());
-        _model->setRotation(_lastRotation);
-        _model->setTranslation(_lastPosition);
-        _model->simulate(0.0f);
-    }
-#endif
-
     updateKeyZoneItemFromEntity(entity);
 
     if (keyLightChanged) {
@@ -296,6 +268,10 @@ ItemKey ZoneEntityRenderer::getKey() {
 }
 
 bool ZoneEntityRenderer::needsRenderUpdateFromTypedEntity(const TypedEntityPointer& entity) const {
+    if (entity->getVisible() != _lastVisible) {
+        return true;
+    }
+
     if (entity->keyLightPropertiesChanged() ||
         entity->ambientLightPropertiesChanged() ||
         entity->hazePropertiesChanged() ||
@@ -323,25 +299,7 @@ bool ZoneEntityRenderer::needsRenderUpdateFromTypedEntity(const TypedEntityPoint
         return true;
     }
 
-#if 0
+    // FIXME: do we need to trigger an update when shapeType changes?  see doRenderUpdateAsynchronousTyped
-    if (_typedEntity->getCompoundShapeURL() != _lastShapeURL) {
-        return true;
-    }
-
-    if (_model) {
-        if (!_model->needsFixupInScene() && (!ZoneEntityItem::getDrawZoneBoundaries() || _entity->getShapeType() != SHAPE_TYPE_COMPOUND)) {
-            return true;
-        }
-
-        if (_model->needsFixupInScene() && (ZoneEntityItem::getDrawZoneBoundaries() || _entity->getShapeType() == SHAPE_TYPE_COMPOUND)) {
-            return true;
-        }
-
-        if (_lastModelActive != _model->isActive()) {
-            return true;
-        }
-    }
-#endif
 
     return false;
 }
@@ -450,7 +408,6 @@ void ZoneEntityRenderer::updateKeyZoneItemFromEntity(const TypedEntityPointer& e
 }
 
 void ZoneEntityRenderer::setAmbientURL(const QString& ambientUrl) {
-    // nothing change if nothing change
     if (_ambientTextureURL == ambientUrl) {
         return;
     }
@@ -466,8 +423,6 @@ void ZoneEntityRenderer::setAmbientURL(const QString& ambientUrl) {
         _pendingAmbientTexture = true;
         auto textureCache = DependencyManager::get<TextureCache>();
         _ambientTexture = textureCache->getTexture(_ambientTextureURL, image::TextureUsage::AMBIENT_TEXTURE);
-
-        // keep whatever is assigned on the ambient map/sphere until texture is loaded
     }
 }
 
@@ -492,7 +447,6 @@ void ZoneEntityRenderer::updateAmbientMap() {
 }
 
 void ZoneEntityRenderer::setSkyboxURL(const QString& skyboxUrl) {
-    // nothing change if nothing change
     if (_skyboxTextureURL == skyboxUrl) {
         return;
     }

libraries/entities-renderer/src/RenderableZoneEntityItem.h

@@ -24,9 +24,6 @@
 #include "RenderableEntityItem.h"
 #include <ComponentMode.h>
 
-#if 0
-#include <Model.h>
-#endif
 namespace render { namespace entities { 
 
 class ZoneEntityRenderer : public TypedEntityRenderer<ZoneEntityItem> {
@@ -40,7 +37,6 @@ protected:
     virtual void onRemoveFromSceneTyped(const TypedEntityPointer& entity) override;
     virtual ItemKey getKey() override;
     virtual void doRender(RenderArgs* args) override;
-    virtual void removeFromScene(const ScenePointer& scene, Transaction& transaction) override;
     virtual bool needsRenderUpdateFromTypedEntity(const TypedEntityPointer& entity) const override;
     virtual void doRenderUpdateSynchronousTyped(const ScenePointer& scene, Transaction& transaction, const TypedEntityPointer& entity) override;
     virtual void doRenderUpdateAsynchronousTyped(const TypedEntityPointer& entity) override;
@@ -76,14 +72,7 @@ private:
     glm::vec3 _lastPosition;
     glm::vec3 _lastDimensions;
     glm::quat _lastRotation;
-
+    bool _lastVisible;
-    // FIXME compount shapes are currently broken
-    // FIXME draw zone boundaries are currently broken (also broken in master)
-#if 0
-    ModelPointer _model;
-    bool _lastModelActive { false };
-    QString _lastShapeURL;
-#endif
 
     LightStagePointer _stage;
     const graphics::LightPointer _sunLight { std::make_shared<graphics::Light>() };
@@ -137,25 +126,4 @@ private:
 
 } } // namespace 
 
-#if 0
-
-class NetworkGeometry;
-class KeyLightPayload;
-
-class RenderableZoneEntityItemMeta;
-
-class RenderableZoneEntityItem : public ZoneEntityItem, public RenderableEntityInterface  {
-public:
-    virtual bool contains(const glm::vec3& point) const override;
-    virtual bool addToScene(const EntityItemPointer& self, const render::ScenePointer& scene, render::Transaction& transaction) override;
-    virtual void removeFromScene(const EntityItemPointer& self, const render::ScenePointer& scene, render::Transaction& transaction) override;
-private:
-    virtual void locationChanged(bool tellPhysics = true, bool tellChildren = true) override { EntityItem::locationChanged(tellPhysics, tellChildren); notifyBoundChanged(); }
-    virtual void dimensionsChanged() override { EntityItem::dimensionsChanged(); notifyBoundChanged(); }
-    void notifyBoundChanged();
-    void notifyChangedRenderItem();
-    void sceneUpdateRenderItemFromEntity(render::Transaction& transaction);
-};
-#endif
-
 #endif // hifi_RenderableZoneEntityItem_h

libraries/render-utils/src/ZoneRenderer.cpp

@@ -43,7 +43,9 @@ const Selection::Name ZoneRendererTask::ZONES_SELECTION { "RankedZones" };
 
 void ZoneRendererTask::build(JobModel& task, const Varying& input, Varying& output) {
     // Filter out the sorted list of zones
-    const auto zoneItems = task.addJob<render::SelectSortItems>("FilterZones", input, ZONES_SELECTION.c_str());
+    // FIXME: the zones in the selection are already sorted, but we're doing another sort here to pick the selected items
+    // out of `input`, which means we're also looping over the inItems an extra time.
+    const auto zoneItems = task.addJob<render::SelectSortItems>("FilterZones", input, ZONES_SELECTION);
 
     // just setup the current zone env
     task.addJob<SetupZones>("SetupZones", zoneItems);

libraries/render/src/render/Scene.cpp

@@ -287,12 +287,7 @@ void Scene::processTransactionFrame(const Transaction& transaction) {
         _numAllocatedItems.exchange(maxID);
     }
 
-    if (transaction.touchTransactions()) {
-        std::unique_lock<std::mutex> lock(_selectionsMutex);
-
-        // resets and potential NEW items
     resetSelections(transaction._resetSelections);
-    }
 
     resetHighlights(transaction._highlightResets);
     removeHighlights(transaction._highlightRemoves);
@@ -581,35 +576,34 @@ void Scene::resetItemTransition(ItemID itemId) {
     }
 }
 
-// This function is thread safe
 Selection Scene::getSelection(const Selection::Name& name) const {
     std::unique_lock<std::mutex> lock(_selectionsMutex);
     auto found = _selections.find(name);
     if (found == _selections.end()) {
         return Selection();
     } else {
-        return (*found).second;
+        return found->second;
     }
 }
 
-// This function is thread safe
 bool Scene::isSelectionEmpty(const Selection::Name& name) const {
     std::unique_lock<std::mutex> lock(_selectionsMutex);
     auto found = _selections.find(name);
     if (found == _selections.end()) {
         return true;
     } else {
-        return (*found).second.isEmpty();
+        return found->second.isEmpty();
     }
 }
 
 void Scene::resetSelections(const Transaction::SelectionResets& transactions) {
+    std::unique_lock<std::mutex> lock(_selectionsMutex);
     for (auto selection : transactions) {
         auto found = _selections.find(selection.getName());
         if (found == _selections.end()) {
-            _selections.insert(SelectionMap::value_type(selection.getName(), selection));
+            _selections[selection.getName()] = selection;
         } else {
-            (*found).second = selection;
+            found->second = selection;
         }
     }
 }

libraries/render/src/render/Scene.h

@@ -78,9 +78,6 @@ public:
     void merge(Transaction&& transaction);
     void clear();
 
-    // Checkers if there is work to do when processing the transaction
-    bool touchTransactions() const { return !_resetSelections.empty(); }
-
 protected:
 
     using Reset = std::tuple<ItemID, PayloadPointer>;

libraries/render/src/render/Selection.h

@@ -45,9 +45,8 @@ namespace render {
         Name _name;
         ItemIDs _items;
     };
-    using Selections = std::vector<Selection>;
 
-    using SelectionMap = std::map<const Selection::Name, Selection>;
+    using SelectionMap = std::unordered_map<Selection::Name, Selection>;
 
 }