Skip to content

geometry: stop stranding copied placement ancestors on removal (#9419) - #9421

Open
BIMvoice wants to merge 1 commit into
IfcOpenShell:v0.9.0from
BIMvoice:fix-9419-orphan-placements
Open

geometry: stop stranding copied placement ancestors on removal (#9419)#9421
BIMvoice wants to merge 1 commit into
IfcOpenShell:v0.9.0from
BIMvoice:fix-9419-orphan-placements

Conversation

@BIMvoice

@BIMvoice BIMvoice commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Fixes the second defect of #9419, the orphan IfcLocalPlacement entities. Follow-up to #9420, which fixed the empty IfcPresentationLayerAssignment.AssignedItems and documented this leftover.

The bug

project.append_asset copies a product together with its full placement chain (the ancestors reached through PlacementRelTo), then rebases the copy by calling geometry.edit_object_placement. In src/ifcopenshell-python/ifcopenshell/api/geometry/edit_object_placement.py (lines 101 to 104 before this change), the removal of the old placement did:

self.settings["product"].ObjectPlacement = None
old_placement.PlacementRelTo = None
ifcopenshell.util.element.remove_deep2(self.file, old_placement)

Severing PlacementRelTo first hides the ancestor chain from remove_deep2's traversal, so only the immediate placement is purged. The copied ancestors survive with an empty PlacesObject inverse, disconnected from every used placement chain, which violates PlacesObject : SET [1:1] on IFC2X3 (and ifcopenshell.validate reports exactly that). This matches the reporter's observation of orphan placements disjoint from the used chains after ExtractElements.

The severing predates remove_deep2: it was necessary for the unsafe remove_deep, which deleted everything forward-reachable unconditionally. remove_deep2 checks 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 = None line so remove_deep2 sees 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's ObjectPlacement, or a sibling placement's PlacementRelTo) has an inverse outside the subgraph, is kept, and nothing above it is even queued. No new reachability logic was added.

Rejected: making append_asset not copy the placement ancestors it is about to discard. The chain is needed at copy time (its transform feeds get_local_placement for the rebase), the copy happens inside generic file.add/file_add machinery shared with other flows, and the same leak can be produced by any caller handing edit_object_placement a 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_placement tests 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.py overlaid 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 no IfcLocalPlacement with empty PlacesObject, and ifcopenshell.validate reports no PlacesObject violation (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 no PlacesObject violation (RED before on IFC4 and IFC2X3).

Suite totals on the v0.9.0 core, before and after:

Suite Before After
test_append_asset.py 2 failed, 77 passed 79 passed
test_edit_object_placement.py 2 failed, 41 passed 43 passed
test_ExtractElements.py 4 failed, 11 passed, 1 skipped 15 passed, 1 skipped

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 ExtractElements cases behave differently, because there file.add double-registers an inverse (the same referent and attribute index counted twice), which makes get_total_inverses(old_placement) == 1 fail 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

…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant