IfcDiff: compare container and aggregate by GlobalId (#4110) - #8249
IfcDiff: compare container and aggregate by GlobalId (#4110)#8249BIMvoice wants to merge 1 commit into
Conversation
Sorry, something went wrong.
Container and aggregate changes were detected by comparing the result of get_container/get_aggregate between the two files directly. Those are entity instances from different files, so they are never equal, and every element with a container or aggregate was reported as changed (82 false positives on the reporter's model). Compare by GlobalId instead, matching the existing "type" relationship check in the same method. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Closing this in favour of #9296, which is a strict superset. This PR fixed the container and aggregate comparison, which compared entity_instance objects across two different ifcopenshell.file instances and so were never equal. That fix is included verbatim in #9296, on the same lines. While working in the same function I found six further defects of the same kind, all causing a revision diff to silently under-report: class changes never detected, placement never compared, material never compared, the id exclusion regex never matching, attribute detail computed then discarded, is_shallow=False never accumulating, and geometry never queued for models without a MODEL_VIEW subcontext. Keeping both open would mean two PRs conflicting on the same lines for one bug, so folding is cleaner than competing. Nothing is lost by closing this. If the larger PR is unwelcome for any reason, say so and I will reopen this one on its own. |
Sorry, something went wrong.
Part of #4110.
Problem
The container and aggregate relationship checks compared the results of get_container/get_aggregate between the two files directly:
Those are entity instances belonging to two different ifcopenshell.file objects, so != is always true even when the container is unchanged. Every element with a container or aggregate was reported as changed. On the model in #4110 this produced 82 false "changes" for container.
The type check in the same method already does the right thing (compares GlobalId). This makes container and aggregate consistent with it.
Verify
On the #4110 sample models, relationships=["container"] goes from 82 changes to 1, and that 1 is a real change (an element whose container GlobalId actually differs). Reproduced before/after.
Added test_unchanged_container_is_not_reported and test_changed_container_is_reported. Full test.py suite passes (6 tests).
Not included here
The same models also show that a pure class change (IfcWallStandardCase -> IfcController, same GlobalId, same attribute values) is not detected, because diff_element never compares is_a(). That is a separate concern with an output-shape question (new key vs fold into attributes_changed); happy to send a follow-up PR if you want it addressed.
🤖 Generated with Claude Code