From d3f344c8d0da2ffce6ac9a37f278deff8311f980 Mon Sep 17 00:00:00 2001 From: codereader Date: Sat, 1 Oct 2022 04:20:26 +0200 Subject: [PATCH] #6107: Implement layer hierarchies in LayerManager --- include/ilayer.h | 5 +++ radiantcore/layers/LayerManager.cpp | 43 ++++++++++++++++--- radiantcore/layers/LayerManager.h | 9 +++- test/LayerManipulation.cpp | 66 ++++++++++++++++++++++++----- 4 files changed, 103 insertions(+), 20 deletions(-) diff --git a/include/ilayer.h b/include/ilayer.h index 5a3a396a11..c767731be2 100644 --- a/include/ilayer.h +++ b/include/ilayer.h @@ -208,6 +208,11 @@ class ILayerManager */ virtual sigc::signal signal_layerVisibilityChanged() = 0; + /** + * Fired whenever a parent of a layer has been changed. + */ + virtual sigc::signal signal_layerHierarchyChanged() = 0; + /** * Public signal to get notified about layer membership changes, * e.g. when a node has been added to a layer, or moved to a new one. diff --git a/radiantcore/layers/LayerManager.cpp b/radiantcore/layers/LayerManager.cpp index bc62f92705..bdbef36171 100644 --- a/radiantcore/layers/LayerManager.cpp +++ b/radiantcore/layers/LayerManager.cpp @@ -21,6 +21,7 @@ namespace { constexpr const char* const DEFAULT_LAYER_NAME = N_("Default"); constexpr int DEFAULT_LAYER = 0; + constexpr int NO_PARENT_ID = -1; } LayerManager::LayerManager() : @@ -51,17 +52,19 @@ int LayerManager::createLayer(const std::string& name, int layerID) // Update the visibility cache, so get the highest ID int highestID = getHighestLayerID(); - // Make sure the vector has allocated enough memory + // Make sure the vectors are large enough _layerVisibility.resize(highestID+1); + _layerParentIds.resize(highestID+1); // Set the newly created layer to "visible" - _layerVisibility[result.first->first] = true; + _layerVisibility[layerID] = true; + _layerParentIds[layerID] = NO_PARENT_ID; // Layers have changed onLayersChanged(); // Return the ID of the inserted layer - return result.first->first; + return layerID; } int LayerManager::createLayer(const std::string& name) @@ -106,8 +109,9 @@ void LayerManager::deleteLayer(const std::string& name) // Remove the layer _layers.erase(layerID); - // Reset the visibility flag to TRUE + // Reset the visibility flag to TRUE, remove parent _layerVisibility[layerID] = true; + _layerParentIds[layerID] = NO_PARENT_ID; if (layerID == _activeLayer) { @@ -141,9 +145,13 @@ void LayerManager::reset() _layerVisibility.resize(1); _layerVisibility[DEFAULT_LAYER] = true; - // Update the dialog + _layerParentIds.resize(1); + _layerParentIds[DEFAULT_LAYER] = NO_PARENT_ID; + + // Emit all changed signals _layersChangedSignal.emit(); _layerVisibilityChangedSignal.emit(); + _layerHierarchyChangedSignal.emit(); } bool LayerManager::renameLayer(int layerID, const std::string& newLayerName) @@ -363,12 +371,28 @@ void LayerManager::setSelected(int layerID, bool selected) int LayerManager::getParentLayer(int layerId) { - return -1; + return _layerParentIds.at(layerId); } void LayerManager::setParentLayer(int childLayerId, int parentLayerId) { - + if (childLayerId == DEFAULT_LAYER) + { + throw std::invalid_argument("Cannot assign a parent to the default layer"); + } + + // Non-existent layer IDs will throw + if (!layerExists(childLayerId) || + parentLayerId != -1 && !layerExists(parentLayerId)) + { + throw std::invalid_argument("Invalid layer ID"); + } + + if (_layerParentIds.at(childLayerId) != parentLayerId) + { + _layerParentIds.at(childLayerId) = parentLayerId; + _layerHierarchyChangedSignal.emit(); + } } sigc::signal LayerManager::signal_layersChanged() @@ -381,6 +405,11 @@ sigc::signal LayerManager::signal_layerVisibilityChanged() return _layerVisibilityChangedSignal; } +sigc::signal LayerManager::signal_layerHierarchyChanged() +{ + return _layerHierarchyChangedSignal; +} + sigc::signal LayerManager::signal_nodeMembershipChanged() { return _nodeMembershipChangedSignal; diff --git a/radiantcore/layers/LayerManager.h b/radiantcore/layers/LayerManager.h index 345fbe9a8c..39516079d6 100644 --- a/radiantcore/layers/LayerManager.h +++ b/radiantcore/layers/LayerManager.h @@ -11,19 +11,23 @@ class LayerManager : public ILayerManager { private: + // The list of named layers, indexed by an integer ID + std::map _layers; + // greebo: An array of booleans reflects the visibility status // of all layers. Indexed by the layer id, it can be used to // quickly check whether a layer is visible or not. std::vector _layerVisibility; - // The list of named layers, indexed by an integer ID - std::map _layers; + // The parent IDs of each layer (-1 for no parent) + std::vector _layerParentIds; // The ID of the active layer int _activeLayer; sigc::signal _layersChangedSignal; sigc::signal _layerVisibilityChangedSignal; + sigc::signal _layerHierarchyChangedSignal; sigc::signal _nodeMembershipChangedSignal; public: @@ -111,6 +115,7 @@ class LayerManager : sigc::signal signal_layersChanged() override; sigc::signal signal_layerVisibilityChanged() override; + sigc::signal signal_layerHierarchyChanged() override; sigc::signal signal_nodeMembershipChanged() override; private: diff --git a/test/LayerManipulation.cpp b/test/LayerManipulation.cpp index 39e8a0eee1..f96fa0ab52 100644 --- a/test/LayerManipulation.cpp +++ b/test/LayerManipulation.cpp @@ -168,20 +168,18 @@ TEST_F(LayerTest, ResetLayerManager) { auto& layerManager = GlobalMapModule().getRoot()->getLayerManager(); - layerManager.createLayer("TestLayer"); - layerManager.createLayer("TestLayer2"); + auto testLayerId = layerManager.createLayer("TestLayer"); + auto testLayer2Id = layerManager.createLayer("TestLayer2"); - std::size_t layersChangedFireCount = 0; - layerManager.signal_layersChanged().connect([&]() - { - ++layersChangedFireCount; - }); + // Test Layer 2 is a child of Test Layer + layerManager.setParentLayer(testLayer2Id, testLayerId); + std::size_t layersChangedFireCount = 0; std::size_t layerVisibilityChangedFireCount = 0; - layerManager.signal_layerVisibilityChanged().connect([&]() - { - ++layerVisibilityChangedFireCount; - }); + std::size_t layerHierarchyChangedFireCount = 0; + layerManager.signal_layersChanged().connect([&]() { ++layersChangedFireCount; }); + layerManager.signal_layerVisibilityChanged().connect([&]() { ++layerVisibilityChangedFireCount; }); + layerManager.signal_layerHierarchyChanged().connect([&]() { ++layerHierarchyChangedFireCount; }); layerManager.reset(); @@ -194,8 +192,14 @@ TEST_F(LayerTest, ResetLayerManager) EXPECT_EQ(layerManager.getActiveLayer(), 0) << "Default layer should be active"; EXPECT_EQ(layerManager.layerIsVisible(0), true) << "Default layer should be visible"; + layerManager.foreachLayer([&](int layerId, const std::string& name) + { + EXPECT_EQ(layerManager.getParentLayer(layerId), -1) << "Parent relationship should have been reset"; + }); + EXPECT_EQ(layersChangedFireCount, 1) << "Layers changed signal should have been fired"; EXPECT_EQ(layerVisibilityChangedFireCount, 1) << "Layer visibility changed signal should have been fired"; + EXPECT_EQ(layerHierarchyChangedFireCount, 1) << "Layer hierarchy changed signal should have been fired"; } TEST_F(LayerTest, RenameLayer) @@ -454,6 +458,46 @@ TEST_F(LayerTest, SetLayerParent) EXPECT_EQ(layerManager.getParentLayer(childLayerId), 0) << "Parent Id has not been reassigned"; } +TEST_F(LayerTest, HierarchyChangedSignal) +{ + auto& layerManager = GlobalMapModule().getRoot()->getLayerManager(); + + auto testLayerId = layerManager.createLayer("TestLayer"); + auto testLayer2Id = layerManager.createLayer("TestLayer2"); + + std::size_t layerVisibilityChangedFireCount = 0; + std::size_t layersChangedFireCount = 0; + std::size_t layerHierarchyChangedFireCount = 0; + layerManager.signal_layersChanged().connect([&]() { ++layersChangedFireCount; }); + layerManager.signal_layerVisibilityChanged().connect([&]() { ++layerVisibilityChangedFireCount; }); + layerManager.signal_layerHierarchyChanged().connect([&]() { ++layerHierarchyChangedFireCount; }); + + // Trigger a hierarchy change + layerManager.setParentLayer(testLayer2Id, testLayerId); + + EXPECT_EQ(layerHierarchyChangedFireCount, 1) << "Hierarchy changed signal should have been fired once"; + EXPECT_EQ(layerVisibilityChangedFireCount, 0) << "Visibility changed signal should not have been fired"; + EXPECT_EQ(layersChangedFireCount, 0) << "Layers changed signal should NOT have been fired at all"; + + // Hiding it again, doesn't trigger any signals + layerHierarchyChangedFireCount = 0; + layerVisibilityChangedFireCount = 0; + layersChangedFireCount = 0; + + layerManager.setParentLayer(testLayer2Id, testLayerId); + + EXPECT_EQ(layerHierarchyChangedFireCount, 0) << "Hierarchy changed signal should not have been fired"; + EXPECT_EQ(layerVisibilityChangedFireCount, 0) << "Visibility changed signal should not have been fired"; + EXPECT_EQ(layersChangedFireCount, 0) << "Layers changed signal should NOT have been fired at all"; + + layerManager.setParentLayer(testLayer2Id, 0); + layerManager.setParentLayer(testLayer2Id, -1); + + EXPECT_EQ(layerHierarchyChangedFireCount, 2) << "Hierarchy changed signal should have been fired twice"; + EXPECT_EQ(layerVisibilityChangedFireCount, 0) << "Visibility changed signal should not have been fired"; + EXPECT_EQ(layersChangedFireCount, 0) << "Layers changed signal should NOT have been fired at all"; +} + void runLayerHierarchyPersistenceTest(const std::string& mapFilePath) { GlobalMapModule().findOrInsertWorldspawn(); // to not save an empty map