From fca258fb0761ade0ebd1038cc3097ee30e0f2b37 Mon Sep 17 00:00:00 2001 From: Dion Moult Date: Sat, 21 Mar 2026 18:21:41 +1100 Subject: [PATCH] Fix add_boolean removing second operands from unrelated representations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit add_boolean was removing second operands from ALL IfcShapeRepresentations that referenced them, which could corrupt unrelated shapes and leave representations with empty Items (bug #7803). The API no longer modifies Items — callers manage this explicitly. validate_type and Bonsai's AddBoolean operator now handle their own item removal scoped to the correct representation. Co-Authored-By: Claude Opus 4.6 --- src/bonsai/bonsai/bim/module/model/opening.py | 10 +++++ .../ifcopenshell/api/geometry/add_boolean.py | 14 ------ .../api/geometry/validate_type.py | 1 + .../test/api/geometry/test_add_boolean.py | 44 ++++++++++--------- .../test/api/geometry/test_validate_type.py | 6 +-- 5 files changed, 38 insertions(+), 37 deletions(-) diff --git a/src/bonsai/bonsai/bim/module/model/opening.py b/src/bonsai/bonsai/bim/module/model/opening.py index e680ceeb54..1c157afe4e 100644 --- a/src/bonsai/bonsai/bim/module/model/opening.py +++ b/src/bonsai/bonsai/bim/module/model/opening.py @@ -540,6 +540,16 @@ class AddBoolean(Operator, tool.Ifc.Operator): booleans = ifcopenshell.api.geometry.add_boolean(tool.Ifc.get(), first_item, second_items, props.operator) rep_obj = tool.Geometry.get_geometry_props().representation_obj + if booleans: + # Users typically select two top-level items and expect the + # operand to be absorbed into the boolean, not remain as a + # standalone item alongside it. + representation = tool.Geometry.get_active_representation(rep_obj) + representation = ifcopenshell.util.representation.resolve_representation(representation) + second_items_set = set(second_items) + new_items = [i for i in representation.Items if i not in second_items_set] + if new_items: + representation.Items = new_items rep_element = tool.Ifc.get_entity(rep_obj) tool.Model.mark_manual_booleans(rep_element, booleans) tool.Geometry.reload_representation(rep_obj) diff --git a/src/ifcopenshell-python/ifcopenshell/api/geometry/add_boolean.py b/src/ifcopenshell-python/ifcopenshell/api/geometry/add_boolean.py index 910df42d8c..80ebf5bb89 100644 --- a/src/ifcopenshell-python/ifcopenshell/api/geometry/add_boolean.py +++ b/src/ifcopenshell-python/ifcopenshell/api/geometry/add_boolean.py @@ -31,17 +31,6 @@ def add_boolean( ) -> list[ifcopenshell.entity_instance]: """Adds a boolean operation to two or more representation items - If an IfcBooleanOperand is part of the top level items in an - IfcShapeRepresentation, it will be removed from that level whilst being - added to the IfcBooleanResult. This is because it is generally intuitive - that an item is either participating in a boolean operation, or being an - item in its own right, but not both. - - However, if an IfcBooleanOperand is part of another boolean operation - already, it will not be removed from the existing operation. A new - operation will be created, and therefore it will participate in two - operations. - This function protects against recursive booleans. After a boolean operation is made, since the items of @@ -101,9 +90,6 @@ def add_boolean( booleans = [] for second_item in second_items: - for inverse in file.get_inverse(second_item): - if inverse.is_a("IfcShapeRepresentation"): - inverse.Items = list(set(inverse.Items) - {second_item}) if first.is_a("IfcTesselatedFaceSet"): first.Closed = True # For now, trust the user to do the right thing. if second_item.is_a("IfcTesselatedFaceSet"): diff --git a/src/ifcopenshell-python/ifcopenshell/api/geometry/validate_type.py b/src/ifcopenshell-python/ifcopenshell/api/geometry/validate_type.py index 46bec8757b..3731b2fffc 100644 --- a/src/ifcopenshell-python/ifcopenshell/api/geometry/validate_type.py +++ b/src/ifcopenshell-python/ifcopenshell/api/geometry/validate_type.py @@ -83,6 +83,7 @@ def validate_type( if remaining_items: ifcopenshell.api.geometry.add_boolean(file, preferred_item, remaining_items, "UNION") + representation.Items = [i for i in representation.Items if i not in remaining_items] representation.RepresentationType = ifcopenshell.util.representation.guess_type(representation.Items) if representation.RepresentationType == "CSG": diff --git a/src/ifcopenshell-python/test/api/geometry/test_add_boolean.py b/src/ifcopenshell-python/test/api/geometry/test_add_boolean.py index d507f89308..f750795ee7 100644 --- a/src/ifcopenshell-python/test/api/geometry/test_add_boolean.py +++ b/src/ifcopenshell-python/test/api/geometry/test_add_boolean.py @@ -42,7 +42,7 @@ class TestAddBoolean(test.bootstrap.IFC4): assert boolean.FirstOperand == first assert boolean.SecondOperand == second assert boolean.Operator == "DIFFERENCE" - assert set(rep.Items) == {boolean} + assert set(rep.Items) == {boolean, second} def test_adding_multiple_booleans_from_three_top_level_items(self): ifcopenshell.api.root.create_entity(self.file, ifc_class="IfcProject") @@ -58,13 +58,14 @@ class TestAddBoolean(test.bootstrap.IFC4): booleans = ifcopenshell.api.geometry.add_boolean(self.file, first, [second1, second2]) assert len(booleans) == 2 - assert len(rep.Items) == 1 - assert rep.Items[0].FirstOperand.is_a("IfcBooleanResult") - assert rep.Items[0].SecondOperand == second2 - assert rep.Items[0].Operator == "DIFFERENCE" - assert rep.Items[0].FirstOperand.FirstOperand == first - assert rep.Items[0].FirstOperand.SecondOperand == second1 - assert rep.Items[0].FirstOperand.Operator == "DIFFERENCE" + final_boolean = booleans[-1] + assert final_boolean.FirstOperand.is_a("IfcBooleanResult") + assert final_boolean.SecondOperand == second2 + assert final_boolean.Operator == "DIFFERENCE" + assert final_boolean.FirstOperand.FirstOperand == first + assert final_boolean.FirstOperand.SecondOperand == second1 + assert final_boolean.FirstOperand.Operator == "DIFFERENCE" + assert set(rep.Items) == {final_boolean, second1, second2} def test_adding_a_boolean_to_an_existing_operand_from_a_top_level_item(self): ifcopenshell.api.root.create_entity(self.file, ifc_class="IfcProject") @@ -78,14 +79,16 @@ class TestAddBoolean(test.bootstrap.IFC4): second2 = builder.block() rep = builder.get_representation(body, [first, second1]) booleans = ifcopenshell.api.geometry.add_boolean(self.file, first, [second1]) + # second1 stays in Items, add second2 as well rep.Items = list(rep.Items) + [second2] booleans = ifcopenshell.api.geometry.add_boolean(self.file, first, [second2]) assert len(booleans) == 1 - assert len(rep.Items) == 1 - assert rep.Items[0].FirstOperand.is_a("IfcBooleanResult") - assert rep.Items[0].SecondOperand == second2 - assert rep.Items[0].FirstOperand.FirstOperand == first - assert rep.Items[0].FirstOperand.SecondOperand == second1 + final_boolean = booleans[0] + assert final_boolean.FirstOperand.is_a("IfcBooleanResult") + assert final_boolean.SecondOperand == second2 + assert final_boolean.FirstOperand.FirstOperand == first + assert final_boolean.FirstOperand.SecondOperand == second1 + assert set(rep.Items) == {final_boolean, second1, second2} def test_adding_a_boolean_to_an_existing_operand_from_another_operand(self): ifcopenshell.api.root.create_entity(self.file, ifc_class="IfcProject") @@ -104,7 +107,7 @@ class TestAddBoolean(test.bootstrap.IFC4): booleans = ifcopenshell.api.geometry.add_boolean(self.file, first1, [second2]) assert len(booleans) == 1 - assert len(rep.Items) == 2 + assert len(rep.Items) == 4 assert self.file.get_total_inverses(first1) == 1 result = next(iter(self.file.get_inverse(first1))) @@ -132,14 +135,15 @@ class TestAddBoolean(test.bootstrap.IFC4): rep = builder.get_representation(body, [first, second]) ifcopenshell.api.geometry.add_boolean(self.file, first, [second]) ifcopenshell.api.geometry.add_boolean(self.file, first, [second]) - assert len(rep.Items) == 1 - assert rep.Items[0].FirstOperand == first - assert rep.Items[0].SecondOperand == second + assert set(rep.Items) == {self.file.by_type("IfcBooleanResult")[0], second} + boolean = self.file.by_type("IfcBooleanResult")[0] + assert boolean.FirstOperand == first + assert boolean.SecondOperand == second ifcopenshell.api.geometry.add_boolean(self.file, second, [second]) ifcopenshell.api.geometry.add_boolean(self.file, second, [first]) - assert len(rep.Items) == 1 - assert rep.Items[0].FirstOperand == first - assert rep.Items[0].SecondOperand == second + assert set(rep.Items) == {boolean, second} + assert boolean.FirstOperand == first + assert boolean.SecondOperand == second assert len(self.file.by_type("IfcBooleanResult")) == 1 diff --git a/src/ifcopenshell-python/test/api/geometry/test_validate_type.py b/src/ifcopenshell-python/test/api/geometry/test_validate_type.py index 7f4a57e995..01fa17bfed 100644 --- a/src/ifcopenshell-python/test/api/geometry/test_validate_type.py +++ b/src/ifcopenshell-python/test/api/geometry/test_validate_type.py @@ -76,7 +76,7 @@ class TestValidateType(test.bootstrap.IFC4): booleans = ifcopenshell.api.geometry.add_boolean(self.file, first, [second1]) assert len(booleans) == 1 - assert len(rep.Items) == 3 + assert len(rep.Items) == 4 assert ifcopenshell.api.geometry.validate_type(self.file, rep) is True assert len(rep.Items) == 1 assert rep.RepresentationType == "CSG" @@ -96,9 +96,9 @@ class TestValidateType(test.bootstrap.IFC4): booleans = ifcopenshell.api.geometry.add_boolean(self.file, first, [second1]) assert len(booleans) == 1 - assert len(rep.Items) == 2 + assert len(rep.Items) == 3 # boolean replaced first, but second1 stays in Items assert ifcopenshell.api.geometry.validate_type(self.file, rep) is False - assert len(rep.Items) == 2 + assert len(rep.Items) == 2 # validate_type unioned second1 into the boolean assert rep.RepresentationType is None