feat(geometry): tessellate IfcFaceBasedSurfaceModel - #59
Merged
Merged
Conversation
|
Warning Review limit reachedNext included review available in 48 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Lutherwaves
force-pushed
the
feat/fbsm-dispatch
branch
from
September 24, 2026 11:15
702fa02 to
2cd90a6
Compare
IfcShellBasedSurfaceModel.SbsmBoundary and IfcFaceBasedSurfaceModel.FbsmFaces are different schema entities with the same shape: attribute 0, a SET of things brepMesh already tessellates. Generalise the shell-based helper so the face-based dispatch case can reuse it rather than copy it. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tessellateItemDepth had no case for it, so every element built from one degraded to a bounding box. It is the top entry of the measured gap list at 65 occurrences, and in duplex_a it hides 235 IfcConnectedFaceSets that no other path can reach. FbsmFaces is a SET of IfcConnectedFaceSet, the same shape as the shell-based model's SbsmBoundary, so the case reuses the helper rather than copying it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The "non-face-set member" subcase passed for an incidental reason: brepMesh type-checks nothing, so an IfcCartesianPoint gets as far as its Coordinates list and declines only because that list holds reals rather than references. Rename and document it so it claims what it verifies. The duck-typing gap in brepMesh is real and tracked separately; closing it would move measured numbers and needs its own measurement pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Regenerated by `make parity-report` and `make parity-baseline` after the dispatch case landed. Gate 2 fails in both directions by design, so a closed gap has to be recorded rather than silently improving the numbers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Lutherwaves
force-pushed
the
feat/fbsm-dispatch
branch
from
September 24, 2026 11:17
2cd90a6 to
7dd759c
Compare
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.
tessellateItemDepthhad no case forIfcFaceBasedSurfaceModel, so everyelement built from one degraded to a bounding box. It was the top entry of the
parity harness's measured gap list at 65 occurrences, and in
duplex_ait hid235
IfcConnectedFaceSets that no other path could reach.Measured result
ifcopenhouseduplex_afzk_hausduplex_a's row moves146 Extrude + 0 Brep + 69 OBB + 3 Emptyto146 + 65 + 4 + 3. Exactly 65 elements moved from OBB to Brep,ExtrudeandEmptydid not move at all, and theIFCFACEBASEDSURFACEMODELrow left theunhandled table. The model had zero brep elements before this.
This is far past the ~13% the design doc predicted, so it is worth saying
why. That prediction came from an estimate that face-based surface models
accounted for ~58% of
duplex_a's fallbacks — an estimate that conflated itemOCCURRENCES with fallback ELEMENTS, which is exactly the error
docs/coverage.md's own prose warns against. The measurement is the answer andthe estimate was the guess. The 65-occurrences/65-elements agreement is not a
coincidence either: occurrences are counted once per element that reaches an
item, so for a model with one such item per element the two figures are equal
by construction.
The residual 1.9% is four elements, and the only remaining entry in the
unhandled table is
IFCPOLYGONALBOUNDEDHALFSPACE(11) — issue #52, the next PR.Gate 1 is unchanged
Checked before regenerating anything, which is the order that matters:
make parity-baselinerewrites the file Gate 2 compares against, so running itfirst would turn a failing gate green whether or not the change was correct.
331 elements compared, 329 contained, with exactly the two pre-existing
IfcStairFlightviolations (1oKjKg9PD3fP1iIwXLh3lKmin-y,3KMJUyUe9DfQ2FOCd5ZoiNmax-y) still present and still violating. No newviolation appeared, and
parity/knownviolations.goneeded no change. Thatmatters because this PR replaces conservative superset boxes with tight
meshes — the change most likely to expose an element that passed containment
only because its box was loose.
Shape of the change
FbsmFacesis attribute 0, a SET ofIfcConnectedFaceSet.SbsmBoundaryisattribute 0, a SET of
IfcShell. Different schema entities, identicaltraversal, and
brepMeshalready tessellated a bareIfcConnectedFaceSet. Sothe first commit generalises the existing shell-based helper into
surfaceModelMesh(m, attr)as a pure refactor, and the second adds thedispatch case using it. The attribute is a parameter rather than assumed
because the two constants agreeing at 0 is a fact about the schema, not a rule.
Split that way so a bisect lands on the dispatch case rather than on the rename.
A note for whoever next touches
brepMeshsurfaceModelMeshandbrepMeshdo not type-check set members —brepMeshreads attribute 0 of whatever it is handed. The decline test documents this
honestly rather than claiming more than it checks: an
IfcCartesianPointgetsas far as its
Coordinateslist and declines only because that list holdsreals rather than references. A wrong-typed member whose attribute 0 were a
list of references would still be walked as a shell. Closing that would move
measured numbers, so it needs its own change with its own measurement pass.
🤖 Generated with Claude Code