geometry: stop stranding copied placement ancestors on removal (#9419) - #9421
Open
BIMvoice wants to merge 1 commit into
Open
geometry: stop stranding copied placement ancestors on removal (#9419)#9421BIMvoice wants to merge 1 commit into
BIMvoice wants to merge 1 commit into
Conversation
…enShell#9419) edit_object_placement severed old_placement.PlacementRelTo before calling remove_deep2, so the removal never saw the ancestor placement chain. For placements copied by project.append_asset (and ifcpatch ExtractElements, which uses it) the copied site/storey/opening chain was left behind as orphan IfcLocalPlacement entities with an empty PlacesObject inverse, violating its SET [1:1] cardinality on IFC2X3. Keeping PlacementRelTo intact lets remove_deep2 traverse the whole chain: ancestors referenced by nothing else are purged with the placement, while ancestors still used elsewhere (a product's ObjectPlacement or a sibling placement's PlacementRelTo is an inverse from outside the subgraph) are kept, along with everything above them. The severing predates remove_deep2 and was only needed for the unsafe remove_deep.
This was referenced Sep 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the second defect of #9419, the orphan
IfcLocalPlacemententities. Follow-up to #9420, which fixed the emptyIfcPresentationLayerAssignment.AssignedItemsand documented this leftover.The bug
project.append_assetcopies a product together with its full placement chain (the ancestors reached throughPlacementRelTo), then rebases the copy by callinggeometry.edit_object_placement. Insrc/ifcopenshell-python/ifcopenshell/api/geometry/edit_object_placement.py(lines 101 to 104 before this change), the removal of the old placement did:Severing
PlacementRelTofirst hides the ancestor chain fromremove_deep2's traversal, so only the immediate placement is purged. The copied ancestors survive with an emptyPlacesObjectinverse, disconnected from every used placement chain, which violatesPlacesObject : SET [1:1]on IFC2X3 (andifcopenshell.validatereports exactly that). This matches the reporter's observation of orphan placements disjoint from the used chains afterExtractElements.The severing predates
remove_deep2: it was necessary for the unsaferemove_deep, which deleted everything forward-reachable unconditionally.remove_deep2checks every subelement for inverses outside the subgraph, so the guard is not needed anymore and only causes the leak.The fix, and the rejected alternative
Chosen: drop the
old_placement.PlacementRelTo = Noneline soremove_deep2sees the whole chain. Its existing reachability rules already do the right thing in both directions: an ancestor referenced by nothing else has all its inverses inside the subgraph and is purged; an ancestor still used elsewhere (a product'sObjectPlacement, or a sibling placement'sPlacementRelTo) has an inverse outside the subgraph, is kept, and nothing above it is even queued. No new reachability logic was added.Rejected: making
append_assetnot copy the placement ancestors it is about to discard. The chain is needed at copy time (its transform feedsget_local_placementfor the rebase), the copy happens inside genericfile.add/file_addmachinery shared with other flows, and the same leak can be produced by any caller handingedit_object_placementa placement with an unreferenced chain. The removal code is the layer whose contract is "purge safely", so it is fixed there.Ordinary placement edits are unaffected: in a normal model every ancestor placement is referenced by its own product, so the chain survives exactly as before. The existing 41
edit_object_placementtests plus a new shared-ancestor regression test cover this.Tests, RED then GREEN
All pure Python, run against the built v0.9.0 core (Linux aarch64 build of an ancestor commit of v0.9.0, with only this PR's
edit_object_placement.pyoverlaid for the GREEN runs):test/api/geometry/test_edit_object_placement.py: a placement with an unreferenced ancestor chain must be fully purged on edit (RED before, 2 failures across IFC4/IFC2X3), and a storey placement shared by two contained walls must survive an edit of one wall, whole subgraph intact (regression guard, passes before and after).test/api/project/test_append_asset.py: appending 2 walls under a site/storey chain leaves noIfcLocalPlacementwith emptyPlacesObject, andifcopenshell.validatereports noPlacesObjectviolation (RED before on IFC2X3 and IFC4).src/ifcpatch/test/test_ExtractElements.py: extracting 2 walls under site/storey (the append_asset: fill IfcPresentationLayerAssignment.AssignedItems across appended assets (#9419) #9420 repro), and extracting only a door placed relative to its opening in a non-extracted wall, must leave no orphan placements and noPlacesObjectviolation (RED before on IFC4 and IFC2X3).Suite totals on the v0.9.0 core, before and after:
Every failure before the fix is one of the new regression tests; no pre-existing test changes behaviour.
One honest caveat from verification: on the 0.8.x wrapper the
ExtractElementscases behave differently, because therefile.adddouble-registers an inverse (the same referent and attribute index counted twice), which makesget_total_inverses(old_placement) == 1fail and skips the removal branch entirely. The built v0.9.0 core does not have that double-registration (verified directly), so the fix takes effect there. The 0.8.x counting quirk is a separate pre-existing issue and is not touched by this PR.🤖 Generated with Claude Code
https://claude.ai/code/session_01E5GFJsNiEk2WfepRgT7miN