feat(ifc): preserve unsupported IFC geometry - #553
Conversation
Aymericr
left a comment
There was a problem hiding this comment.
Thanks for this — it's tackling a real gap, and the layering is the part I most want to keep. A format-neutral imported-mesh node in core (packages/core/src/schema/nodes/imported-mesh.ts) with the Three.js and floorplan builders in packages/nodes/src/imported-mesh/ is the right shape: no kind-specific dispatch leaked into the viewer or editor, no project-name or material-name heuristics, and presentation.hidden: true genuinely keeps it out of the palette (apps/editor/components/build-tab.tsx:90 is the only consumer of that flag). Replacing the skippedBeams / skippedItems console.warn with actual preserved geometry is a clear improvement, and it gives #158 (AutoCAD) and #174 (Sweet Home 3D) a primitive to reuse later. The Pset_DoorCommon / OperationType mapping in door-semantics.ts is also the right call — standardized IFC properties instead of name sniffing — and every value it emits validates against DoorNode. Your Verification section is honest: I reproduced all of it (nodes 877 pass/1 skip, ifc-converter 9 pass, core 760 pass, duplex 245 nodes / 100 fallbacks exactly). Merge onto main is clean and the merged tree typechecks and passes biome, so the stale merge base costs nothing.
The blocker is a unit bug. extractImportedMeshPrimitives multiplies GetFlatMesh vertices by unitFactor (packages/ifc-converter/src/index.ts:684), but web-ifc already normalizes to metres — I checked by reading raw GetFlatMesh output directly, and on 04-ifc-open-house.ifc (which declares MILLIMETRE, unitFactor 0.001) the raw wall bbox is already -5.05..5.05 m. So on that file native walls land correctly at ±5 m while the imported-mesh bbox extent comes out 0.0107 x 0.0078 x 0.0058 m instead of ~10 x 3 x 8 m. Same on 10-sample-house.ifc, where all 4 zones also get ceilingHeight clamped to the 0.1 floor. Two of the four bundled fixtures are millimetre files, and I think they just weren't in the loop — everything you validated (duplex, and paris) happens to be unitFactor 1.
The fix is small: scale only originOffset (raw STEP data), not the flat-mesh coords.
if (swapYZ) {
positions.push(
world[0]! - originOffset[0]! * unitFactor,
world[1]! - originOffset[2]! * unitFactor - levelElevation,
-(world[2]! + originOffset[1]! * unitFactor),
)
}I applied exactly that and confirmed 04-ifc-open-house becomes 10.65 x 7.78 x 5.80 m and sample-house coverings align with the native walls, while duplex and the georeferenced paris file are byte-identical to before. Could you add a regression test asserting the imported-mesh bbox lands within a metre of the native wall bbox on a millimetre fixture? That's the guard that would have caught this.
Three more I'd want fixed before merge:
-
Stair flights double-render.
IFCSTAIRFLIGHTis infallbackTypes(index.ts:2245) but theIFCSTAIRpass never registers child flight express IDs, so duplex emits 2 native stairs plus 2 flight meshes over the same volume. AfterexpressIdToNodeId.set(stairExpressID, nodeId), claim the descendants fromchildrenMapso the fallback skips them. (Bugbot flagged this one and it's real.) -
space.Namenull-deref silently deletes all zones.index.ts:2229readsspace.Name.valueafter guarding onlyspace.Name?.value. web-ifc yields literalnullfor unset optionals — I verified this on duplex (Description,ObjectType,ElevationWithFlooringare allnull). Because thetry/catchatindex.ts:2165wraps the whole loop, oneIfcSpacewithLongNamebut noNamewipes every remaining zone in the file. Guard the read, and move the try/catch inside the loop so a malformed space skips only itself. -
ROOF slabs are lost, not rerouted. On
04-ifc-open-house,mainemits 2 slabs ("South roof" / "North roof", bothPredefinedType=ROOF); this branch emits 0 and no compensating imported-mesh appears. The skip atindex.ts:1750assumes the mesh fallback catches them and it doesn't here. Extract primitives first and only skip the native path whenprimitives.length > 0.
On scope: the diff has grown well past its title. Wall centerline recomputation, the 5x wall-merge tolerance cut, the material merge guard, level height/index assignment, storey inference, level-relative elevations, skylight/landing/roof skipping, unhosted-opening removal, and IfcSpace → zone are each independently risky. Two are measurably behavior-changing: the tolerance cut takes 05-paris-ground-floor from mergedWallGroups 15 / removedMergedWalls 28 to 8 / 13, leaving 100 walls where main leaves 85; and the material guard that ships with it is a no-op whenever only one fragment has a material association, since wallMaterialCompatible returns true if either side is missing (cleanup.ts:212 — also Bugbot). Could you split the cleanup.ts changes into their own PR with before/after wall counts for all four fixtures? That would let the mesh-preservation work land on its own merits.
Two smaller things worth folding in while you're here:
capabilities.movablewill double-transform. The converter bakes level-local world coordinates intoprimitivesand leavespositionat[0,0,0], butmove-registry-node-toolcommits an absolute plan position andParametricNodeRendererappliesnode.positionon the outer group (parametric-node-renderer.tsx:86). Simplest fix consistent with the "import-only" framing: dropmovablefor now and keep selectable + deletable. (Reasoned from the two code paths, not observed in the UI — worth a manual check.)- Payload. Positions and normals serialize as unrounded doubles, so imported-mesh is 96% of duplex's 4.48 MB scene and 91% of paris's 6.66 MB. Rounding positions to 1e-4 m (0.1 mm) and normals to 1e-3 cuts duplex to 1.97 MB with no visible change. Worth doing before this reaches hosted
scene_graphrows.
One design note rather than a defect: the mesh-hull zone fallback (meshFootprint, index.ts:748) is a convex hull, so a concave or L-shaped space with no swept profile gets an inflated polygon that still counts as a successful import. I'd rather skip the zone and let the mesh preserve exact geometry, or set metadata.footprintApproximated — a wrong room boundary looks authoritative in a way a missing one doesn't.
Fix the unit scaling, the stair duplication, the Name deref, and the ROOF slabs, split out cleanup.ts, and I think this is close.
3449f30 to
f5635ae
Compare
|
Addressed the requested review items in f5635ae:
The cleanup changes are now isolated in #603, including main-vs-branch wall counts for all four bundled fixtures. I also made material compatibility explicit when only one fragment has a material association. Verification is updated in the PR description: converter tests 11 passed, nodes 946 passed / 1 skipped, core passed, all three affected packages build, and focused Biome checks pass. |
|
One scope clarification on the storey-related changes that remain in this PR: They are coupled to imported-mesh preservation rather than being a separate cleanup pass. Pascal renders level children in level-local coordinates and stacks levels using The inference is source-semantic only: prefer explicit IFC spatial containment; otherwise select the nearest storey at or below the element elevation, with the lowest storey as the below-grade fallback. Focused tests cover below-all, between-storeys, above-all, and no-storey cases. I did separate the unrelated wall-cleanup behavior into #603 as requested. If you would still prefer the storey normalization as a prerequisite PR, I can split it, but #553 would then need to depend on that PR for correctly placed fallback geometry. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit be1afd5. Configure here.

Summary
Architecture
Review fixes
Verification
Note
Medium Risk
Large changes to IFC import (coordinates, parenting, and fallback vs native paths) can misplace geometry or affect slab/wall support; schema and event bus additions are additive and well-tested on fixtures.
Overview
Adds a first-class
imported-meshnode in core (schema, union, level children, editor events) and registers it inpackages/nodeswith Three.js geometry, floorplan bounds, selectable/deletable-but-not-movable behavior, and palette hidden.The IFC converter is reworked so elements without a reliable native mapping become serialized triangle buffers (with colors, rounded positions/normals, and a dedicated WebIFC axis mapping that avoids double
swapYZ). IfcSpace becomes room zones; stair flights drop parametric stair nodes in favor of meshes; roof/landing slabs with mesh skip horizontal SlabNode conversion. Parenting uses storey inference by elevation when containment is missing, level heights from IFC storey spacing, and sharedattachNodeToGraph. Native conversion fixes include wall centerlines from profile, level-local elevations, and doors/windows only when wall-hosted (with OperationType / GlazingAreaFraction semantics).wasmPathis configurable for Node tests.New unit/integration tests cover door/storey helpers and fixture regressions (mesh–wall alignment, roof slabs, stair flights, malformed spaces).
Reviewed by Cursor Bugbot for commit 972db34. Bugbot is set up for automated code reviews on this repo. Configure here.