ifcdiff: compare container and aggregate by GlobalId, not by instance - #9312
Conversation
Sorry, something went wrong.
The container and aggregate relationship checks compared the entity instances returned by get_container and get_aggregate. Those instances belong to two different ifcopenshell.file objects, and instance equality is identity within a single file, so the comparison was always unequal. Every element that had a container or an aggregate was reported changed, and a genuine move was indistinguishable from the noise. Compare GlobalId instead, falling back to None so an added or removed container is still reported. This matches what the type check in the same method already does. Diffing a model against an identical copy of itself with relationships=["container", "aggregate"] goes from 8 of 8 elements reported changed to 0. Moving one wall to another storey still reports container_changed for exactly that wall. Regression tests added in src/ifcdiff/test.py: 2 failed and 7 passed before the change, 9 passed after. Part of IfcOpenShell#4110.
|
Nice catch, and the right shape of fix. I had reached the same conclusion independently in #9296, with the same comparison by GlobalId and the same None handling, so this is a genuine confirmation rather than a competing opinion. Yours is the better PR to carry it: it is narrower, it is not a draft, and the regression tests are focused on exactly this behaviour. So I have dropped the container and aggregate fix from #9296 and left those code paths byte identical to v0.8.0, and confirmed with git merge-tree that the two branches now merge cleanly in either order. Nothing here needs to wait on anything of mine. For anyone reviewing both: #9296 now covers six other ifcdiff defects that this one does not touch, mostly changes that go entirely undetected rather than falsely detected. The two are complementary. |
Sorry, something went wrong.
|
Appreciated, and thanks for checking the merge order both ways. Agreed the two are complementary; the under-reporting fixes in #9296 look worth having. I'll keep this one rebased if v0.8.0 moves. |
Sorry, something went wrong.
Running IfcDiff with relationships=["container", "aggregate"] on a model and an identical copy of it reports every element that has a container or an aggregate as changed. On a small IFC4 model with 8 spatial and physical elements, all 8 come back with {"container_changed": True} or {"aggregate_changed": True}, and the register is exactly the same as the one you get when a wall genuinely moves to another storey. A real move ends up indistinguishable from no change at all.
The cause is in diff_element_relationships: it compares the entity_instance objects returned by get_container and get_aggregate, but the old and new instances belong to two different ifcopenshell.file objects, and instance equality is identity within a single file, so the comparison is unequal even when nothing moved.
The fix compares the GlobalIds instead, with a None fallback so an added or removed container is still reported. This is what the type branch a few lines above already does. The register value stays True, so the output shape and the shallow-return behaviour are unchanged.
Repro (Project > Site > Building > two storeys, 4 walls contained in Level 1, copied via to_string/from_string):
Three tests added to src/ifcdiff/test.py in the existing setup_project() style: test_unchanged_container_is_not_a_change, test_unchanged_aggregate_is_not_a_change, test_changed_container. pytest -p no:pytest-blender test.py is 2 failed / 7 passed before the change and 9 passed after. black and ruff at line-length 120 are clean.
This is the container/aggregate part of #4110. I'm aware #8294 and #8249 proposed the same fix and were closed by their author in favour of #9296, which is still a draft and bundles this with six other ifcdiff changes, including turning container_changed into a dict of old and new ids, which changes the output shape for anything reading change_register or the exported JSON (Bonsai's diff panel included). This PR is deliberately just the two-line behaviour fix with no shape change. If #9296 lands first this becomes redundant and I'll close it.