3768: Fix up face selections to make them consistent with linked groups

- MapDocument::allSelectedBrushFaces(): if multiple groups in a link set
  are selected, only return one representative face per link set so that
  user actions can be performed without generating conflicts.
  (e.g. this allows selecting 2 closed linked groups in a link set
  and applying textures.)

- MapDocumentCommandFacade::
  performSelect(const std::vector<Model::BrushFaceHandle>&):
  "fix up" face selections (if faces in multiple closed groups are
  attempted to be selected, only select one representative face per
  link set). Also implicitly lock/unlock other groups in the link set.

Fixes TrenchBroom#3768

Co-authored-by: Kristian Duske <[email protected]>

common/src/Model/ModelUtils.cpp

@@ -478,5 +478,91 @@ namespace TrenchBroom {
             }
             return result;
         }
+
+        /**
+         * Gets the parent linked groups of `node` (0, 1, or more) by adding them to `dest`. (Doesn't return a vector, to avoid allocations.)
+         */
+        static void getContainingLinkedGroups(Model::Node& node, std::vector<Model::GroupNode*>& result) {
+            Model::GroupNode* groupNode = Model::findContainingLinkedGroup(node);
+            while (groupNode) {
+                result.push_back(groupNode);
+                groupNode = Model::findContainingLinkedGroup(*groupNode);
+            }
+        }
+
+        SelectionResult nodeSelectionWithLinkedGroupConstraints(Model::WorldNode& world, const std::vector<Model::Node*>& nodes) {
+            auto groupsToLock = kdl::vector_set<Model::GroupNode*>{};
+            auto groupsToKeepUnlocked = kdl::vector_set<Model::GroupNode*>{};
+
+            // collects subset of `nodes` which pass the constraints
+            auto nodesToSelect = std::vector<Model::Node*>{};
+
+            auto linkedGroupsContainingNode = std::vector<Model::GroupNode*>{};
+            for (Model::Node* node : nodes) {                
+                linkedGroupsContainingNode.clear();
+                getContainingLinkedGroups(*node, linkedGroupsContainingNode);
+
+                const bool isNodeInGroupsToLock = std::any_of(
+                    std::begin(linkedGroupsContainingNode), 
+                    std::end(linkedGroupsContainingNode), 
+                    [&](Model::GroupNode* group) { 
+                        return groupsToLock.count(group) == 1u;
+                    });
+
+                if (isNodeInGroupsToLock) {
+                    // don't bother trying to select this node.
+                    continue;
+                }
+
+                // we will allow selection of `node`, but we need to implicitly lock
+                // any other groups in the link sets of the groups listed in `linkedGroupsContainingNode`.
+
+                // first check if we've already processed all of these
+                const bool areAncestorLinkedGroupsHandled = std::all_of(
+                    std::begin(linkedGroupsContainingNode), 
+                    std::end(linkedGroupsContainingNode), 
+                    [&](Model::GroupNode* group) { 
+                        return groupsToKeepUnlocked.count(group) == 1u;
+                    });
+
+                if (!areAncestorLinkedGroupsHandled) {
+                    // for each `group` in `linkedGroupsContainingNode`,
+                    // implicitly lock other groups in the link set of `group`, but keep `group` itself unlocked.
+                    for (Model::GroupNode* group : linkedGroupsContainingNode) {
+                        // find the others and add them to the lock list
+                        for (Model::GroupNode* otherGroup : Model::findLinkedGroups(world, *group->group().linkedGroupId())) {
+                            if (otherGroup == group) {
+                                continue;
+                            }
+                            groupsToLock.insert(otherGroup);
+                        }
+                        groupsToKeepUnlocked.insert(group);
+                    }
+                }
+
+                nodesToSelect.push_back(node);
+            }
+
+            return { nodesToSelect, groupsToLock.release_data() };
+        }
+
+        FaceSelectionResult faceSelectionWithLinkedGroupConstraints(Model::WorldNode& world, const std::vector<Model::BrushFaceHandle>& faces) {
+            const std::vector<Model::Node*> nodes = kdl::vec_transform(faces, [](auto handle) -> Model::Node* { return handle.node(); });
+            auto constrainedNodes = nodeSelectionWithLinkedGroupConstraints(world, nodes);
+
+            const auto nodesToSelect = kdl::vector_set<Model::Node*>{constrainedNodes.nodesToSelect};
+
+            auto facesToSelect = std::vector<Model::BrushFaceHandle>{};
+            for (const auto& handle : faces) {
+                if (nodesToSelect.count(handle.node()) != 0) {
+                    facesToSelect.push_back(handle);
+                }
+            }
+
+            return {
+                std::move(facesToSelect), 
+                std::move(constrainedNodes.groupsToLock)
+            };
+        }
     }
 }

common/src/Model/ModelUtils.h

@@ -20,6 +20,7 @@
 #pragma once
 
 #include "FloatType.h"
+#include "Model/BrushFaceHandle.h"
 #include "Model/HitType.h"
 #include "Model/Node.h"
 
@@ -83,6 +84,39 @@ namespace TrenchBroom {
 
         std::vector<BrushNode*> filterBrushNodes(const std::vector<Node*>& nodes);
         std::vector<EntityNode*> filterEntityNodes(const std::vector<Node*>& nodes);
+
+        struct SelectionResult {
+            std::vector<Model::Node*> nodesToSelect;
+            std::vector<Model::GroupNode*> groupsToLock;
+        };
+
+        /**
+         * Given a list of `nodes` the user wants to select, returns the subset that we should allow selection of,
+         * as well as a list of linked groups to lock.
+         *
+         * - Attempting to select nodes inside a linked group will propose locking all other groups in that link set.
+         *  This is intended to prevent users from making conflicting commands as well as communicate which
+         *  specific linked group they are modifying.
+         *
+         * - If `nodes` contains members of different groups in the same link set,
+         *  only those in the first group will be allowed to be selected ("first" in the order of `nodes`).
+         *
+         * Note: no changes are made, just the proposed selection and locking is returned.
+         */
+        SelectionResult nodeSelectionWithLinkedGroupConstraints(Model::WorldNode& world, const std::vector<Model::Node*>& nodes);
+
+        struct FaceSelectionResult {
+            std::vector<Model::BrushFaceHandle> facesToSelect;
+            std::vector<Model::GroupNode*> groupsToLock;
+        };
+
+        /**
+         * Given a list of `faces` the user wants to select, returns the subset that we should allow selection of,
+         * as well as a list of linked groups to lock.
+         *
+         * @see nodeSelectionWithLinkedGroupConstraints()
+         */
+        FaceSelectionResult faceSelectionWithLinkedGroupConstraints(Model::WorldNode& world, const std::vector<Model::BrushFaceHandle>& faces);
     }
 }
 

common/src/View/MapDocument.cpp

@@ -884,7 +884,9 @@ namespace TrenchBroom {
         std::vector<Model::BrushFaceHandle> MapDocument::allSelectedBrushFaces() const {
             if (hasSelectedBrushFaces())
                 return selectedBrushFaces();
-            return Model::collectBrushFaces(m_selectedNodes.nodes());
+
+            const auto faces = Model::collectBrushFaces(m_selectedNodes.nodes());
+            return Model::faceSelectionWithLinkedGroupConstraints(*m_world.get(), faces).facesToSelect;
         }
 
         std::vector<Model::BrushFaceHandle> MapDocument::selectedBrushFaces() const {

common/src/View/MapDocument.h

@@ -296,10 +296,40 @@ namespace TrenchBroom {
             bool hasSelectedBrushFaces() const override;
             bool hasAnySelectedBrushFaces() const override;
 
+            /**
+             * For commands that modify entities, this returns all entities that should be acted on, based on the current selection.
+             * 
+             * - selected brushes/patches act on their parent entities
+             * - selected groups implicitly act on any contained entities
+             *
+             * If multiple linked groups are selected, returns entities from all of them, so attempting to perform commands
+             * on all of them will be blocked as a conflict.
+             */
             std::vector<Model::EntityNodeBase*> allSelectedEntityNodes() const override;
+
+            /**
+             * For commands that modify brushes, this returns all brushes that should be acted on, based on the current selection.
+             *
+             * - selected groups implicitly act on any contained brushes
+             *
+             * If multiple linked groups are selected, returns brushes from all of them, so attempting to perform commands
+             * on all of them will be blocked as a conflict.
+             */
             std::vector<Model::BrushNode*> allSelectedBrushNodes() const;
             bool hasAnySelectedBrushNodes() const;
             const Model::NodeCollection& selectedNodes() const override;
+
+            /**
+             * For commands that modify brush faces, this returns all that should be acted on, based on the current selection.
+             * 
+             * - if brush faces are explicitly selected (hasSelectedBrushFaces()), use those
+             * - selected groups implicitly act on any contained brushes
+             * - selected brushes implicitly act on their faces
+             *
+             * Unlike allSelectedBrushNodes()/allSelectedEntityNodes(), if multiple groups in a link set are selected,
+             * only return one representative face per brush, so that user actions can be performed without generating conflicts.
+             * (e.g. this allows selecting 2 closed linked groups in a link set and applying textures.)
+             */
             std::vector<Model::BrushFaceHandle> allSelectedBrushFaces() const override;
             std::vector<Model::BrushFaceHandle> selectedBrushFaces() const override;
 

common/src/View/MapDocumentCommandFacade.cpp

@@ -103,10 +103,17 @@ namespace TrenchBroom {
         void MapDocumentCommandFacade::performSelect(const std::vector<Model::BrushFaceHandle>& faces) {
             selectionWillChangeNotifier();
 
+            const auto constrained = Model::faceSelectionWithLinkedGroupConstraints(*m_world.get(), faces);
+
+            for (Model::GroupNode* node : constrained.groupsToLock) {
+                node->setLockedByOtherSelection(true);
+            }
+            nodeLockingDidChangeNotifier(kdl::vec_element_cast<Model::Node*>(constrained.groupsToLock));
+
             std::vector<Model::BrushFaceHandle> selected;
-            selected.reserve(faces.size());
+            selected.reserve(constrained.facesToSelect.size());
 
-            for (const auto& handle : faces) {
+            for (const auto& handle : constrained.facesToSelect) {
                 Model::BrushNode* node = handle.node();
                 const Model::BrushFace& face = handle.face();
                 if (!face.selected() && m_editorContext->selectable(node, face)) {
@@ -165,6 +172,8 @@ namespace TrenchBroom {
         }
 
         void MapDocumentCommandFacade::performDeselect(const std::vector<Model::BrushFaceHandle>& faces) {
+            const auto implicitlyLockedGroups = kdl::vector_set<Model::GroupNode*>{kdl::vec_filter(Model::findAllLinkedGroups(*m_world.get()), [](const auto* group) { return group->lockedByOtherSelection(); })};
+
             selectionWillChangeNotifier();
 
             std::vector<Model::BrushFaceHandle> deselected;
@@ -185,6 +194,20 @@ namespace TrenchBroom {
             selection.addDeselectedBrushFaces(deselected);
 
             selectionDidChangeNotifier(selection);
+
+            // Selection change is done. Next, update implicit locking of linked groups.
+            // The strategy is to figure out what needs to be locked given m_selectedBrushFaces, and then un-implicitly-lock all other linked groups.
+            const auto groupsToLock = kdl::vector_set<Model::GroupNode*>{Model::faceSelectionWithLinkedGroupConstraints(*m_world.get(), m_selectedBrushFaces).groupsToLock};
+            for (Model::GroupNode* node : groupsToLock) {
+                node->setLockedByOtherSelection(true);
+            }
+            nodeLockingDidChangeNotifier(kdl::vec_element_cast<Model::Node*>(groupsToLock.get_data()));
+
+            const auto groupsToUnlock = kdl::set_difference(implicitlyLockedGroups, groupsToLock);
+            for (Model::GroupNode* node : groupsToUnlock) {
+                node->setLockedByOtherSelection(false);
+            }
+            nodeLockingDidChangeNotifier(kdl::vec_element_cast<Model::Node*>(groupsToUnlock));
         }
 
         void MapDocumentCommandFacade::performDeselectAll() {

common/test/src/View/GroupNodesTest.cpp

@@ -21,7 +21,10 @@
 #include "TestUtils.h"
 
 #include "Model/BrushBuilder.h"
+#include "Model/BrushFace.h"
+#include "Model/BrushFaceHandle.h"
 #include "Model/BrushNode.h"
+#include "Model/ChangeBrushFaceAttributesRequest.h"
 #include "Model/Entity.h"
 #include "Model/EntityNode.h"
 #include "Model/GroupNode.h"
@@ -33,6 +36,8 @@
 
 #include <kdl/result.h>
 
+#include <vecmath/mat_ext.h>
+
 #include <functional>
 #include <set>
 
@@ -596,5 +601,99 @@ namespace TrenchBroom {
 
             CHECK(document->currentGroup() == nullptr);
         }
+
+        // https://github.com/TrenchBroom/TrenchBroom/issues/3768
+        TEST_CASE_METHOD(MapDocumentTest, "GroupNodesTest.operationsOnSeveralGroupsInLinkSet", "[GroupNodesTest]") {
+            auto* brushNode = createBrushNode();
+            document->addNodes({{document->parentForNodes(), {brushNode}}});
+            document->select(brushNode);
+
+            auto* groupNode = document->groupSelection("test");
+            REQUIRE(groupNode != nullptr);
+
+            auto* linkedGroupNode = document->createLinkedDuplicate();
+            REQUIRE(linkedGroupNode != nullptr);
+
+            document->deselectAll();
+
+            SECTION("Face selection locks other groups in link set") {
+                CHECK(!linkedGroupNode->locked());
+
+                document->select({Model::BrushFaceHandle{brushNode, 0}});
+                CHECK(linkedGroupNode->locked());
+
+                document->deselectAll();
+                CHECK(!linkedGroupNode->locked());
+            }
+
+            SECTION("Can select two linked groups and apply a texture") {
+                document->select(std::vector<Model::Node*>{groupNode, linkedGroupNode});
+
+                auto setTexture = Model::ChangeBrushFaceAttributesRequest{};
+                setTexture.setTextureName("abc");
+                CHECK(document->setFaceAttributes(setTexture));
+
+                // check that the brushes in both linked groups were textured
+                for (auto* g : std::vector<Model::GroupNode*>{groupNode, linkedGroupNode}) {
+                    auto* brush = dynamic_cast<Model::BrushNode*>(g->children().at(0));
+                    REQUIRE(brush != nullptr);
+
+                    auto attrs = brush->brush().face(0).attributes();
+                    CHECK(attrs.textureName() == "abc");
+                }
+            }
+
+            SECTION("Can't snap to grid with both groups selected") {
+                document->select(std::vector<Model::Node*>{groupNode, linkedGroupNode});
+
+                CHECK(document->transformObjects("", vm::translation_matrix(vm::vec3{0.5, 0.5, 0.0})));
+
+                // This could generate conflicts, because what snaps one group could misalign another
+                // group in the link set. So, just reject the change.
+                CHECK(!document->snapVertices(16.0));
+            }
+        }
+
+        // https://github.com/TrenchBroom/TrenchBroom/issues/3768
+        TEST_CASE_METHOD(MapDocumentTest, "GroupNodesTest.operationsOnSeveralGroupsInLinkSetWithPointEntities", "[GroupNodesTest]") {
+            {
+                auto* entityNode = new Model::EntityNode{Model::Entity{}};
+                document->addNodes({{document->parentForNodes(), {entityNode}}});
+                document->select(entityNode);
+            }
+
+            auto* groupNode = document->groupSelection("test");
+            auto* linkedGroupNode1 = document->createLinkedDuplicate();
+            auto* linkedGroupNode2 = document->createLinkedDuplicate();
+
+            REQUIRE(groupNode != nullptr);
+            REQUIRE(linkedGroupNode1 != nullptr);
+            REQUIRE(linkedGroupNode2 != nullptr);
+
+            document->deselectAll();
+
+            SECTION("Attempt to set a property with 2 out of 3 groups selected") {
+                document->select(std::vector<Model::Node*>{groupNode, linkedGroupNode1});
+
+                // Current design is to reject this because it's modifying entities from multiple groups in a link set.
+                // While in this case the change isn't conflicting, some entity changes are,
+                // e.g. unprotecting a property with 2 linked groups selected, where entities have different values
+                // for that protected property.
+                //
+                // Additionally, the use case for editing entity properties with the entire map selected seems unlikely.
+                CHECK(!document->setProperty("key", "value"));
+
+                auto* groupNodeEntity = dynamic_cast<Model::EntityNode*>(groupNode->children().at(0));
+                auto* linkedEntityNode1 = dynamic_cast<Model::EntityNode*>(linkedGroupNode1->children().at(0));
+                auto* linkedEntityNode2 = dynamic_cast<Model::EntityNode*>(linkedGroupNode2->children().at(0));
+                REQUIRE(groupNodeEntity != nullptr);
+                REQUIRE(linkedEntityNode1 != nullptr);
+                REQUIRE(linkedEntityNode2 != nullptr);
+
+                CHECK(!groupNodeEntity->entity().hasProperty("key"));
+                CHECK(!linkedEntityNode1->entity().hasProperty("key"));
+                CHECK(!linkedEntityNode2->entity().hasProperty("key"));
+            }
+        }
     }
 }