From 684ec6432362efc8949dab19e2e457fb4d95c08f Mon Sep 17 00:00:00 2001 From: Ryan Schultz Date: Wed, 15 Jul 2026 16:20:21 -0500 Subject: [PATCH] ifcpatch: fix MergeDuplicateTypes destroying distinct/cross-class types MergeDuplicateTypes grouped IfcTypeObjects by a single attribute value only, which caused two failures: - With should_merge_null=True (the old default), every type with an empty Tag was treated as a duplicate and collapsed into one. On Bonsai-authored or mixed models this silently destroyed distinct, differently-named types (e.g. array-child proxy types), leaving BBIM_Array parents pointing at removed child GUIDs ("Arrays With Missing Children" on reload). - Ignoring the IFC class let unrelated types merge, raising e.g. "IfcTypeProduct cannot type IfcBeam" from assign_type. Include the IFC class in the merge key so only same-class types merge, and default should_merge_null to False so untagged types are kept separate unless explicitly opted in via ["Tag", True]. Genuine Revit duplicates always carry a populated Tag, so the target workflow is unaffected. Adds regression tests: distinct null-attribute types preserved by default, and cross-class types never merged. See https://github.com/IfcOpenShell/IfcOpenShell/issues/8609 Co-Authored-By: Claude Opus 4.8 --- .../ifcpatch/recipes/MergeDuplicateTypes.py | 29 +++++++++++++++---- src/ifcpatch/test/test_MergeDuplicateTypes.py | 29 +++++++++++++++++++ 2 files changed, 53 insertions(+), 5 deletions(-) diff --git a/src/ifcpatch/ifcpatch/recipes/MergeDuplicateTypes.py b/src/ifcpatch/ifcpatch/recipes/MergeDuplicateTypes.py index 8da51a4ac8..8370eb8a15 100644 --- a/src/ifcpatch/ifcpatch/recipes/MergeDuplicateTypes.py +++ b/src/ifcpatch/ifcpatch/recipes/MergeDuplicateTypes.py @@ -25,7 +25,7 @@ import ifcopenshell.util.element class Patcher: - def __init__(self, file: ifcopenshell.file, logger: Logger, attribute: str = "Tag", should_merge_null: bool = True): + def __init__(self, file: ifcopenshell.file, logger: Logger, attribute: str = "Tag", should_merge_null: bool = False): """Merge duplicate element types via the Tag or another attribute Revit is notorious for creating many duplicate element types. Element @@ -50,8 +50,18 @@ class Patcher: :param attribute: The name of the attribute to merge element types based on. Typically this will be "Tag" as it stores the unique ID from the proprietary BIM software. - :param should_merge_null: If True, all elements with an empty attribute - will be merged. If False, they will be kept separate. + Only types of the same IFC class are ever merged, so e.g. an + annotation type and a beam type that happen to share an attribute value + are never combined. + + :param should_merge_null: If True, all types with an empty attribute + will be merged together. This defaults to False because an empty + attribute is an absence of evidence, not proof of duplication: + merging every untagged type into one silently destroys distinct + types (a common failure on Bonsai-authored or mixed models, where + genuine, differently-named types may all lack a Tag). Genuine Revit + duplicates always carry a populated Tag, so the default only affects + types the recipe has no reason to believe are duplicates. Example: @@ -62,6 +72,9 @@ class Patcher: # Explicitly say we want to merge based on the Name attribute ifcpatch.execute({"file": model, "recipe": "MergeDuplicateTypes", "arguments": ["Name"]}) + + # Also merge all types that have an empty Tag into a single type + ifcpatch.execute({"file": model, "recipe": "MergeDuplicateTypes", "arguments": ["Tag", True]}) """ self.file = file self.logger = logger @@ -71,9 +84,15 @@ class Patcher: def patch(self): keys: dict[Any, ifcopenshell.entity_instance] = {} for element_type in self.file.by_type("IfcTypeObject"): - key = getattr(element_type, self.attribute) - if not key and not self.should_merge_null: + attribute_value = getattr(element_type, self.attribute) + if not attribute_value and not self.should_merge_null: continue + # Include the IFC class in the key so that only types of the same + # class are ever merged. Two types of different classes are not + # duplicates, and merging them would reassign occurrences to an + # incompatible type (e.g. an IfcTypeProduct used for annotations + # cannot type an IfcBeam), raising a TypeError in assign_type. + key = (element_type.is_a(), attribute_value) original_type = keys.get(key, None) if original_type: elements = ifcopenshell.util.element.get_types(element_type) diff --git a/src/ifcpatch/test/test_MergeDuplicateTypes.py b/src/ifcpatch/test/test_MergeDuplicateTypes.py index 565a62517b..9a9e35e0a6 100644 --- a/src/ifcpatch/test/test_MergeDuplicateTypes.py +++ b/src/ifcpatch/test/test_MergeDuplicateTypes.py @@ -72,6 +72,35 @@ class TestMergeDuplicateTypes(test.bootstrap.IFC4): assert ifcopenshell.util.element.get_material(wall1, should_inherit=False) == None assert ifcopenshell.util.element.get_material(wall2, should_inherit=False) == None + def test_not_merging_different_classes(self): + # Types of different classes are not duplicates even if they share the + # merge attribute (here an empty Tag). Merging them would try to + # reassign occurrences to an incompatible type. See the IfcTypeProduct + # (annotation) vs IfcBeam case. + annotation_type = ifcopenshell.api.root.create_entity(self.file, ifc_class="IfcTypeProduct") + annotation = ifcopenshell.api.root.create_entity(self.file, ifc_class="IfcAnnotation") + ifcopenshell.api.type.assign_type( + self.file, related_objects=[annotation], relating_type=annotation_type, should_map_representations=False + ) + beam_type = ifcopenshell.api.root.create_entity(self.file, ifc_class="IfcBeamType") + beam = ifcopenshell.api.root.create_entity(self.file, ifc_class="IfcBeam") + ifcopenshell.api.type.assign_type( + self.file, related_objects=[beam], relating_type=beam_type, should_map_representations=False + ) + output = ifcpatch.execute({"file": self.file, "recipe": "MergeDuplicateTypes", "arguments": ["Tag", True]}) + assert len(output.by_type("IfcTypeProduct")) == 2 + assert output.by_type("IfcBeamType") + + def test_empty_attributes_kept_separate_by_default(self): + # An empty attribute is an absence of evidence, not proof of + # duplication. By default (should_merge_null=False) distinct types that + # merely lack a Tag must be preserved, otherwise e.g. differently-named + # Bonsai-authored types get silently collapsed into one. + for name in ("Wire Ladder", "CMU Core", "Mortar Joint"): + ifcopenshell.api.root.create_entity(self.file, ifc_class="IfcBuildingElementProxyType", name=name) + output = ifcpatch.execute({"file": self.file, "recipe": "MergeDuplicateTypes", "arguments": []}) + assert len(output.by_type("IfcBuildingElementProxyType")) == 3 + def test_not_merging_empty_attributes(self): wall_type1 = ifcopenshell.api.root.create_entity(self.file, ifc_class="IfcWallType", name="") wall_type2 = ifcopenshell.api.root.create_entity(self.file, ifc_class="IfcWallType", name="")