From 8cf7057abcb0822c997a55e2255b029d64ee6cfc Mon Sep 17 00:00:00 2001 From: Dion Moult Date: Mon, 13 Jan 2025 15:44:52 +1100 Subject: [PATCH] Fix #5954. Special automatic movement of features shouldn't then also move children of those features. If you move a wall, and that wall has features (e.g. openings), it's desirable to also move those openings (because they are invisible). This is a special exception to `should_transform_children` because the definition of the feature (opening) is inherently tied to the parent (wall). What wasn't considered is that this would typically then also move subchildren of the features (e.g. fills like doors). I'm surprised nobody caught this earlier. I did also consider another approach where if you move a wall, it moves all unfilled openings, and if you move a door which fills a opening, it moves the opening too. Intuitively it sounds nice, but it doesn't work because: - Openings can have multiple fillings. If you move all fillings, they all fight to move the openings. - All logic about children goes one way: a placement may have child placements relative to it. This breaks the convention (if moving a door instead moves its opening) which can make brains explode. - It starts to conflate rules about relative / referenced placements with spatial decomposition. We assume all IFCs are valid and follows the convention of relative placement but we cannot guarantee this. This also leads to brain explosion. --- .../api/geometry/edit_object_placement.py | 10 +++++++++- .../api/geometry/test_edit_object_placement.py | 14 +++++++++++++- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/src/ifcopenshell-python/ifcopenshell/api/geometry/edit_object_placement.py b/src/ifcopenshell-python/ifcopenshell/api/geometry/edit_object_placement.py index e0c58c3dea..3351ed3da4 100644 --- a/src/ifcopenshell-python/ifcopenshell/api/geometry/edit_object_placement.py +++ b/src/ifcopenshell-python/ifcopenshell/api/geometry/edit_object_placement.py @@ -130,7 +130,15 @@ class Usecase: continue elif obj.is_a("IfcFeatureElement"): # Feature elements affect the geometry of their parent, and - # so logically should always move with the parent. + # so logically should always move with the parent. However, + # subchildren shouldn't move. + placement2 = obj.ObjectPlacement + for referenced_placement2 in placement2.ReferencedByPlacements: + matrix2 = ifcopenshell.util.placement.get_local_placement(referenced_placement2) + for obj2 in referenced_placement2.PlacesObject: + results.append( + {"product": obj2, "matrix": matrix2, "is_si": False, "should_transform_children": True} + ) continue results.append({"product": obj, "matrix": matrix, "is_si": False, "should_transform_children": True}) return results diff --git a/src/ifcopenshell-python/test/api/geometry/test_edit_object_placement.py b/src/ifcopenshell-python/test/api/geometry/test_edit_object_placement.py index 85b32435a3..7c6bb46b60 100644 --- a/src/ifcopenshell-python/test/api/geometry/test_edit_object_placement.py +++ b/src/ifcopenshell-python/test/api/geometry/test_edit_object_placement.py @@ -545,11 +545,12 @@ class TestEditObjectPlacement(test.bootstrap.IFC4): with pytest.raises(RuntimeError): self.file.by_id(previous_placement_id) - def test_changing_placements_always_affecting_child_features_as_a_special_case(self): + def test_changing_placements_always_affecting_child_features_but_not_subchildren_as_a_special_case(self): ifcopenshell.api.root.create_entity(self.file, ifc_class="IfcProject") ifcopenshell.api.unit.assign_unit(self.file) element = ifcopenshell.api.root.create_entity(self.file, ifc_class="IfcWall") subelement = ifcopenshell.api.root.create_entity(self.file, ifc_class="IfcOpeningElement") + subsubelement = ifcopenshell.api.root.create_entity(self.file, ifc_class="IfcDoor") matrix = numpy.eye(4) matrix[:3, 3] = (1, 1, 1) @@ -557,16 +558,23 @@ class TestEditObjectPlacement(test.bootstrap.IFC4): submatrix = numpy.eye(4) submatrix[:3, 3] = (1, 2, 3) + subsubmatrix = numpy.eye(4) + subsubmatrix[:3, 3] = (7, 8, 9) + shifted_submatrix = numpy.eye(4) shifted_submatrix[:3, 3] = (1, 3, 5) ifcopenshell.api.void.add_opening(self.file, opening=subelement, element=element) + ifcopenshell.api.void.add_filling(self.file, opening=subelement, element=subsubelement) previous_placement_id = ifcopenshell.api.geometry.edit_object_placement( self.file, product=element, matrix=matrix.copy(), is_si=False ).id() ifcopenshell.api.geometry.edit_object_placement( self.file, product=subelement, matrix=submatrix.copy(), is_si=False ) + ifcopenshell.api.geometry.edit_object_placement( + self.file, product=subsubelement, matrix=subsubmatrix.copy(), is_si=False + ) ifcopenshell.api.geometry.edit_object_placement( self.file, product=element, @@ -578,7 +586,11 @@ class TestEditObjectPlacement(test.bootstrap.IFC4): assert numpy.array_equal( ifcopenshell.util.placement.get_local_placement(subelement.ObjectPlacement), shifted_submatrix ) + assert numpy.array_equal( + ifcopenshell.util.placement.get_local_placement(subsubelement.ObjectPlacement), subsubmatrix + ) assert subelement.ObjectPlacement.PlacementRelTo == element.ObjectPlacement + assert subsubelement.ObjectPlacement.PlacementRelTo == subelement.ObjectPlacement # old placement should be removed to avoid orphaned entities with pytest.raises(RuntimeError): self.file.by_id(previous_placement_id)