#6162: Some advancements to keeping the skin cache up to date after v…

…arious operations
codereader · Nov 16, 2022 · b284b65 · b284b65
1 parent baec14d
commit b284b65
Show file tree

Hide file tree

Showing 3 changed files with 74 additions and 11 deletions.
diff --git a/radiantcore/skins/Doom3SkinCache.cpp b/radiantcore/skins/Doom3SkinCache.cpp
@@ -136,6 +136,9 @@ void Doom3SkinCache::initialiseModule(const IApplicationContext& ctx)
     _declRemovedConnection = GlobalDeclarationManager().signal_DeclRemoved().connect(
         sigc::mem_fun(this, &Doom3SkinCache::onSkinDeclRemoved)
     );
+    _declRenamedConnection = GlobalDeclarationManager().signal_DeclRenamed().connect(
+        sigc::mem_fun(this, &Doom3SkinCache::onSkinDeclRenamed)
+    );
 }
 
 void Doom3SkinCache::shutdownModule()
@@ -148,13 +151,9 @@ void Doom3SkinCache::shutdownModule()
     _allSkins.clear();
 }
 
-void Doom3SkinCache::onSkinDeclCreated(decl::Type type, const std::string& name)
+void Doom3SkinCache::handleSkinAddition(const std::string& name)
 {
-    if (type != decl::Type::Skin) return;
-
     // Add the skin to the cached lists
-    std::lock_guard<std::mutex> lock(_cacheLock);
-
     _allSkins.push_back(name);
 
     auto skin = findSkin(name);
@@ -167,13 +166,9 @@ void Doom3SkinCache::onSkinDeclCreated(decl::Type type, const std::string& name)
     }
 }
 
-void Doom3SkinCache::onSkinDeclRemoved(decl::Type type, const std::string& name)
+void Doom3SkinCache::handleSkinRemoval(const std::string& name)
 {
-    if (type != decl::Type::Skin) return;
-
     // Remove the skin from the cached lists
-    std::lock_guard<std::mutex> lock(_cacheLock);
-
     auto allSkinIt = std::find(_allSkins.begin(), _allSkins.end(), name);
     if (allSkinIt != _allSkins.end())
     {
@@ -190,6 +185,31 @@ void Doom3SkinCache::onSkinDeclRemoved(decl::Type type, const std::string& name)
     }
 }
 
+void Doom3SkinCache::onSkinDeclCreated(decl::Type type, const std::string& name)
+{
+    if (type != decl::Type::Skin) return;
+
+    std::lock_guard<std::mutex> lock(_cacheLock);
+    handleSkinAddition(name);
+}
+
+void Doom3SkinCache::onSkinDeclRemoved(decl::Type type, const std::string& name)
+{
+    if (type != decl::Type::Skin) return;
+
+    std::lock_guard<std::mutex> lock(_cacheLock);
+    handleSkinRemoval(name);
+}
+
+void Doom3SkinCache::onSkinDeclRenamed(decl::Type type, const std::string& oldName, const std::string& newName)
+{
+    if (type != decl::Type::Skin) return;
+
+    std::lock_guard<std::mutex> lock(_cacheLock);
+    handleSkinRemoval(oldName);
+    handleSkinAddition(newName);
+}
+
 void Doom3SkinCache::onSkinDeclsReloaded()
 {
     {

diff --git a/radiantcore/skins/Doom3SkinCache.h b/radiantcore/skins/Doom3SkinCache.h
@@ -2,6 +2,7 @@
 
 #include "Doom3ModelSkin.h"
 
+#include <map>
 #include "imodule.h"
 
 #include "modelskin.h"
@@ -31,8 +32,12 @@ class Doom3SkinCache :
 
 	sigc::signal<void> _sigSkinsReloaded;
     sigc::connection _declsReloadedConnection;
-    sigc::connection _declRemovedConnection;
     sigc::connection _declCreatedConnection;
+    sigc::connection _declRemovedConnection;
+    sigc::connection _declRenamedConnection;
+
+    // This instance monitors all existing skins for changes
+    std::map<std::string, sigc::connection> _declChangedConnections;
 
 public:
     decl::ISkin::Ptr findSkin(const std::string& name) override;
@@ -61,6 +66,10 @@ class Doom3SkinCache :
     void updateModelsInScene();
     void onSkinDeclCreated(decl::Type type, const std::string& name);
     void onSkinDeclRemoved(decl::Type type, const std::string& name);
+    void onSkinDeclRenamed(decl::Type type, const std::string& oldName, const std::string& newName);
+
+    void handleSkinAddition(const std::string& name); // requires held lock
+    void handleSkinRemoval(const std::string& name); // requires held lock
 };
 
 } // namespace
diff --git a/test/Skin.cpp b/test/Skin.cpp
@@ -651,4 +651,38 @@ TEST_F(ModelSkinTest, SkinIsListedAfterSkinChange)
         << "Skin should have reappeared on the matching skin list";
 }
 
+// Renaming a skin affects the internal lists in the global model skin cache
+TEST_F(ModelSkinTest, SkinIsListedAfterSkinRename)
+{
+    constexpr auto skinToRename = "tile_skin2";
+    constexpr auto newName = "some/new/skin";
+
+    auto originalSkin = GlobalModelSkinCache().findSkin(skinToRename);
+    EXPECT_TRUE(originalSkin);
+    auto associatedModel = *originalSkin->getModels().begin();
+
+    // Check prerequisites, skin should be listed
+    auto allSkins = GlobalModelSkinCache().getAllSkins();
+    EXPECT_NE(std::find(allSkins.begin(), allSkins.end(), skinToRename), allSkins.end());
+
+    // Skin should be reported as matching skin for a model
+    auto skinsForModel = GlobalModelSkinCache().getSkinsForModel(associatedModel);
+    EXPECT_NE(std::find(skinsForModel.begin(), skinsForModel.end(), skinToRename), skinsForModel.end());
+
+    GlobalModelSkinCache().renameSkin(skinToRename, newName);
+
+    // The skin should appear with the new name now
+    allSkins = GlobalModelSkinCache().getAllSkins();
+    EXPECT_NE(std::find(allSkins.begin(), allSkins.end(), newName), allSkins.end())
+        << "Skin should be listed with its new name";
+    EXPECT_EQ(std::find(allSkins.begin(), allSkins.end(), skinToRename), allSkins.end())
+        << "Old skin name should be gone now";
+
+    skinsForModel = GlobalModelSkinCache().getSkinsForModel(associatedModel);
+    EXPECT_NE(std::find(skinsForModel.begin(), skinsForModel.end(), newName), skinsForModel.end())
+        << "New skin name should be associated now";
+    EXPECT_EQ(std::find(skinsForModel.begin(), skinsForModel.end(), skinToRename), skinsForModel.end())
+        << "Old skin name should not be associated";
+}
+
 }