Skip to content

Commit e409924

Browse files
authored
fix(geometry): preserve winding through Y-up conversion and invalidate stale caches (#4058)
* fix(wasm): preserve triangle winding during Y-up rotation * fix(viewer): invalidate geometry caches with pre-correction winding * docs: record flat orientation correction and qualification limits * docs: cover winding correction across simplification and export * fix(processing): preserve winding through demesher frame rotations * test(wasm): cover generated meshes through demesher winding * fix(export): preserve winding through proper Y-up rotation * chore(wasm): regenerate orientation contract documentation * docs: retain final browser orientation and cache qualification * docs(wasm): remove stale winding and byte-identity claims
1 parent 96ea5f0 commit e409924

18 files changed

Lines changed: 912 additions & 152 deletions

File tree

.changeset/short-lies-cheat.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@ifc-lite/wasm": patch
3+
---
4+
5+
Preserve triangle orientation when rotating flat IFC meshes into viewer coordinates. The rotation preserves handedness, so reversing indices made face winding disagree with the transformed normals and with instanced geometry. Apply the same orientation-preserving convention to simplification and native Y-up exports. Viewer caches created before this correction are invalidated and rebuilt on the next IFC open. Previously exported geometry retains its stored indices and needs regeneration to receive the correction.

apps/viewer/src/hooks/geometryCacheKey.test.ts

Lines changed: 28 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -10,17 +10,17 @@ import { buildGeometryCacheKey } from './geometryCacheKey.js';
1010
describe('buildGeometryCacheKey', () => {
1111
it('folds size, fingerprint and format version into the key', () => {
1212
const key = buildGeometryCacheKey(1024, 'abc123', false, 7);
13-
assert.strictEqual(key, 'ifc-1024-abc123-v7');
13+
assert.strictEqual(key, 'ifc-1024-abc123-v7-g1');
1414
});
1515

16-
it('omits the merge-layers discriminator when merging is off (preserves legacy default-off entries)', () => {
16+
it('omits the merge-layers discriminator when merging is off', () => {
1717
const key = buildGeometryCacheKey(2048, 'deadbeef', false, 5);
1818
assert.ok(!key.includes('-ml'), `expected no merge suffix, got ${key}`);
1919
});
2020

2121
it('appends a merge-layers discriminator when merging is on', () => {
2222
const key = buildGeometryCacheKey(2048, 'deadbeef', true, 5);
23-
assert.strictEqual(key, 'ifc-2048-deadbeef-v5-ml');
23+
assert.strictEqual(key, 'ifc-2048-deadbeef-v5-g1-ml');
2424
});
2525

2626
it('produces distinct keys for the two merge-layers states (issue #1107: toggle+reload must miss)', () => {
@@ -34,40 +34,55 @@ describe('buildGeometryCacheKey', () => {
3434
assert.match(key, /^[A-Za-z0-9_-]+$/);
3535
});
3636

37-
it('omits the skip-small-cuts discriminator by default (preserves legacy full-cut entries)', () => {
37+
it('omits the skip-small-cuts discriminator by default', () => {
3838
const unset = buildGeometryCacheKey(2048, 'deadbeef', false, 5);
3939
const off = buildGeometryCacheKey(2048, 'deadbeef', false, 5, false);
40-
assert.strictEqual(unset, 'ifc-2048-deadbeef-v5');
41-
assert.strictEqual(off, 'ifc-2048-deadbeef-v5');
40+
assert.strictEqual(unset, 'ifc-2048-deadbeef-v5-g1');
41+
assert.strictEqual(off, 'ifc-2048-deadbeef-v5-g1');
4242
});
4343

4444
it('appends a skip-small-cuts discriminator when on (#1286: skipped display cache must not collide with full-cut)', () => {
4545
const skip = buildGeometryCacheKey(2048, 'deadbeef', false, 5, true);
4646
const full = buildGeometryCacheKey(2048, 'deadbeef', false, 5, false);
47-
assert.strictEqual(skip, 'ifc-2048-deadbeef-v5-sc');
47+
assert.strictEqual(skip, 'ifc-2048-deadbeef-v5-g1-sc');
4848
assert.notStrictEqual(skip, full);
4949
});
5050

5151
it('composes the merge-layers and skip-small-cuts discriminators and stays filename-safe', () => {
5252
const key = buildGeometryCacheKey(4096, 'feed', true, 5, true);
53-
assert.strictEqual(key, 'ifc-4096-feed-v5-ml-sc');
53+
assert.strictEqual(key, 'ifc-4096-feed-v5-g1-ml-sc');
5454
assert.match(key, /^[A-Za-z0-9_-]+$/);
5555
});
5656

57-
it('omits the tessellation-tier discriminator at the medium default (preserves legacy entries)', () => {
57+
it('omits the tessellation-tier discriminator at the medium default', () => {
5858
const unset = buildGeometryCacheKey(2048, 'deadbeef', false, 5, false);
5959
const medium = buildGeometryCacheKey(2048, 'deadbeef', false, 5, false, 'medium');
60-
assert.strictEqual(unset, 'ifc-2048-deadbeef-v5');
61-
assert.strictEqual(medium, 'ifc-2048-deadbeef-v5');
60+
assert.strictEqual(unset, 'ifc-2048-deadbeef-v5-g1');
61+
assert.strictEqual(medium, 'ifc-2048-deadbeef-v5-g1');
6262
});
6363

6464
it('appends a tessellation-tier discriminator for a non-default tier (auto-low must not collide with medium)', () => {
6565
const low = buildGeometryCacheKey(2048, 'deadbeef', false, 5, false, 'low');
6666
const medium = buildGeometryCacheKey(2048, 'deadbeef', false, 5, false, 'medium');
67-
assert.strictEqual(low, 'ifc-2048-deadbeef-v5-tlow');
67+
assert.strictEqual(low, 'ifc-2048-deadbeef-v5-g1-tlow');
6868
assert.notStrictEqual(low, medium);
6969
});
7070

71+
it('#4056 invalidates pre-correction geometry for every persisted option combination', () => {
72+
for (const mergeLayers of [false, true]) {
73+
for (const skipSmallCuts of [false, true]) {
74+
for (const tier of ['medium', 'low', 'lowest']) {
75+
// The old persisted namespace is a compatibility witness, not a
76+
// second implementation of the new revision-bearing key.
77+
const legacy = `ifc-4096-feed-v18${mergeLayers ? '-ml' : ''}${skipSmallCuts ? '-sc' : ''}${tier === 'medium' ? '' : `-t${tier}`}`;
78+
const current = buildGeometryCacheKey(4096, 'feed', mergeLayers, 18, skipSmallCuts, tier);
79+
assert.notStrictEqual(current, legacy);
80+
assert.match(current, /^[A-Za-z0-9_-]+$/);
81+
}
82+
}
83+
}
84+
});
85+
7186
it('produces distinct keys per tier so different densities cache separately', () => {
7287
const low = buildGeometryCacheKey(4096, 'feed', false, 5, false, 'low');
7388
const lowest = buildGeometryCacheKey(4096, 'feed', false, 5, false, 'lowest');
@@ -76,7 +91,7 @@ describe('buildGeometryCacheKey', () => {
7691

7792
it('composes all discriminators and stays filename-safe', () => {
7893
const key = buildGeometryCacheKey(4096, 'feed', true, 5, true, 'lowest');
79-
assert.strictEqual(key, 'ifc-4096-feed-v5-ml-sc-tlowest');
94+
assert.strictEqual(key, 'ifc-4096-feed-v5-g1-ml-sc-tlowest');
8095
assert.match(key, /^[A-Za-z0-9_-]+$/);
8196
});
8297
});

apps/viewer/src/hooks/geometryCacheKey.ts

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,10 @@
44

55
import { FORMAT_VERSION } from '@ifc-lite/cache';
66

7+
// #4056: pre-revision flat caches have reversed winding after a proper rotation.
8+
// Invalidate viewer-generated geometry without changing the public cache format.
9+
const GEOMETRY_OUTPUT_REVISION = 1;
10+
711
/**
812
* Build the persisted geometry cache key for a loaded model.
913
*
@@ -20,6 +24,8 @@ import { FORMAT_VERSION } from '@ifc-lite/cache';
2024
* guard + a full-file hash (see `sourceFingerprint.ts` and
2125
* `cacheTier.decideMeshOnlyCacheHit`).
2226
* - `FORMAT_VERSION`: a format bump invalidates incompatible entries
27+
* - `GEOMETRY_OUTPUT_REVISION`: corrected geometry must not hit an entry made
28+
* under an older output policy, even when the binary layout is unchanged.
2329
* - `mergeLayers`: the multi-layer-wall merge flag is a load-time WASM
2430
* tessellation input (issue #540). It was previously absent from the key,
2531
* so toggling it and reloading served geometry built with the *previous*
@@ -37,8 +43,8 @@ import { FORMAT_VERSION } from '@ifc-lite/cache';
3743
* versa).
3844
*
3945
* The `mergeLayers`, `skipSmallCuts`, and `tessellationTier` discriminators are
40-
* omitted at their defaults (`false` / `medium`) so pre-existing cache entries
41-
* stay valid — only the opt-in / non-default paths get a distinct key.
46+
* omitted at their defaults (`false` / `medium`). The output revision applies
47+
* to every option combination; it does not migrate public serialized models.
4248
*
4349
* The desktop (Tauri) cache backend only accepts `[A-Za-z0-9_-]`, so the key
4450
* stays filename-safe and independent of the original filename.
@@ -52,5 +58,5 @@ export function buildGeometryCacheKey(
5258
tessellationTier?: string
5359
): string {
5460
const tier = tessellationTier && tessellationTier !== 'medium' ? `-t${tessellationTier}` : '';
55-
return `ifc-${byteLength}-${fingerprint}-v${formatVersion}${mergeLayers ? '-ml' : ''}${skipSmallCuts ? '-sc' : ''}${tier}`;
61+
return `ifc-${byteLength}-${fingerprint}-v${formatVersion}-g${GEOMETRY_OUTPUT_REVISION}${mergeLayers ? '-ml' : ''}${skipSmallCuts ? '-sc' : ''}${tier}`;
5662
}

packages/wasm/pkg/ifc-lite.d.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -609,8 +609,8 @@ export class IfcAPI {
609609
* Process geometry for a subset of pre-scanned entities → flat
610610
* MeshCollection. Takes raw bytes + pre-pass data from buildPrePassOnce.
611611
* Thin wrapper over [`IfcAPI::produce_batch`]; converts each produced mesh
612-
* to MeshDataJs (the IFC Z-up→WebGL Y-up swap + winding reversal happen
613-
* there). Output is byte-for-byte what the pre-refactor method produced.
612+
* to MeshDataJs (the IFC Z-up→WebGL Y-up rotation with preserved winding happens
613+
* there). Flat indices preserve source order through this proper rotation.
614614
*/
615615
processGeometryBatch(data: Uint8Array, jobs_flat: Uint32Array, unit_scale: number, rtc_x: number, rtc_y: number, rtc_z: number, needs_shift: boolean, void_keys: Uint32Array, void_counts: Uint32Array, void_values: Uint32Array, style_ids: Uint32Array, style_colors: Uint8Array, plane_angle_to_radians?: number | null, material_element_ids?: Uint32Array | null, material_color_counts?: Uint32Array | null, material_colors_rgba?: Uint8Array | null): MeshCollection;
616616
/**

rust/export/src/frame.rs

Lines changed: 7 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
//! GLB/OBJ exporters must redo the identical conversion to match:
1010
//!
1111
//! - positions + normals: `(x, y, z) -> (x, z, -y)`
12-
//! - triangle winding reversed (mirrors `MeshDataJs::new`, keeps front faces)
12+
//! - triangle winding preserved: this is a determinant +1 rotation, not a reflection
1313
//! - per-element `origin` swapped the same way
1414
//!
1515
//! The from-meshes GLB path (the viewer's own `MeshData`) is already Y-up and
@@ -72,7 +72,7 @@ pub(crate) fn yup_matrix4(m: &[f64]) -> [f64; 16] {
7272
/// Reusable owned buffers for the streaming Y-up conversion. The streaming/bounded
7373
/// export passes convert one mesh at a time and drop it; reusing a single scratch across
7474
/// meshes (clear + refill, capacity persists) avoids the 3 fresh heap allocations per
75-
/// mesh that per-call [`to_yup`] incurred — on a million-submesh model, per pass.
75+
/// mesh that allocating fresh buffers would incur — on a million-submesh model, per pass.
7676
pub(crate) struct YUpScratch {
7777
pub positions: Vec<f32>,
7878
pub normals: Vec<f32>,
@@ -86,9 +86,8 @@ impl YUpScratch {
8686
}
8787
}
8888

89-
/// [`to_yup`] into a reusable [`YUpScratch`] instead of freshly allocated buffers. The
90-
/// scratch is cleared and refilled, so its capacity is retained across calls; every value
91-
/// is identical to [`to_yup`], so output is unchanged.
89+
/// Convert into reusable buffers, preserving triangle winding. The scratch is
90+
/// cleared and refilled, so its capacity is retained across calls.
9291
pub(crate) fn to_yup_into(
9392
scratch: &mut YUpScratch,
9493
positions: &[f32],
@@ -108,25 +107,15 @@ pub(crate) fn to_yup_into(
108107
}
109108
scratch.indices.clear();
110109
scratch.indices.extend_from_slice(indices);
111-
let tri_end = scratch.indices.len() - scratch.indices.len() % 3;
112-
let mut i = 0;
113-
while i < tri_end {
114-
scratch.indices.swap(i + 1, i + 2);
115-
i += 3;
116-
}
117110
scratch.origin = yup_f64(origin);
118111
}
119112

120-
/// In-place variant of [`to_yup`]: rewrites `positions` / `normals` / `indices` /
121-
/// `origin` from IFC Z-up to WebGL Y-up WITHOUT allocating new buffers. The in-memory
122-
/// GLB path owns its `MeshData` and drops it immediately after assembly, so mutating it
123-
/// is invisible to any other consumer. Every value matches [`to_yup`]
124-
/// (`(x,y,z) -> (x,z,-y)`, winding reversed), so the emitted GLB is byte-for-byte
125-
/// unchanged — it just skips the full second copy of the model's geometry.
113+
/// In-place variant of [`to_yup_into`], without allocating new buffers.
114+
/// The determinant +1 frame rotation preserves triangle indices and winding.
126115
pub(crate) fn to_yup_in_place(
127116
positions: &mut [f32],
128117
normals: &mut [f32],
129-
indices: &mut [u32],
118+
_indices: &mut [u32],
130119
origin: &mut [f64; 3],
131120
) {
132121
for c in positions.chunks_exact_mut(3) {
@@ -140,12 +129,6 @@ pub(crate) fn to_yup_in_place(
140129
c[1] = z;
141130
c[2] = -y;
142131
}
143-
let tri_end = indices.len() - indices.len() % 3;
144-
let mut i = 0;
145-
while i < tri_end {
146-
indices.swap(i + 1, i + 2);
147-
i += 3;
148-
}
149132
*origin = yup_f64(*origin);
150133
}
151134

rust/export/src/frame_tests.rs

Lines changed: 45 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,8 @@
77
use super::*;
88

99

10-
/// Two triangles whose index triples are all distinct, so an unswapped
11-
/// pass-through is distinguishable from a correctly reversed winding.
10+
/// Two distinct triangles exercise every index in the frame conversion.
1211
const TRIS: [u32; 6] = [0, 1, 2, 1, 2, 3];
13-
/// Per triangle `[a, b, c]` the 2nd/3rd entries swap: `[a, c, b]`.
14-
const TRIS_REVERSED: [u32; 6] = [0, 2, 1, 1, 3, 2];
1512

1613
fn cube_corner_positions() -> Vec<f32> {
1714
vec![
@@ -30,35 +27,39 @@ fn yup_swaps_and_negates_the_expected_axes() {
3027
assert_eq!(yup_f64([1.0, 2.0, 3.0]), [1.0, 3.0, -2.0]);
3128
}
3229

33-
/// The module header declares reversed triangle winding a load-bearing part
34-
/// of the Z-up→Y-up contract ("mirrors `MeshDataJs::new`, keeps front
35-
/// faces"). Nothing pinned it: deleting the swap from BOTH `to_yup_into` and
36-
/// `to_yup_in_place` AND from `obj.rs`'s hand-written copy left the whole
37-
/// crate suite green (82/82). glTF materials are emitted `doubleSided: true`
38-
/// unconditionally, so no renderer-facing assertion can ever fail on
39-
/// winding, and the `*_is_byte_identical` tests compare the exporter against
40-
/// itself — a bug applied consistently is invisible to them by construction.
41-
///
42-
/// So pin each copy DIRECTLY against a literal, rather than against another
43-
/// copy: an equivalence test between two implementations of the same
44-
/// conversion cannot see a mutation applied symmetrically to both.
45-
#[test]
46-
fn to_yup_into_reverses_triangle_winding() {
47-
let mut scratch = YUpScratch::new();
48-
let positions = cube_corner_positions();
49-
let normals = vec![0.0f32; positions.len()];
50-
to_yup_into(&mut scratch, &positions, &normals, &TRIS, [0.0, 0.0, 0.0]);
51-
assert_eq!(scratch.indices, TRIS_REVERSED, "streaming path must reverse winding");
30+
// #4056: an independent geometric invariant catches an incorrect reversal in
31+
// either conversion, even when both implementations make the same mistake.
32+
fn assert_faces_agree_with_normals(positions: &[f32], normals: &[f32], indices: &[u32]) {
33+
for tri in indices.chunks_exact(3) {
34+
let p = |i: usize| &positions[tri[i] as usize * 3..][..3];
35+
let a = p(0);
36+
let b = p(1);
37+
let c = p(2);
38+
let u = [b[0] - a[0], b[1] - a[1], b[2] - a[2]];
39+
let v = [c[0] - a[0], c[1] - a[1], c[2] - a[2]];
40+
let cross = [u[1]*v[2] - u[2]*v[1], u[2]*v[0] - u[0]*v[2], u[0]*v[1] - u[1]*v[0]];
41+
let n = &normals[tri[0] as usize * 3..][..3];
42+
let dot = cross[0]*n[0] + cross[1]*n[1] + cross[2]*n[2];
43+
assert!(dot > 0.0, "face must agree with its normal: {dot}");
44+
}
5245
}
5346

5447
#[test]
55-
fn to_yup_in_place_reverses_triangle_winding() {
56-
let mut positions = cube_corner_positions();
57-
let mut normals = vec![0.0f32; positions.len()];
48+
fn frame_paths_preserve_face_normal_agreement_4056() {
49+
let positions = cube_corner_positions();
50+
let normals = [0.0, 0.0, 1.0].repeat(4);
51+
assert_faces_agree_with_normals(&positions, &normals, &TRIS);
52+
let mut scratch = YUpScratch::new();
53+
to_yup_into(&mut scratch, &positions, &normals, &TRIS, [0.0; 3]);
54+
assert_faces_agree_with_normals(&scratch.positions, &scratch.normals, &scratch.indices);
55+
assert_eq!(scratch.indices, TRIS);
56+
57+
let mut positions = positions;
58+
let mut normals = normals;
5859
let mut indices = TRIS;
59-
let mut origin = [0.0f64; 3];
60-
to_yup_in_place(&mut positions, &mut normals, &mut indices, &mut origin);
61-
assert_eq!(indices, TRIS_REVERSED, "in-place path must reverse winding");
60+
to_yup_in_place(&mut positions, &mut normals, &mut indices, &mut [0.0; 3]);
61+
assert_faces_agree_with_normals(&positions, &normals, &indices);
62+
assert_eq!(indices, TRIS);
6263
}
6364

6465
/// The two frame paths are documented as producing identical output; the
@@ -102,31 +103,26 @@ fn to_yup_into_clears_previous_contents_when_reused() {
102103
to_yup_into(&mut scratch, &positions, &normals, &TRIS, [0.0, 0.0, 0.0]);
103104
assert_eq!(scratch.positions.len(), positions.len());
104105
assert_eq!(scratch.normals.len(), normals.len());
105-
assert_eq!(scratch.indices, TRIS_REVERSED);
106+
assert_eq!(scratch.indices, TRIS);
106107
}
107108

108109
#[test]
109110
fn trailing_partial_triangle_is_left_alone() {
110-
// `tri_end` rounds down to a whole triangle; a stray index must not be
111-
// swapped into the previous triangle. Kills `% 3` -> `% 2`.
112-
//
113-
// Both paths compute `tri_end` independently, so both are asserted — the
114-
// whole point of this file is that pinning one copy of a duplicated
115-
// calculation says nothing about the other.
111+
// A frame rotation leaves every index unchanged, including a partial tail.
116112
let positions = cube_corner_positions();
117113
let normals = vec![0.0f32; positions.len()];
118114

119115
let mut scratch = YUpScratch::new();
120116
let indices: Vec<u32> = vec![0, 1, 2, 3];
121117
to_yup_into(&mut scratch, &positions, &normals, &indices, [0.0, 0.0, 0.0]);
122-
assert_eq!(scratch.indices, vec![0, 2, 1, 3], "streaming path");
118+
assert_eq!(scratch.indices, vec![0, 1, 2, 3], "streaming path");
123119

124120
let mut ip = positions.clone();
125121
let mut in_ = normals.clone();
126122
let mut ii: [u32; 4] = [0, 1, 2, 3];
127123
let mut io = [0.0f64; 3];
128124
to_yup_in_place(&mut ip, &mut in_, &mut ii, &mut io);
129-
assert_eq!(ii, [0, 2, 1, 3], "in-place path");
125+
assert_eq!(ii, [0, 1, 2, 3], "in-place path");
130126
}
131127

132128
/// IFC Z is up; glTF Y is up. A rotation about the IFC up-axis has to come out
@@ -201,3 +197,14 @@ fn the_bottom_row_is_left_alone() {
201197
// Column-major: the bottom row is entries 3, 7, 11, 15.
202198
assert_eq!([y[3], y[7], y[11], y[15]], [0.0, 0.0, 0.0, 1.0], "{y:?}");
203199
}
200+
201+
/// #4056: preserving winding during the frame rotation must not erase an
202+
/// actual reflection carried by an element placement.
203+
#[test]
204+
fn frame_change_preserves_a_reflected_placement_4056() {
205+
let m = [-1.0, 0.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0,
206+
0.0, 0.0, 1.0, 0.0, 0.0, 0.0, 0.0, 1.0];
207+
let y = yup_matrix4(&m);
208+
assert_eq!(y, m, "an X reflection remains an X reflection after changing up-axis");
209+
assert_eq!(y[0] * y[5] * y[10], -1.0);
210+
}

rust/export/src/gltf.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1613,7 +1613,7 @@ fn with_result_views<R>(
16131613
// `process_geometry` emits the producer-native IFC **Z-up** frame (the Z-up→Y-up
16141614
// swap normally happens at the wasm FFI, which this path never crosses). glTF
16151615
// mandates +Y-up, so convert each visible mesh to Y-up — positions/normals
1616-
// swapped, winding reversed, origin swapped — matching the viewer/legacy output.
1616+
// rotated, winding preserved, origin rotated — matching the viewer/legacy output.
16171617
//
16181618
// The visible indices are collected first so the immutable visibility borrow ends
16191619
// before the in-place mutation; then each visible mesh is converted to Y-up IN PLACE

0 commit comments

Comments
 (0)