From 133f73249343e486115c7f01930a09e0971ee3b0 Mon Sep 17 00:00:00 2001 From: Martin Yankovs Date: Thu, 24 Sep 2026 08:39:58 +0300 Subject: [PATCH 1/4] refactor(geometry): share one mesh helper between the surface models 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) --- geometry/brep.go | 22 +++++++++++++++------- geometry/geometry.go | 2 +- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/geometry/brep.go b/geometry/brep.go index 78b83c4..d56675a 100644 --- a/geometry/brep.go +++ b/geometry/brep.go @@ -10,15 +10,23 @@ const ( attrBoundOrientation = 1 // IfcFaceBound.Orientation attrLoopPolygon = 0 // IfcPolyLoop.Polygon attrSbsmBoundary = 0 // IfcShellBasedSurfaceModel.SbsmBoundary + attrFbsmFaces = 0 // IfcFaceBasedSurfaceModel.FbsmFaces ) -// shellBasedSurfaceModelMesh unions the faces of every shell (IfcClosedShell -// or IfcOpenShell) in an IfcShellBasedSurfaceModel's SbsmBoundary set, raw -// units. Used by multi-shell family instances (e.g. a door's frame/leaf/ -// hardware, each its own shell) that mix this representation type with plain -// IfcFacetedBrep siblings. -func shellBasedSurfaceModelMesh(sbsm *step.Instance) (verts []float32, tris []uint32, ok bool) { - boundaryV, has := sbsm.Get(attrSbsmBoundary) +// surfaceModelMesh unions the faces of every shell or face set in a surface +// model's boundary attribute, raw units. +// +// Shared by IfcShellBasedSurfaceModel (SbsmBoundary, a SET of IfcShell) and +// IfcFaceBasedSurfaceModel (FbsmFaces, a SET of IfcConnectedFaceSet). Two +// different schema entities with the same shape: a set of things brepMesh +// already tessellates. attr is a parameter rather than assumed, because the +// two constants agreeing at 0 is a fact about the schema, not a rule. +// +// Used by multi-shell family instances (a door's frame/leaf/hardware, each its +// own shell) that mix these representation types with plain IfcFacetedBrep +// siblings in the same element. +func surfaceModelMesh(m *step.Instance, attr int) (verts []float32, tris []uint32, ok bool) { + boundaryV, has := m.Get(attr) if !has || boundaryV.Kind != step.KindList { return nil, nil, false } diff --git a/geometry/geometry.go b/geometry/geometry.go index a0a83b5..242d7f4 100644 --- a/geometry/geometry.go +++ b/geometry/geometry.go @@ -120,7 +120,7 @@ func tessellateItemDepth(item *step.Instance, unitScale float64, depth int) ([]f // overall reported Source stayed "brep" (since brep still won on the other // sibling items) — a few stray boxed sub-shells shift the whole element's // AABB by a few mm-cm without ever showing up as a Source mismatch. - if v, t, ok := shellBasedSurfaceModelMesh(item); ok { + if v, t, ok := surfaceModelMesh(item, attrSbsmBoundary); ok { return scaleVerts(v, unitScale), t, SourceBrep } case item.IsA("IfcBooleanClippingResult"), item.IsA("IfcBooleanResult"): From 4c5fba6f7706cae24a2e2315450406c01219ebed Mon Sep 17 00:00:00 2001 From: Martin Yankovs Date: Thu, 24 Sep 2026 08:44:07 +0300 Subject: [PATCH 2/4] feat(geometry): tessellate IfcFaceBasedSurfaceModel 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) --- geometry/brep_test.go | 60 +++++++++++++++++++ geometry/geometry.go | 8 +++ .../synthetic/face_based_surface_model.ifc | 43 +++++++++++++ geometry/unhandled.go | 1 + 4 files changed, 112 insertions(+) create mode 100644 geometry/testdata/synthetic/face_based_surface_model.ifc diff --git a/geometry/brep_test.go b/geometry/brep_test.go index f135f87..6c632bb 100644 --- a/geometry/brep_test.go +++ b/geometry/brep_test.go @@ -1,6 +1,7 @@ package geometry import ( + "strings" "testing" "github.com/blox-eng/goifc/model" @@ -33,3 +34,62 @@ func TestBrep_FacetedFaces(t *testing.T) { t.Errorf("brep world min = %v, want {10 20 5}", e.BBoxMin) } } + +// IfcFaceBasedSurfaceModel had no dispatch case at all, so every element built +// from one degraded to a bounding box. It is the top entry of the measured gap +// list (65 occurrences), and in duplex_a it hides 235 IfcConnectedFaceSets no +// other path can reach. +func TestFaceBasedSurfaceModel_Tessellates(t *testing.T) { + f, err := step.ParseFile("testdata/synthetic/face_based_surface_model.ifc") + if err != nil { + t.Fatal(err) + } + r, err := model.Extract(f) + if err != nil { + t.Fatal(err) + } + s, err := Build(f, r) + if err != nil { + t.Fatal(err) + } + e := s.Elements[0] + if e.Source != SourceBrep { + t.Fatalf("source = %q, want brep — the element fell back to a box", e.Source) + } + // Two quad faces -> 2 triangles each -> 12 indices, same as known_box.ifc. + if len(e.Tris) != 12 { + t.Errorf("tris = %d indices, want 12", len(e.Tris)) + } + if !closeVec(e.BBoxMin, [3]float64{10, 20, 5}, 1e-6) { + t.Errorf("world min = %v, want {10 20 5}", e.BBoxMin) + } +} + +// An empty or unusable FbsmFaces set must decline so the element falls back to +// its OBB box. Returning an empty mesh that still claims SourceBrep would +// produce a zero-volume element — the "Collapsed" case the coverage page +// exists to surface, reported as a success. +func TestFaceBasedSurfaceModel_EmptyDeclines(t *testing.T) { + for _, tc := range []struct{ name, faces string }{ + {"empty set", "()"}, + {"non-face-set member", "(#22)"}, // an IfcCartesianPoint + } { + t.Run(tc.name, func(t *testing.T) { + f, err := step.Parse(strings.NewReader( + "ISO-10303-21;\nHEADER;\nFILE_SCHEMA(('IFC4'));\nENDSEC;\nDATA;\n" + + "#22=IFCCARTESIANPOINT((0.,0.,0.));\n" + + "#61=IFCFACEBASEDSURFACEMODEL(" + tc.faces + ");\n" + + "ENDSEC;\nEND-ISO-10303-21;\n")) + if err != nil { + t.Fatal(err) + } + item, ok := f.ByID(61) + if !ok { + t.Fatal("fixture instance #61 missing") + } + if _, _, gotOK := surfaceModelMesh(item, attrFbsmFaces); gotOK { + t.Error("surfaceModelMesh accepted an unusable FbsmFaces set; want ok=false so the caller boxes the element") + } + }) + } +} diff --git a/geometry/geometry.go b/geometry/geometry.go index 242d7f4..ba0138b 100644 --- a/geometry/geometry.go +++ b/geometry/geometry.go @@ -123,6 +123,14 @@ func tessellateItemDepth(item *step.Instance, unitScale float64, depth int) ([]f if v, t, ok := surfaceModelMesh(item, attrSbsmBoundary); ok { return scaleVerts(v, unitScale), t, SourceBrep } + case item.IsA("IfcFaceBasedSurfaceModel"): + // FbsmFaces is a SET of IfcConnectedFaceSet — union their faces, the + // same traversal the shell-based case above does over IfcShell. + // Without this case duplex_a's 235 nested face sets are unreachable, + // and every element built from one becomes a box. + if v, t, ok := surfaceModelMesh(item, attrFbsmFaces); ok { + return scaleVerts(v, unitScale), t, SourceBrep + } case item.IsA("IfcBooleanClippingResult"), item.IsA("IfcBooleanResult"): if v, t, s, ok := clipMeshByDifference(item, unitScale, depth); ok { return v, t, s diff --git a/geometry/testdata/synthetic/face_based_surface_model.ifc b/geometry/testdata/synthetic/face_based_surface_model.ifc new file mode 100644 index 0000000..ad866cb --- /dev/null +++ b/geometry/testdata/synthetic/face_based_surface_model.ifc @@ -0,0 +1,43 @@ +ISO-10303-21; +HEADER; +FILE_DESCRIPTION((''),'2;1'); +FILE_NAME('face_based_surface_model.ifc','',(''),(''),'','',''); +FILE_SCHEMA(('IFC4')); +ENDSEC; +DATA; +/* The same two faces as known_box.ifc, reached through an + IfcFaceBasedSurfaceModel -> IfcConnectedFaceSet pair instead of an + IfcFacetedBrep -> IfcClosedShell pair. duplex_a.ifc nests 235 + IfcConnectedFaceSets behind 40 of these, and before the dispatch case + existed every one of them was unreachable: the element fell back to its + OBB box. */ +#1=IFCPROJECT('0proj',$,'P',$,$,$,$,(#20),#10); +#10=IFCUNITASSIGNMENT((#11)); +#11=IFCSIUNIT(*,.LENGTHUNIT.,$,.METRE.); +#20=IFCGEOMETRICREPRESENTATIONCONTEXT($,'Model',3,1.E-05,#21,$); +#21=IFCAXIS2PLACEMENT3D(#22,$,$); +#22=IFCCARTESIANPOINT((0.,0.,0.)); +#30=IFCCARTESIANPOINT((10.,20.,5.)); +#31=IFCAXIS2PLACEMENT3D(#30,$,$); +#32=IFCLOCALPLACEMENT($,#31); +#40=IFCCARTESIANPOINT((0.,0.,0.)); +#41=IFCCARTESIANPOINT((1.,0.,0.)); +#42=IFCCARTESIANPOINT((1.,1.,0.)); +#43=IFCCARTESIANPOINT((0.,1.,0.)); +#44=IFCCARTESIANPOINT((0.,0.,1.)); +#45=IFCCARTESIANPOINT((1.,0.,1.)); +#46=IFCCARTESIANPOINT((1.,1.,1.)); +#47=IFCCARTESIANPOINT((0.,1.,1.)); +#50=IFCPOLYLOOP((#40,#41,#42,#43)); +#51=IFCFACEOUTERBOUND(#50,.T.); +#52=IFCFACE((#51)); +#53=IFCPOLYLOOP((#44,#45,#46,#47)); +#54=IFCFACEOUTERBOUND(#53,.T.); +#55=IFCFACE((#54)); +#60=IFCCONNECTEDFACESET((#52,#55)); +#61=IFCFACEBASEDSURFACEMODEL((#60)); +#62=IFCSHAPEREPRESENTATION(#20,'Body','SurfaceModel',(#61)); +#63=IFCPRODUCTDEFINITIONSHAPE($,$,(#62)); +#70=IFCBUILDINGELEMENTPROXY('0fbsm',$,'FaceSetBox',$,$,#32,#63,$); +ENDSEC; +END-ISO-10303-21; diff --git a/geometry/unhandled.go b/geometry/unhandled.go index f3dd2c8..4bf0795 100644 --- a/geometry/unhandled.go +++ b/geometry/unhandled.go @@ -18,6 +18,7 @@ var handledItemTypes = []string{ "IfcConnectedFaceSet", "IfcOpenShell", "IfcShellBasedSurfaceModel", + "IfcFaceBasedSurfaceModel", "IfcBooleanClippingResult", "IfcBooleanResult", } From 8bf89cd069cbd4c900b99bf0be620eb9df80843d Mon Sep 17 00:00:00 2001 From: Martin Yankovs Date: Thu, 24 Sep 2026 08:47:54 +0300 Subject: [PATCH 3/4] test(geometry): say what the surface-model decline case actually pins 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) --- geometry/brep_test.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/geometry/brep_test.go b/geometry/brep_test.go index 6c632bb..dd457f9 100644 --- a/geometry/brep_test.go +++ b/geometry/brep_test.go @@ -69,10 +69,19 @@ func TestFaceBasedSurfaceModel_Tessellates(t *testing.T) { // its OBB box. Returning an empty mesh that still claims SourceBrep would // produce a zero-volume element — the "Collapsed" case the coverage page // exists to surface, reported as a success. +// +// Note what the second case does and does not pin. surfaceModelMesh and +// brepMesh do not type-check set members: brepMesh reads attribute 0 of +// whatever it is handed. An IfcCartesianPoint's attribute 0 is Coordinates, +// which IS a list, so it gets that far — and declines only because the list +// holds reals rather than entity references. So this pins "attribute 0 is not +// a list of face refs", not "the member is rejected for having the wrong +// type". A wrong-typed member whose attribute 0 happened to be a list of +// references would still be walked as a shell. func TestFaceBasedSurfaceModel_EmptyDeclines(t *testing.T) { for _, tc := range []struct{ name, faces string }{ {"empty set", "()"}, - {"non-face-set member", "(#22)"}, // an IfcCartesianPoint + {"member whose attribute 0 is not a list of face refs", "(#22)"}, // an IfcCartesianPoint } { t.Run(tc.name, func(t *testing.T) { f, err := step.Parse(strings.NewReader( From 7dd759c5681d24f03acd3a26e51667b6a424b10d Mon Sep 17 00:00:00 2001 From: Martin Yankovs Date: Thu, 24 Sep 2026 08:53:39 +0300 Subject: [PATCH 4/4] feat(geometry): record the IfcFaceBasedSurfaceModel coverage gain 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) --- docs/coverage.md | 3 +-- parity/testdata/baseline.json | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/docs/coverage.md b/docs/coverage.md index aff1465..84527eb 100644 --- a/docs/coverage.md +++ b/docs/coverage.md @@ -42,7 +42,7 @@ the page cannot quietly disagree with the gate. | Model | Elements | Extrude | Brep | OBB | Empty | OBB rate | Collapsed | AABB ratio p50 | p90 | max | |---|---|---|---|---|---|---|---|---|---|---| | `ifcopenhouse` | 40 | 34 | 0 | 0 | 6 | 0.0% | 0 | 1.0000 | 1.0000 | 1.0000 | -| `duplex_a` | 218 | 146 | 0 | 69 | 3 | 32.1% | 0 | 1.0000 | 1.0000 | 1.1156 | +| `duplex_a` | 218 | 146 | 65 | 4 | 3 | 1.9% | 0 | 1.0000 | 1.0000 | 1.1156 | | `fzk_haus` | 85 | 16 | 64 | 2 | 3 | 2.4% | 0 | 1.0000 | 1.0000 | 1.0334 | ## What falls back, and how often @@ -86,7 +86,6 @@ at least one unhandled item type below. | Item type | Occurrences | |---|---| -| `IFCFACEBASEDSURFACEMODEL` | 65 | | `IFCPOLYGONALBOUNDEDHALFSPACE` | 11 | ## Known Gate 1 violations diff --git a/parity/testdata/baseline.json b/parity/testdata/baseline.json index 85e76e1..d26fe75 100644 --- a/parity/testdata/baseline.json +++ b/parity/testdata/baseline.json @@ -2,8 +2,8 @@ "duplex_a": { "total": 218, "extrude": 146, - "brep": 0, - "obb": 69, + "brep": 65, + "obb": 4, "empty": 3 }, "fzk_haus": {