Skip to content

Commit c3f0da3

Browse files
authored
fix(viewer): colour overrides reach assembly parts whose meshes stream in later (#3899)
Closes #3890. Stacked on #3880 and retargeted to main once it lands. #3880 makes hide and isolate complete at action time, but colour was left out: both colour sinks drain and clear, and `scene.setColorOverrides` builds overlay batches once from the meshes present, so an assembly part that streams in later stays unpainted. A small hook now watches geometry arrival while a colour override map is installed and re-issues `setColorOverrides(scene.getColorOverrides())` so the overlay batches are rebuilt with the late meshes. Guards: it reads the scene's retained map rather than a captured one, so a user's targeted reset stays reset and a later correction is not undone; it retries while `hasQueuedMeshes()` because meshes only enter the scene on queue drain, not batch receipt; and the waiting set is only narrowed once the queue is empty so an entity whose remaining stream fragments are still queued is not stranded. The presence probe uses the two O(1) scene accessors added here rather than materialising instanced geometry. Test written first: with the flush effect alone the late part was absent from the rebuilt overlay batch (2 pass, 1 fail); after the fix 5 of 5, and each guard is mutation-tested (deleting the arrival check, replaying a captured map, or deleting the drain retry each fails its test). Deferred with a note in the module doc: the deeper fix would live in `scene.appendToBatches`. Gates on the head: viewer typecheck 0, full viewer suite 6880 pass / 0 fail, renderer suite 1204 pass, check-module-size 0 (`useGeometryStreaming.ts` shrank and its row was ratcheted down), check-isolate-expansion-routing 0, check-unused-locals 0. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Color overrides now correctly reapply to meshes that arrive through late streaming. * User resets and subsequent color corrections are preserved. * Unnecessary renderer rebuilds are avoided while streamed geometry is still loading. * **Improvements** * Color updates are retried automatically as queued meshes become available. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 parent 34aacc6 commit c3f0da3

9 files changed

Lines changed: 845 additions & 25 deletions
Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
---
2+
"ifc-lite": patch
3+
"@ifc-lite/renderer": minor
4+
---
5+
6+
fix(viewer): colouring an assembly now reaches parts whose meshes stream in later (#3890)
7+
8+
Hide and isolate are whitelists the renderer re-matches mesh ids against, so a part that streams in after the action is caught the moment its mesh lands. Colour was not: `pendingColorUpdates` is a one-shot signal that is flushed and nulled, and `scene.setColorOverrides` builds overlay batches once from `meshDataMap`, so a part with no mesh at flush time was never painted at all.
9+
10+
The colour channel now hands the scene back its own retained override map once the geometry counter settles and the mesh queue has drained, which rebuilds the overlay batches with the late meshes included. It does that only when an override id it was waiting on has actually arrived, so an active overlay does not pay a rebuild after every later streaming burst, and it costs nothing when nothing is coloured. Because the map is read from the scene at that moment rather than remembered, a targeted `resetColors` is not repainted and a correction made in the meantime is the colour that lands.
11+
12+
GPU-instanced occurrences need their own half of this and cannot use the counter: a streaming event carrying only instanced shards appends through `appendInstancedShards`, which never changes `geometryResult`, so the counter does not move. `Scene.addInstancedShard` now applies a recorded colour override to occurrences arriving in the shard, mirroring the late-selection seeding that already sat beside it.
13+
14+
`SceneContents` gains `getColorOverrides`, `hasMeshData` and `isInstancedEntity`. The first exposes the retained map the catch-up re-applies; the other two are the O(1) presence probes it needs, since `getMeshDataPieces` and `getInstancedMeshDataPieces` answer the same question but materialize geometry to do it.
Lines changed: 375 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,375 @@
1+
/* This Source Code Form is subject to the terms of the Mozilla Public
2+
* License, v. 2.0. If a copy of the MPL was not distributed with this
3+
* file, You can obtain one at https://mozilla.org/MPL/2.0/. */
4+
5+
/**
6+
* Colour reaches parts whose meshes stream in AFTER the colour was applied
7+
* (#3890).
8+
*
9+
* #3880 made hide and isolate complete at action time by expanding an assembly
10+
* to every aggregated descendant: those two are whitelists the renderer
11+
* re-matches mesh ids against, so a mesh-less id starts matching the moment
12+
* its mesh lands. Colour gets no such benefit. `pendingColorUpdates` is a
13+
* one-shot signal — `useColorOverlaySync` hands it to `scene.setColorOverrides`
14+
* and nulls it out — and `setColorOverrides` builds overlay batches ONCE by
15+
* looking each id up in `meshDataMap`. An id with no mesh at flush time
16+
* contributes nothing to a batch and nothing ever revisits it.
17+
*
18+
* The stub scene below models exactly that: `setColorOverrides` retains the
19+
* map and derives `paintedIds` from the ids that currently have a mesh. The
20+
* real `Scene` does the same thing (packages/renderer/src/scene.ts: it copies
21+
* `overrides` into `this.colorOverrides`, then walks the map and only pushes
22+
* `meshDataMap.get(expressId)` pieces into a colour group).
23+
*
24+
* The two hazards recorded on the issue get a test each:
25+
* - a targeted `resetColors([part])` must not be repainted by the next tick;
26+
* - the repaint must not undo a user's LATER correction.
27+
* Both are covered by re-reading `scene.getColorOverrides()` at the moment the
28+
* debounce fires rather than capturing a map when it is scheduled, and both
29+
* are asserted below so that a future "optimisation" that captures early
30+
* fails here.
31+
*/
32+
33+
import '@/test/setup-dom.js';
34+
import { describe, it } from 'node:test';
35+
import assert from 'node:assert/strict';
36+
import { act } from 'react';
37+
import { createRoot, type Root } from 'react-dom/client';
38+
import type { Renderer } from '@ifc-lite/renderer';
39+
import { createViewerAdapter } from '../../sdk/adapters/viewer-adapter.js';
40+
import type { StoreApi } from '../../sdk/adapters/types.js';
41+
import { useColorOverlaySync } from './useColorOverlaySync.js';
42+
43+
type Color = [number, number, number, number];
44+
type ColorMap = Map<number, Color>;
45+
46+
/**
47+
* Stands in for `Scene`'s colour-overlay half. `paintedIds` is the overlay
48+
* batch: built once per `setColorOverrides` call from the ids that have a mesh
49+
* RIGHT NOW, which is the defect's whole mechanism.
50+
*/
51+
class StubScene {
52+
meshedIds = new Set<number>();
53+
private overrides: ColorMap | null = null;
54+
paintedIds: number[] = [];
55+
setColorOverridesCalls = 0;
56+
57+
setColorOverrides(overrides: ColorMap): void {
58+
this.setColorOverridesCalls++;
59+
if (overrides.size === 0) {
60+
this.overrides = null;
61+
this.paintedIds = [];
62+
return;
63+
}
64+
this.overrides = new Map(overrides);
65+
this.paintedIds = [...overrides.keys()].filter((id) => this.meshedIds.has(id));
66+
}
67+
68+
getColorOverrides(): ColorMap | null {
69+
return this.overrides;
70+
}
71+
72+
/** `meshDataMap` presence: what the hook asks to decide whether an id that
73+
* had no mesh at the last build has since gained one. */
74+
hasMeshData(expressId: number): boolean {
75+
return this.meshedIds.has(expressId);
76+
}
77+
78+
isInstancedEntity(): boolean {
79+
return false;
80+
}
81+
82+
/** Meshes are RECEIVED into a queue and only enter `meshDataMap` when the
83+
* animation loop drains it, so "a batch arrived" and "the mesh is paintable"
84+
* are different moments. Scenarios that care set this. */
85+
queuedMeshes = false;
86+
87+
hasQueuedMeshes(): boolean {
88+
return this.queuedMeshes;
89+
}
90+
91+
clearColorOverrides(): void {
92+
this.overrides = null;
93+
this.paintedIds = [];
94+
}
95+
}
96+
97+
function fakeRenderer(scene: StubScene): Renderer {
98+
return {
99+
getGPUDevice: () => ({}) as unknown,
100+
getPipeline: () => ({}) as unknown,
101+
getScene: () => scene,
102+
requestRender: () => {},
103+
} as unknown as Renderer;
104+
}
105+
106+
/** The store shape `createViewerAdapter` actually reads, with idOffset 0 so a
107+
* ref's expressId IS its global id. */
108+
function makeStore(): { store: StoreApi; getPending: () => ColorMap | null; clear: () => void } {
109+
let pendingColorUpdates: ColorMap | null = null;
110+
const state = {
111+
models: new Map([['default', { idOffset: 0 }]]),
112+
// #3880's expansion: the assembly resolves to both of its parts, meshed or
113+
// not. This is `cameraCallbacks.resolveHighlightIds`' post-#3865 answer.
114+
cameraCallbacks: {
115+
resolveHighlightIds: (ids: number[]) =>
116+
ids.flatMap((id) => (id === ASSEMBLY ? [PART_MESHED, PART_LATE] : [id])),
117+
},
118+
get pendingColorUpdates() {
119+
return pendingColorUpdates;
120+
},
121+
setPendingColorUpdates: (updates: ColorMap) => {
122+
pendingColorUpdates = new Map(updates);
123+
},
124+
};
125+
return {
126+
store: { getState: () => state, subscribe: () => () => {} } as unknown as StoreApi,
127+
getPending: () => pendingColorUpdates,
128+
clear: () => {
129+
pendingColorUpdates = null;
130+
},
131+
};
132+
}
133+
134+
const ASSEMBLY = 100;
135+
const PART_MESHED = 101;
136+
const PART_LATE = 102;
137+
const RED: Color = [1, 0, 0, 1];
138+
const BLUE: Color = [0, 0, 1, 1];
139+
140+
/** Debounce window in `useColorOverlaySync`, plus slack. */
141+
const PAST_DEBOUNCE_MS = 400;
142+
143+
function Harness({
144+
rendererRef,
145+
pendingColorUpdates,
146+
clearPendingColorUpdates,
147+
geometryVersion,
148+
}: {
149+
rendererRef: { current: Renderer | null };
150+
pendingColorUpdates: ColorMap | null;
151+
clearPendingColorUpdates: () => void;
152+
geometryVersion: number;
153+
}) {
154+
useColorOverlaySync({
155+
rendererRef,
156+
isInitialized: true,
157+
pendingColorUpdates,
158+
clearPendingColorUpdates,
159+
geometryVersion,
160+
});
161+
return null;
162+
}
163+
164+
const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms));
165+
166+
/**
167+
* Drives one scenario: the real adapter writes into the real-shaped store, the
168+
* real hook flushes into the stub scene, then a mesh lands and the geometry
169+
* counter bumps.
170+
*/
171+
async function mount(scene: StubScene) {
172+
const container = document.createElement('div');
173+
document.body.appendChild(container);
174+
const root: Root = createRoot(container);
175+
const rendererRef = { current: fakeRenderer(scene) };
176+
const { store, getPending, clear } = makeStore();
177+
const adapter = createViewerAdapter(store);
178+
let version = 0;
179+
180+
const render = async () => {
181+
await act(async () => {
182+
root.render(
183+
<Harness
184+
rendererRef={rendererRef}
185+
pendingColorUpdates={getPending()}
186+
clearPendingColorUpdates={clear}
187+
geometryVersion={version}
188+
/>,
189+
);
190+
});
191+
};
192+
193+
return {
194+
adapter,
195+
render,
196+
async bumpGeometry() {
197+
version++;
198+
await render();
199+
await act(async () => {
200+
await sleep(PAST_DEBOUNCE_MS);
201+
});
202+
},
203+
/** Let pending timers run without bumping the geometry counter. */
204+
async settle() {
205+
await act(async () => {
206+
await sleep(PAST_DEBOUNCE_MS);
207+
});
208+
},
209+
cleanup() {
210+
act(() => root.unmount());
211+
container.remove();
212+
},
213+
};
214+
}
215+
216+
describe('useColorOverlaySync — colour reaches late-streamed meshes (#3890)', () => {
217+
it('repaints a part whose mesh arrives after the colour was flushed', async () => {
218+
const scene = new StubScene();
219+
scene.meshedIds.add(PART_MESHED);
220+
const h = await mount(scene);
221+
try {
222+
h.adapter.colorize([{ modelId: 'default', expressId: ASSEMBLY }], RED);
223+
await h.render();
224+
225+
assert.deepEqual(
226+
scene.paintedIds.sort(),
227+
[PART_MESHED],
228+
'only the already-meshed part can be in the first overlay batch',
229+
);
230+
231+
// PART_LATE's mesh streams in.
232+
scene.meshedIds.add(PART_LATE);
233+
await h.bumpGeometry();
234+
235+
assert.ok(
236+
scene.paintedIds.includes(PART_LATE),
237+
'the late part must be in the rebuilt overlay batch',
238+
);
239+
assert.ok(
240+
scene.paintedIds.includes(PART_MESHED),
241+
'the already-painted part must survive the rebuild',
242+
);
243+
} finally {
244+
h.cleanup();
245+
}
246+
});
247+
248+
it('leaves a part the user reset alone when its mesh arrives later', async () => {
249+
const scene = new StubScene();
250+
scene.meshedIds.add(PART_MESHED);
251+
const h = await mount(scene);
252+
try {
253+
h.adapter.colorize([{ modelId: 'default', expressId: ASSEMBLY }], RED);
254+
await h.render();
255+
// The user un-colours the part that has not rendered yet.
256+
h.adapter.resetColors([{ modelId: 'default', expressId: PART_LATE }]);
257+
await h.render();
258+
259+
scene.meshedIds.add(PART_LATE);
260+
await h.bumpGeometry();
261+
262+
assert.equal(
263+
scene.paintedIds.includes(PART_LATE),
264+
false,
265+
'a reset part must stay reset when its mesh lands',
266+
);
267+
assert.ok(scene.paintedIds.includes(PART_MESHED), 'the rest of the assembly stays painted');
268+
} finally {
269+
h.cleanup();
270+
}
271+
});
272+
273+
it('applies a correction made before the late mesh landed, not the older colour', async () => {
274+
const scene = new StubScene();
275+
scene.meshedIds.add(PART_MESHED);
276+
const h = await mount(scene);
277+
try {
278+
h.adapter.colorize([{ modelId: 'default', expressId: ASSEMBLY }], RED);
279+
await h.render();
280+
// The user corrects the still-unmeshed part to BLUE. The catch-up has
281+
// not run yet, so this is the map it will find when it does.
282+
h.adapter.colorize([{ modelId: 'default', expressId: PART_LATE }], BLUE);
283+
await h.render();
284+
285+
scene.meshedIds.add(PART_LATE);
286+
await h.bumpGeometry();
287+
288+
assert.ok(
289+
scene.paintedIds.includes(PART_LATE),
290+
'the catch-up must actually run for this test to mean anything',
291+
);
292+
assert.deepEqual(
293+
scene.getColorOverrides()?.get(PART_LATE),
294+
BLUE,
295+
'the correction must survive: the catch-up re-reads the scene map rather than replaying an older one',
296+
);
297+
assert.deepEqual(scene.getColorOverrides()?.get(PART_MESHED), RED);
298+
} finally {
299+
h.cleanup();
300+
}
301+
});
302+
303+
it('stops rebuilding once every override that can be painted has been', async () => {
304+
const scene = new StubScene();
305+
scene.meshedIds.add(PART_MESHED);
306+
const h = await mount(scene);
307+
try {
308+
h.adapter.colorize([{ modelId: 'default', expressId: ASSEMBLY }], RED);
309+
await h.render();
310+
311+
// What makes this test bite, pinned rather than assumed: the map keeps
312+
// the geometry-less ASSEMBLY id next to its parts. The fixture's resolver
313+
// answers with the parts only, but `resolvePresentationColorMap` unions
314+
// the raw ids back in (#2680's never-substitute policy), so the id that
315+
// can never gain a mesh IS in the override map, exactly as in the app.
316+
assert.ok(
317+
scene.getColorOverrides()?.has(ASSEMBLY),
318+
'the geometry-less assembly id must be in the override map',
319+
);
320+
assert.equal(scene.hasMeshData(ASSEMBLY), false, 'and it never gains a mesh');
321+
322+
scene.meshedIds.add(PART_LATE);
323+
await h.bumpGeometry();
324+
const afterCatchUp = scene.setColorOverridesCalls;
325+
326+
// Two more streaming bursts that bring nothing the overlay is waiting
327+
// for. Because ASSEMBLY stays unmeshed forever, "is anything still
328+
// waiting" would rebuild here on every burst, for the life of the
329+
// overlay. "Did an awaited id arrive" is the question that terminates.
330+
await h.bumpGeometry();
331+
await h.bumpGeometry();
332+
333+
assert.equal(
334+
scene.setColorOverridesCalls,
335+
afterCatchUp,
336+
'a burst that lands no awaited mesh must not rebuild the overlay batches',
337+
);
338+
} finally {
339+
h.cleanup();
340+
}
341+
});
342+
343+
it('waits for the mesh queue to drain instead of spending its one shot mid-drain', async () => {
344+
const scene = new StubScene();
345+
scene.meshedIds.add(PART_MESHED);
346+
const h = await mount(scene);
347+
try {
348+
h.adapter.colorize([{ modelId: 'default', expressId: ASSEMBLY }], RED);
349+
await h.render();
350+
351+
// The last batch has been RECEIVED — the counter bumps and will never
352+
// bump again — but the animation loop has not drained it, so PART_LATE
353+
// is not in meshDataMap yet.
354+
scene.queuedMeshes = true;
355+
await h.bumpGeometry();
356+
assert.equal(
357+
scene.paintedIds.includes(PART_LATE),
358+
false,
359+
'nothing to paint yet: the mesh is still queued',
360+
);
361+
362+
// The drain completes. No further geometry counter bump follows.
363+
scene.queuedMeshes = false;
364+
scene.meshedIds.add(PART_LATE);
365+
await h.settle();
366+
367+
assert.ok(
368+
scene.paintedIds.includes(PART_LATE),
369+
'the catch-up must retry across the drain, not give up after one attempt',
370+
);
371+
} finally {
372+
h.cleanup();
373+
}
374+
});
375+
});

0 commit comments

Comments
 (0)