#5613: Removing key values can change a value set's status again. Mor…

…e test cases.
codereader · Oct 16, 2021 · f51e537 · f51e537
1 parent 20b508e
commit f51e537
Show file tree

Hide file tree

Showing 2 changed files with 55 additions and 4 deletions.
diff --git a/libs/selection/CollectiveSpawnargs.h b/libs/selection/CollectiveSpawnargs.h
@@ -114,6 +114,8 @@ class CollectiveSpawnargs
 
     void onKeyErase(Entity* entity, const std::string& key, EntityKeyValue& value)
     {
+        const auto& valueString = value.get();
+
         auto kv = _keyValuesByEntity.find(entity);
 
         if (kv != _keyValuesByEntity.end())
@@ -131,17 +133,43 @@ class CollectiveSpawnargs
 
         if (entityList != _entitiesByKey.end())
         {
-            entityList->second.entities.erase(entity);
+            auto& keyValueSet = entityList->second;
 
-            if (entityList->second.entities.empty())
+            keyValueSet.entities.erase(entity);
+
+            if (keyValueSet.entities.empty())
             {
-                _entitiesByKey.erase(entityList);
                 // This was the last occurrence of this key, remove it
+                _entitiesByKey.erase(entityList);
                 _sigKeyRemoved.emit(key);
             }
-            else
+            // If the value was not shared before, this could be the case now
+            else if (!keyValueSet.valueIsEqualOnAllEntities)
             {
+                auto firstEntity = keyValueSet.entities.begin();
+                auto remainingValue = (*firstEntity)->getKeyValue(key);
 
+                // Skip the first entity and check the others for uniqueness
+                // std::all_of will still return true if the range is empty
+                bool valueIsUnique = std::all_of(
+                    ++firstEntity, keyValueSet.entities.end(), [&](Entity* entity)
+                {
+                    return entity->getKeyValue(key) == remainingValue;
+                });
+
+                if (valueIsUnique)
+                {
+                    keyValueSet.valueIsEqualOnAllEntities = true;
+                    _sigKeyValueSetChanged.emit(key, remainingValue);
+                }
+            }
+            // The value was shared on all entities before, but maybe that's no longer the case
+            // it must still be present on *all* involved entities
+            else if (keyValueSet.entities.size() != _keyValuesByEntity.size())
+            {
+                // Size differs, we have more entities in play than we have entities for this key
+                keyValueSet.valueIsEqualOnAllEntities = false;
+                _sigKeyRemoved.emit(key);
             }
         }
     }

diff --git a/test/EntityInspector.cpp b/test/EntityInspector.cpp
@@ -324,6 +324,28 @@ TEST_F(EntityInspectorTest, RemoveOneSharedKeyValue)
     expectNotListed(keyValueStore, "light_center");
 }
 
+TEST_F(EntityInspectorTest, ReAddOneSharedKeyValue)
+{
+    KeyValueStore keyValueStore;
+    GlobalCommandSystem().executeCommand("OpenMap", cmd::Argument("maps/entityinspector.map"));
+
+    auto entity1 = selectEntity("light_torchflame_1");
+    auto entity2 = selectEntity("light_torchflame_2");
+    auto entity3 = selectEntity("light_torchflame_3");
+    Node_getEntity(entity2)->setKeyValue("light_center", "");
+    keyValueStore.rescanSelection();
+
+    // Since entity 2 doesn't have the ligh_center key, it should not be listed
+    expectNotListed(keyValueStore, "light_center");
+
+    auto sharedValue = Node_getEntity(entity2)->getKeyValue("light_center");
+    Node_getEntity(entity2)->setKeyValue("light_center", sharedValue);
+    keyValueStore.rescanSelection();
+
+    // It should be listed again
+    expectUnique(keyValueStore, "light_center", "0 0 0");
+}
+
 TEST_F(EntityInspectorTest, DeselectOneEntity)
 {
     KeyValueStore keyValueStore;
@@ -341,6 +363,7 @@ TEST_F(EntityInspectorTest, DeselectOneEntity)
 
     // De-select entity3, it should make the canBeBlownOut key value unique
     Node_setSelected(entity3, false);
+    keyValueStore.rescanSelection();
 
     // This one is unique now
     expectUnique(keyValueStore, "canBeBlownOut", "0");