#5565: Better material renaming implementation, more thorough unit te…

…sts. Mark new materials as modified right from the start.
codereader · Mar 27, 2021 · b903217 · b903217
1 parent 4e9a4cd
commit b903217
Show file tree

Hide file tree

Showing 5 changed files with 39 additions and 2 deletions.
diff --git a/radiantcore/shaders/CShader.cpp b/radiantcore/shaders/CShader.cpp
@@ -320,6 +320,11 @@ bool CShader::isModified()
     return _template != _originalTemplate;
 }
 
+void CShader::setIsModified()
+{
+    ensureTemplateCopy();
+}
+
 void CShader::revertModifications()
 {
     _template = _originalTemplate;

diff --git a/radiantcore/shaders/CShader.h b/radiantcore/shaders/CShader.h
@@ -145,6 +145,10 @@ class CShader final :
     int getParseFlags() const override;
 
     bool isModified() override;
+
+    // Set this material to modified (this just creates the internal backup copy)
+    void setIsModified();
+
     void revertModifications() override;
     sigc::signal<void>& sig_materialChanged() override;
 

diff --git a/radiantcore/shaders/Doom3ShaderSystem.cpp b/radiantcore/shaders/Doom3ShaderSystem.cpp
@@ -349,13 +349,16 @@ MaterialPtr Doom3ShaderSystem::createEmptyMaterial(const std::string& name)
     // Create a new template/definition
     auto shaderTemplate = std::make_shared<ShaderTemplate>(candidate, "");
 
-    ShaderDefinition def{ shaderTemplate, vfs::FileInfo("", "", vfs::Visibility::HIDDEN)};
+    ShaderDefinition def{ shaderTemplate, vfs::FileInfo("", "", vfs::Visibility::HIDDEN) };
 
     _library->addDefinition(candidate, def);
 
     _sigMaterialCreated.emit(candidate);
 
-    return std::make_shared<CShader>(candidate, def, true);
+    auto material = _library->findShader(candidate);
+    material->setIsModified();
+
+    return material;
 }
 
 bool Doom3ShaderSystem::renameMaterial(const std::string& oldName, const std::string& newName)

diff --git a/radiantcore/shaders/ShaderLibrary.cpp b/radiantcore/shaders/ShaderLibrary.cpp
@@ -84,17 +84,31 @@ void ShaderLibrary::renameDefinition(const std::string& oldName, const std::stri
     assert(definitionExists(oldName));
     assert(!definitionExists(newName));
 
+    // Rename in definition table
     auto extracted = _definitions.extract(oldName);
     extracted.key() = newName;
 
     _definitions.insert(std::move(extracted));
+
+    // Rename in shaders table (if existing)
+    if (_shaders.count(oldName) > 0)
+    {
+        auto extractedShader = _shaders.extract(oldName);
+        extractedShader.key() = newName;
+
+        // Rename the CShader instance
+        extractedShader.mapped()->setName(newName);
+
+        _shaders.insert(std::move(extractedShader));
+    }
 }
 
 void ShaderLibrary::removeDefinition(const std::string& name)
 {
     assert(definitionExists(name));
 
     _definitions.erase(name);
+    _shaders.erase(name);
 }
 
 ShaderDefinition& ShaderLibrary::getEmptyDefinition()

diff --git a/test/Materials.cpp b/test/Materials.cpp
@@ -66,6 +66,7 @@ TEST_F(MaterialsTest, MaterialCreation)
     auto material = materialManager.createEmptyMaterial("textures/test/doesnotexistyet");
     EXPECT_TRUE(material);
     EXPECT_EQ(material->getName(), "textures/test/doesnotexistyet");
+    EXPECT_TRUE(material->isModified()); // new material should be marked as modified
 
     // Check that the signal got emitted
     EXPECT_NE(firedName, "");
@@ -103,6 +104,16 @@ TEST_F(MaterialsTest, MaterialRenaming)
     // Rename the first material
     EXPECT_TRUE(materialManager.renameMaterial("textures/test/firstname", "textures/test/anothername"));
 
+    // The material reference needs to be renamed too
+    EXPECT_EQ(material->getName(), "textures/test/anothername");
+
+    // Renamed material should still be marked as modified
+    EXPECT_TRUE(material->isModified());
+
+    // Re-acquiring the material reference should also deliver the same modified instance
+    material = materialManager.getMaterial("textures/test/anothername");
+    EXPECT_TRUE(material->isModified());
+
     // Check signal emission
     EXPECT_EQ(firedOldName, "textures/test/firstname");
     EXPECT_EQ(firedNewName, "textures/test/anothername");