From eb0c63e6fac8f412babeb71934391eeb2d00192e Mon Sep 17 00:00:00 2001 From: Petru Conduraru Date: Fri, 21 Aug 2026 11:40:01 +0300 Subject: [PATCH 1/3] fix(embed): make SET_CAMERA, RESET_COLORS and ENTITY_HOVERED do something (#2934) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three commands the embed API advertises reported success while doing nothing. Each was broken at a different link, so each needed a different fix. SET_CAMERA had no actuator at all. handler.ts called the store's setCameraRotation, which was `set({ cameraRotation })` and stopped there: every orientation entry point on the camera is relative (orbit, the 90° rotate steppers) or names a direction (setPresetView), so an absolute azimuth/elevation pair had nothing to reach. The host received a requestId ack AND a CAMERA_CHANGED echo of its own numbers while the view never moved. Adds Camera.setRotation(azimuth, elevation) — the inverse of Camera.getRotation, keeping the target and orbit distance, normalizing azimuth into 0-360, clamping elevation to MIN_PHI off the poles (as orbit does), rejecting non-finite angles, and cancelling any in-flight tween so the next update() cannot erase the pose. cameraSlice.setCameraRotation now drives it through a new CameraCallbacks.setCameraRotation, the same shape setProjectionMode already used, registered in Viewport.tsx. RESET_COLORS cleared the wrong channel, wrong in both directions. SET_COLORS bakes into geometryResult.meshes[].color via updateMeshColors; clearPendingColorUpdates empties pendingColorUpdates, the transient overlay channel the lens, IDS, clash and schedule overlays own. So the host's own override survived the reset and another subsystem's claim was destroyed by it. updateMeshColors takes `{ override: true }`, which captures the colors it displaces into meshColorBackup (first write per entity wins), and the new resetMeshColors restores those, re-queues them for the renderer, and leaves pendingColorUpdates alone. The loader's deferred IFC style pass deliberately does NOT pass `override`: those colors are the model's, and backing them up would make a reset strip the model's own styling. ENTITY_HOVERED had zero emit sites in apps/viewer-embed. The SDK's tests pass because they call harness.emit('ENTITY_HOVERED', ...) themselves, proving the SDK dispatches an event the viewer never sent. The viewer's hover pipeline (useMouseControls' throttled renderer.pick -> setHoverState) was already reachable but gated on hoverTooltipsEnabled, which defaults false and has no embed chrome to toggle it; the embed now forces it on (safe — it never renders HoverTooltip) and emits on each hover-target change, subscribing to hoverState.entityId so a pointer drifting within one mesh does not re-post. SET_CAMERA's `zoom` stays unapplied and is now documented as reserved rather than silently dropped: it has no defined meaning on the viewer side and guessing one is worse than saying so. Tests assert effects, not messages — a recording double is what let these ship inert. New: camera-absolute-rotation.test.ts (real pose), the first cameraSlice.test.ts (recording proxy over cameraCallbacks, the same probe that showed zero callbacks), resetMeshColors cases in dataSlice.test.ts (both ownership directions, plus the IFC-style-colors case), handler.effects.test.ts (bridge driven against the real slices) and ENTITY_HOVERED cases in EmbedViewer.test.ts (captured at window.parent.postMessage). Each was checked by reverting the fix. --- ...-set-camera-reset-colors-entity-hovered.md | 39 ++++ .../src/bridge/handler.effects.test.ts | 174 ++++++++++++++++++ apps/viewer-embed/src/bridge/handler.test.ts | 16 +- apps/viewer-embed/src/bridge/handler.ts | 11 +- .../src/components/EmbedViewer.test.ts | 96 ++++++++++ .../src/components/EmbedViewer.tsx | 39 ++++ .../viewer/src/components/viewer/Viewport.tsx | 9 + .../src/store/slices/cameraSlice.test.ts | 95 ++++++++++ apps/viewer/src/store/slices/cameraSlice.ts | 11 +- .../viewer/src/store/slices/dataSlice.test.ts | 92 +++++++++ apps/viewer/src/store/slices/dataSlice.ts | 87 ++++++++- apps/viewer/src/store/types.ts | 12 ++ packages/embed-protocol/src/index.ts | 10 + packages/embed-sdk/src/index.ts | 6 +- .../src/camera-absolute-rotation.test.ts | 170 +++++++++++++++++ packages/renderer/src/camera.ts | 71 +++++++ 16 files changed, 928 insertions(+), 10 deletions(-) create mode 100644 .changeset/embed-set-camera-reset-colors-entity-hovered.md create mode 100644 apps/viewer-embed/src/bridge/handler.effects.test.ts create mode 100644 apps/viewer/src/store/slices/cameraSlice.test.ts create mode 100644 packages/renderer/src/camera-absolute-rotation.test.ts diff --git a/.changeset/embed-set-camera-reset-colors-entity-hovered.md b/.changeset/embed-set-camera-reset-colors-entity-hovered.md new file mode 100644 index 0000000000..cc338edd4a --- /dev/null +++ b/.changeset/embed-set-camera-reset-colors-entity-hovered.md @@ -0,0 +1,39 @@ +--- +'@ifc-lite/renderer': minor +'@ifc-lite/embed-protocol': patch +'@ifc-lite/embed-sdk': patch +--- + +Three embed API commands that reported success while doing nothing now work +(#2934). Each was broken at a different link in the chain. + +`SET_CAMERA` had no actuator. The handler called the store's +`setCameraRotation`, which was `set({ cameraRotation })` and nothing more — +every orientation entry point on the camera was either relative (`orbit`, the +90° rotate steppers) or named a direction (`setPresetView`), so an absolute +azimuth/elevation pair had nothing to reach. The host got a `requestId` ack +*and* a `CAMERA_CHANGED` echo of its own numbers back, while the view never +moved. `Camera.setRotation(azimuth, elevation)` is new on `@ifc-lite/renderer` +— the exact inverse of `Camera.getRotation`, absolute and idempotent, keeping +the target and orbit distance, with the same pole clamp `orbit` uses — and the +store action now drives it the way `setProjectionMode` drives its own callback. + +`RESET_COLORS` cleared the wrong channel, in both directions at once. +`SET_COLORS` bakes into the mesh colors, while `clearPendingColorUpdates` +empties the transient overlay channel the lens, IDS, clash and schedule +overlays own: the host's own override survived the reset, and another +subsystem's state was destroyed by it. `SET_COLORS` now marks its writes as an +override, which captures the colors it displaces, and `RESET_COLORS` restores +those and leaves the overlay channel alone. The loader's own IFC style pass is +deliberately not treated as an override, so a reset restores the model's IFC +colors rather than stripping them. + +`ENTITY_HOVERED` was declared, exposed by the SDK, and never emitted — the SDK +tests passed because they fabricated the event themselves. The viewer's hover +pipeline was already there but gated behind a toolbar toggle the embed has no +chrome to offer; the embed now enables it and emits on each hover-target +change. + +`SET_CAMERA`'s `zoom` field remains unapplied and is now documented as +reserved rather than silently dropped: it has no defined meaning on the viewer +side, and guessing one is worse than saying so. diff --git a/apps/viewer-embed/src/bridge/handler.effects.test.ts b/apps/viewer-embed/src/bridge/handler.effects.test.ts new file mode 100644 index 0000000000..eb432628bf --- /dev/null +++ b/apps/viewer-embed/src/bridge/handler.effects.test.ts @@ -0,0 +1,174 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ + +/** + * `SET_CAMERA` / `SET_COLORS` / `RESET_COLORS` against the REAL store slices. + * + * `handler.test.ts` drives a recording double of the store, which is the right + * shape for "did the bridge dispatch the right thing" but is structurally + * blind to the failure these three commands actually had (#2934): the handler + * called a real store action, the action existed, and the action did nothing. + * A double records the call and passes. + * + * So this file wires the handler to `createDataSlice` / `createCameraSlice` + * themselves and asserts the EFFECT — the mesh color that comes back, the + * orientation that reaches the camera actuator, the overlay channel that is + * still intact afterwards. + */ + +import { beforeEach, afterEach, describe, expect, it, vi } from 'vitest'; + +// Same narrow stand-in handler.test.ts uses: the bridge needs exactly one +// function from the store barrel, and importing the real barrel would drag in +// zustand + renderer + wasm. The slice creators below are imported directly, +// so the store logic under test is the real thing. +vi.mock('@/store/index.js', () => ({ + toGlobalIdFromModels: ( + _models: ReadonlyMap, + _modelId: string, + expressId: number, + ): number => expressId, +})); + +import { EMBED_SOURCE, PROTOCOL_VERSION } from '@ifc-lite/embed-protocol'; +import { createDataSlice } from '@/store/slices/dataSlice.js'; +import { createCameraSlice } from '@/store/slices/cameraSlice.js'; +import type { CameraRotation } from '@/store/types.js'; +import { initBridge, destroyBridge } from './handler.js'; + +// --------------------------------------------------------------------------- +// Window double (postMessage in, postMessage out) +// --------------------------------------------------------------------------- + +function installWindow() { + const listeners = new Set<(e: unknown) => void>(); + const win: any = { + addEventListener: (type: string, fn: (e: unknown) => void) => { + if (type === 'message') listeners.add(fn); + }, + removeEventListener: (type: string, fn: (e: unknown) => void) => { + if (type === 'message') listeners.delete(fn); + }, + }; + win.parent = { postMessage: () => { /* replies are not the subject here */ } }; + (globalThis as any).window = win; + return { + dispatch: (data: unknown) => { + for (const fn of [...listeners]) fn({ data, origin: 'https://host.example', source: win.parent }); + }, + }; +} + +function cmd(type: string, data?: unknown) { + return { source: EMBED_SOURCE, version: PROTOCOL_VERSION, type, data, requestId: 'r1' }; +} + +// --------------------------------------------------------------------------- +// Real slices, composed the way the store composes them +// --------------------------------------------------------------------------- + +const mesh = (expressId: number, color: [number, number, number, number]) => ({ + expressId, + positions: new Float32Array([0, 0, 0, 1, 0, 0, 0, 1, 0]), + indices: new Uint32Array([0, 1, 2]), + normals: new Float32Array([0, 0, 1, 0, 0, 1, 0, 0, 1]), + color, + ifcType: 'IfcWall', +}); + +function makeRealState() { + const rotations: CameraRotation[] = []; + let state: any; + const set = (partial: any) => { + const updates = typeof partial === 'function' ? partial(state) : partial; + state = { ...state, ...updates }; + }; + const get = () => state; + + state = { + ...createDataSlice(set, get, undefined as never), + ...createCameraSlice(set, get, undefined as never), + activeModelId: null, + models: new Map(), + // Stand-in for the renderer-side actuator the Viewport registers + // (Viewport.tsx -> camera.setRotation). What it does with the angles is + // `packages/renderer/src/camera-absolute-rotation.test.ts`'s subject; what + // matters here is that the command reaches it at all. + cameraCallbacks: { + setCameraRotation: (rotation: CameraRotation) => { rotations.push(rotation); }, + }, + }; + + return { + rotations, + getState: () => state, + }; +} + +describe('bridge commands against the real store slices', () => { + let win: ReturnType; + let store: ReturnType; + + beforeEach(() => { + win = installWindow(); + store = makeRealState(); + initBridge({ + getState: store.getState as never, + loadModelFromUrl: vi.fn(), + loadModelFromBuffer: vi.fn(), + addModelFromUrl: vi.fn(), + } as never); + }); + + afterEach(() => { + destroyBridge(); + }); + + describe('SET_CAMERA', () => { + it('reaches the camera actuator, not just the store field', () => { + // The whole defect: `setCameraRotation` wrote `cameraRotation` and + // stopped there, so the host got a success ack and a CAMERA_CHANGED echo + // of its own numbers while the view never moved. + win.dispatch(cmd('SET_CAMERA', { azimuth: 120, elevation: 30 })); + + expect(store.rotations).toEqual([{ azimuth: 120, elevation: 30 }]); + }); + + it('records the new orientation in the store as well', () => { + win.dispatch(cmd('SET_CAMERA', { azimuth: 120, elevation: 30 })); + + expect(store.getState().cameraRotation).toEqual({ azimuth: 120, elevation: 30 }); + }); + }); + + describe('RESET_COLORS', () => { + it('actually restores the color SET_COLORS baked in', () => { + store.getState().appendGeometryBatch([mesh(12, [1, 0, 0, 1])] as never); + + win.dispatch(cmd('SET_COLORS', { colorMap: { '12': [0, 1, 0, 1] } })); + expect(store.getState().geometryResult.meshes[0].color).toEqual([0, 1, 0, 1]); + + win.dispatch(cmd('RESET_COLORS')); + + expect(store.getState().geometryResult.meshes[0].color).toEqual([1, 0, 0, 1]); + // And the renderer is told to re-upload the restored color, otherwise the + // GPU keeps showing the override. + expect(store.getState().pendingMeshColorUpdates.get(12)).toEqual([1, 0, 0, 1]); + }); + + it('leaves another subsystem\'s overlay colors intact', () => { + // `pendingColorUpdates` is the lens / IDS / clash / schedule overlay + // channel. RESET_COLORS used to clear exactly this and nothing else — + // wrong in both directions at once: the host's own override survived, + // and an overlay owner's state was destroyed. + store.getState().appendGeometryBatch([mesh(12, [1, 0, 0, 1])] as never); + store.getState().setPendingColorUpdates(new Map([[12, [1, 1, 0, 1]]])); + + win.dispatch(cmd('SET_COLORS', { colorMap: { '12': [0, 1, 0, 1] } })); + win.dispatch(cmd('RESET_COLORS')); + + expect(store.getState().pendingColorUpdates.get(12)).toEqual([1, 1, 0, 1]); + }); + }); +}); diff --git a/apps/viewer-embed/src/bridge/handler.test.ts b/apps/viewer-embed/src/bridge/handler.test.ts index 9520ca9fe7..491d5315dd 100644 --- a/apps/viewer-embed/src/bridge/handler.test.ts +++ b/apps/viewer-embed/src/bridge/handler.test.ts @@ -169,6 +169,7 @@ function makeState() { showAllInAllModels: rec('showAllInAllModels'), updateMeshColors: rec('updateMeshColors'), clearPendingColorUpdates: rec('clearPendingColorUpdates'), + resetMeshColors: rec('resetMeshColors'), setCameraRotation: rec('setCameraRotation'), setSectionPlaneAxis: rec('setSectionPlaneAxis'), setSectionPlanePosition: rec('setSectionPlanePosition'), @@ -814,15 +815,24 @@ describe('selection and visibility commands', () => { it('SET_COLORS converts string keys to numeric entity ids', async () => { initBridge(makeCtx(state)); await send(fw, cmd('SET_COLORS', { colorMap: { '12': [1, 0, 0, 1] } }, 'r1')); - const [updates] = argsOf(state, 'updateMeshColors') as [Map]; + const [updates, options] = argsOf(state, 'updateMeshColors') as [Map, { override?: boolean }]; expect([...updates.keys()]).toEqual([12]); expect(updates.get(12)).toEqual([1, 0, 0, 1]); + // Marked as an override so the displaced colors are captured and + // RESET_COLORS can put them back. + expect(options).toEqual({ override: true }); }); - it('RESET_COLORS clears pending updates', async () => { + it('RESET_COLORS undoes the SET_COLORS bake and leaves the overlay channel alone', async () => { initBridge(makeCtx(state)); await send(fw, cmd('RESET_COLORS', undefined, 'r1')); - expect(called(state, 'clearPendingColorUpdates')).toBe(true); + // `resetMeshColors` restores what SET_COLORS baked into + // geometryResult.meshes[].color. `clearPendingColorUpdates` is the + // lens/IDS/clash overlay channel — a different subsystem's state, which + // this command must not touch (#2934). See handler.effects.test.ts for the + // same pair asserted on the real slices rather than on this double. + expect(called(state, 'resetMeshColors')).toBe(true); + expect(called(state, 'clearPendingColorUpdates')).toBe(false); }); it('SET_TYPE_VISIBILITY toggles only the flags that actually differ', async () => { diff --git a/apps/viewer-embed/src/bridge/handler.ts b/apps/viewer-embed/src/bridge/handler.ts index 48196a0308..ac27126e1f 100644 --- a/apps/viewer-embed/src/bridge/handler.ts +++ b/apps/viewer-embed/src/bridge/handler.ts @@ -375,13 +375,20 @@ async function handleCommand(type: InboundCommandType, data: unknown, requestId? for (const [key, color] of Object.entries(payload.colorMap)) { updates.set(Number(key), color); } - state.updateMeshColors(updates); + // `override` so the displaced colors are captured and RESET_COLORS can + // put them back. + state.updateMeshColors(updates, { override: true }); if (requestId) emitToParent(createResponse(requestId)); return; } case 'RESET_COLORS': { - state.clearPendingColorUpdates(); + // Undo SET_COLORS: restore the colors it baked into + // geometryResult.meshes[].color. Deliberately NOT + // clearPendingColorUpdates() — that is the separate lens/IDS/clash/ + // schedule overlay channel, which SET_COLORS never writes to and this + // command has no claim on. + state.resetMeshColors(); if (requestId) emitToParent(createResponse(requestId)); return; } diff --git a/apps/viewer-embed/src/components/EmbedViewer.test.ts b/apps/viewer-embed/src/components/EmbedViewer.test.ts index 8dc21a880d..563c0caeed 100644 --- a/apps/viewer-embed/src/components/EmbedViewer.test.ts +++ b/apps/viewer-embed/src/components/EmbedViewer.test.ts @@ -171,3 +171,99 @@ describe('EmbedViewer: SET_SECTION emits SECTION_CHANGED to the parent', () => { }); }); }); + +/** + * ENTITY_HOVERED is a declared OutboundEventType with SDK listener plumbing + * and SDK tests — and, before #2934, zero `emitEvent('ENTITY_HOVERED', ...)` + * call sites anywhere in this app. The SDK's tests pass because they call + * `harness.emit('ENTITY_HOVERED', ...)` themselves, which proves the SDK + * dispatches an event the viewer never sent. + * + * The viewer's hover pipeline is: pointermove -> throttled `renderer.pick()` + * -> `setHoverState(...)` (apps/viewer/src/components/viewer/useMouseControls.ts + * ~703), with that whole branch gated on `hoverTooltipsEnabled`. These tests + * enter at `setHoverState` — the store action the pick path calls — and assert + * what reaches `window.parent.postMessage`, covering both remaining links: the + * gate the embed has to force on, and the emit. Driving `renderer.pick()` + * itself needs a real WebGPU device and is out of reach here. + */ +describe('EmbedViewer: emits ENTITY_HOVERED from the viewer hover pipeline', () => { + afterEach(() => { + useViewerStore.getState().clearHover(); + }); + + it('forces hoverTooltipsEnabled on, without which the pick path never runs', () => { + // Defaults to false (UI_DEFAULTS.HOVER_TOOLTIPS_ENABLED) — a main-viewer + // toolbar toggle the embed has no chrome to offer. + useViewerStore.setState({ hoverTooltipsEnabled: false }); + + renderEmbedViewer(); + + expect(useViewerStore.getState().hoverTooltipsEnabled).toBe(true); + }); + + it('posts ENTITY_HOVERED to the parent when the pick path reports a hovered entity', () => { + const posted: EmbedMessageEnvelope[] = []; + Object.defineProperty(window, 'parent', { + configurable: true, + value: { postMessage: (msg: EmbedMessageEnvelope) => posted.push(msg) }, + }); + + renderEmbedViewer(); + // emitToParent withholds every non-READY message until a concrete + // parentOrigin is captured from a real inbound message — establish that + // first, same as the SET_SECTION test above. + dispatchInbound({ type: 'SET_THEME', data: { theme: 'light' } }); + + act(() => { + // Exactly what useMouseControls does with a pick hit. + useViewerStore.getState().setHoverState({ entityId: 42, screenX: 10, screenY: 20 }); + }); + + const hovered = posted.find((m) => m.type === 'ENTITY_HOVERED'); + expect(hovered?.data).toEqual({ id: 42, globalId: undefined, ifcType: undefined }); + }); + + it('does not re-post for the same entity as the pointer drifts across it', () => { + const posted: EmbedMessageEnvelope[] = []; + Object.defineProperty(window, 'parent', { + configurable: true, + value: { postMessage: (msg: EmbedMessageEnvelope) => posted.push(msg) }, + }); + + renderEmbedViewer(); + dispatchInbound({ type: 'SET_THEME', data: { theme: 'light' } }); + + act(() => { + useViewerStore.getState().setHoverState({ entityId: 42, screenX: 10, screenY: 20 }); + }); + act(() => { + // Same entity, new screen position — every throttled mousemove within + // one mesh produces this. + useViewerStore.getState().setHoverState({ entityId: 42, screenX: 11, screenY: 21 }); + }); + + expect(posted.filter((m) => m.type === 'ENTITY_HOVERED').length).toBe(1); + }); + + it('posts again once the pointer moves onto a different entity', () => { + const posted: EmbedMessageEnvelope[] = []; + Object.defineProperty(window, 'parent', { + configurable: true, + value: { postMessage: (msg: EmbedMessageEnvelope) => posted.push(msg) }, + }); + + renderEmbedViewer(); + dispatchInbound({ type: 'SET_THEME', data: { theme: 'light' } }); + + act(() => { + useViewerStore.getState().setHoverState({ entityId: 42, screenX: 10, screenY: 20 }); + }); + act(() => { + useViewerStore.getState().setHoverState({ entityId: 43, screenX: 30, screenY: 40 }); + }); + + expect(posted.filter((m) => m.type === 'ENTITY_HOVERED').map((m) => (m.data as { id: number }).id)) + .toEqual([42, 43]); + }); +}); diff --git a/apps/viewer-embed/src/components/EmbedViewer.tsx b/apps/viewer-embed/src/components/EmbedViewer.tsx index d2b06d9925..b2c55caf26 100644 --- a/apps/viewer-embed/src/components/EmbedViewer.tsx +++ b/apps/viewer-embed/src/components/EmbedViewer.tsx @@ -46,6 +46,19 @@ export function EmbedViewer() { setTheme(urlParams.theme === 'dark' ? 'dark' : 'light'); }, [urlParams.theme, setTheme]); + // Force hover picking on. `hoverState` (apps/viewer/src/store/slices/ + // hoverSlice.ts) is populated by useMouseControls.ts's throttled + // renderer.pick() on mousemove — the same pipeline the main viewer uses — + // but that whole branch is gated behind `hoverTooltipsEnabled`, which + // defaults to false (a toolbar toggle). The embed has no toolbar to flip it, + // and the ENTITY_HOVERED effect below needs `hoverState` to ever populate. + // Safe to force on here: the embed shell never renders (it + // lives in ViewerLayout, which the embed doesn't use), so this activates the + // picking pipeline only — no tooltip UI appears. + useEffect(() => { + useViewerStore.setState({ hoverTooltipsEnabled: true }); + }, []); + // Initialize the postMessage bridge. // // Guarded via mountBridgeLifecycle/unmountBridgeLifecycle so a React 19 @@ -237,6 +250,32 @@ export function EmbedViewer() { } }, [selectedEntityId]); + // Emit hover events to parent. ENTITY_HOVERED is declared in the protocol + // and exposed by the SDK, but nothing in this app ever emitted it — the + // SDK's tests pass because they fabricate the event themselves (#2934). + // + // Subscribes to `hoverState.entityId` specifically, not the whole + // `hoverState` object: screenX/screenY/worldXYZ change on every + // hover-throttled mousemove even while the pointer stays on the same mesh, + // so selecting the object would re-post the event continuously instead of + // only on a hover-target change. The protocol declares no ENTITY_UNHOVERED + // counterpart to ENTITY_DESELECTED, so null (nothing hovered) is tracked but + // never emitted. + const hoveredEntityId = useViewerStore((s) => s.hoverState.entityId); + useEffect(() => { + if (hoveredEntityId === null) return; + + const state = useViewerStore.getState(); + const lookup = state.resolveGlobalIdFromModels(hoveredEntityId); + const model = lookup ? state.models.get(lookup.modelId) : undefined; + const entities = model?.ifcDataStore?.entities; + emitEvent('ENTITY_HOVERED', { + id: hoveredEntityId, + globalId: entities?.getGlobalId(lookup?.expressId ?? hoveredEntityId) ?? undefined, + ifcType: entities?.getTypeName(lookup?.expressId ?? hoveredEntityId) ?? undefined, + }); + }, [hoveredEntityId]); + // Emit camera rotation changes to parent (throttled) const cameraRotation = useViewerStore((s) => s.cameraRotation); const lastCameraEmit = useRef(0); diff --git a/apps/viewer/src/components/viewer/Viewport.tsx b/apps/viewer/src/components/viewer/Viewport.tsx index 65aa0c957b..7bfda84949 100644 --- a/apps/viewer/src/components/viewer/Viewport.tsx +++ b/apps/viewer/src/components/viewer/Viewport.tsx @@ -1049,6 +1049,15 @@ export function Viewport({ renderCurrent(); calculateScale(); }, + setCameraRotation: ({ azimuth, elevation }) => { + // Absolute counterpart to rotateLeft/rotateRight below (which step by + // 90° from wherever the camera already is). Snaps rather than + // animates: the caller is a host command that may arrive at slider + // rate, and a tween per message would queue up behind itself. + camera.setRotation(azimuth, elevation); + renderCurrent(); + calculateScale(); + }, rotateLeft: () => { animateHorizontalRotation(-Math.PI / 2); }, diff --git a/apps/viewer/src/store/slices/cameraSlice.test.ts b/apps/viewer/src/store/slices/cameraSlice.test.ts new file mode 100644 index 0000000000..36cf26a767 --- /dev/null +++ b/apps/viewer/src/store/slices/cameraSlice.test.ts @@ -0,0 +1,95 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ + +/** + * `setCameraRotation` must ACTUATE, not just record. + * + * It used to be `set({ cameraRotation })` and nothing else, while its one + * caller — the embed bridge's `SET_CAMERA` handler — acked the command as + * successful and the outbound `CAMERA_CHANGED` echoed the host's own numbers + * straight back. Every success signal, no camera movement (#2934). The + * recording proxy over `cameraCallbacks` below is the same probe that showed + * zero callbacks being invoked; it now pins the opposite. + * + * `setProjectionMode` in this same slice is the established shape: drive the + * callback, then store the state. + */ + +import { describe, it, beforeEach } from 'node:test'; +import assert from 'node:assert'; +import { createCameraSlice, type CameraSlice } from './cameraSlice.js'; +import { CAMERA_DEFAULTS } from '../constants.js'; + +describe('cameraSlice', () => { + let state: CameraSlice; + let calls: Array<[string, unknown]>; + + function build(withCallbacks: boolean): void { + calls = []; + const set = ( + partial: Partial | ((s: CameraSlice) => Partial), + ) => { + const updates = typeof partial === 'function' ? partial(state) : partial; + state = { ...state, ...updates }; + }; + const get = () => state; + state = createCameraSlice(set as never, get as never, undefined as never); + if (withCallbacks) { + state = { + ...state, + // Recording proxy over the callback surface the Viewport registers. + cameraCallbacks: { + setCameraRotation: (rotation) => calls.push(['setCameraRotation', rotation]), + setProjectionMode: (mode) => calls.push(['setProjectionMode', mode]), + }, + }; + } + } + + beforeEach(() => build(true)); + + describe('setCameraRotation', () => { + it('drives the renderer through cameraCallbacks.setCameraRotation', () => { + state.setCameraRotation({ azimuth: 120, elevation: 30 }); + + assert.deepStrictEqual(calls, [['setCameraRotation', { azimuth: 120, elevation: 30 }]]); + }); + + it('still records the rotation in the store', () => { + state.setCameraRotation({ azimuth: 120, elevation: 30 }); + + assert.deepStrictEqual(state.cameraRotation, { azimuth: 120, elevation: 30 }); + }); + + it('records the rotation even when no renderer has registered yet', () => { + // cameraCallbacks is `{}` until the Viewport mounts; the store write must + // not depend on the actuator existing. + build(false); + + state.setCameraRotation({ azimuth: 15, elevation: 5 }); + + assert.deepStrictEqual(state.cameraRotation, { azimuth: 15, elevation: 5 }); + assert.deepStrictEqual(calls, []); + }); + + it('starts from the shared camera defaults', () => { + assert.deepStrictEqual(state.cameraRotation, { + azimuth: CAMERA_DEFAULTS.AZIMUTH, + elevation: CAMERA_DEFAULTS.ELEVATION, + }); + }); + }); + + describe('updateCameraRotationRealtime', () => { + it('stays off the actuator — the per-frame path reports, it does not command', () => { + // This is the callback the live navigation loop drives on every frame. + // Routing it through the actuator would fight the very gesture that + // produced it. + state.setOnCameraRotationChange(() => {}); + state.updateCameraRotationRealtime({ azimuth: 200, elevation: 10 }); + + assert.deepStrictEqual(calls, []); + }); + }); +}); diff --git a/apps/viewer/src/store/slices/cameraSlice.ts b/apps/viewer/src/store/slices/cameraSlice.ts index 345b0c8dac..c3bbd13a03 100644 --- a/apps/viewer/src/store/slices/cameraSlice.ts +++ b/apps/viewer/src/store/slices/cameraSlice.ts @@ -41,7 +41,16 @@ export const createCameraSlice: StateCreator = onScaleChange: null, // Actions - setCameraRotation: (cameraRotation) => set({ cameraRotation }), + // Drive the renderer FIRST, then record — the same shape as + // setProjectionMode below. Recording alone is what made the embed API's + // SET_CAMERA inert: the store field was written, `CAMERA_CHANGED` echoed it + // back to the host as confirmation, and the camera never moved (#2934). + // This is the absolute-orientation path only; live navigation reports + // through `updateCameraRotationRealtime`, which must NOT actuate. + setCameraRotation: (cameraRotation) => { + get().cameraCallbacks.setCameraRotation?.(cameraRotation); + set({ cameraRotation }); + }, setCameraCallbacks: (cameraCallbacks) => set({ cameraCallbacks }), setProjectionMode: (projectionMode) => { get().cameraCallbacks.setProjectionMode?.(projectionMode); diff --git a/apps/viewer/src/store/slices/dataSlice.test.ts b/apps/viewer/src/store/slices/dataSlice.test.ts index 6832a65c3c..44e3c02b8c 100644 --- a/apps/viewer/src/store/slices/dataSlice.test.ts +++ b/apps/viewer/src/store/slices/dataSlice.test.ts @@ -197,6 +197,98 @@ describe('DataSlice', () => { }); }); + /** + * `resetMeshColors` is what the embed API's `RESET_COLORS` undoes + * `SET_COLORS` with (#2934). It used to call `clearPendingColorUpdates`, + * which is a DIFFERENT channel: `SET_COLORS` bakes into + * `geometryResult.meshes[].color`, while `pendingColorUpdates` belongs to the + * lens / IDS / clash / schedule overlays. So the reset both failed to undo + * the override and destroyed a claim it did not own — the two directions + * asserted separately below. + */ + describe('resetMeshColors', () => { + it('restores the pre-override mesh color and re-queues it for the renderer', () => { + const mesh = createMockMesh(1, [1, 0, 0, 1]); // original: red + state.appendGeometryBatch([mesh] as any); + + const updates = new Map([[1, [0, 1, 0, 1]]]); + state.updateMeshColors(updates, { override: true }); // SET_COLORS: green + assert.deepStrictEqual(state.geometryResult?.meshes[0].color, [0, 1, 0, 1]); + + state.resetMeshColors(); + + assert.deepStrictEqual(state.geometryResult?.meshes[0].color, [1, 0, 0, 1]); + assert.deepStrictEqual(state.pendingMeshColorUpdates?.get(1), [1, 0, 0, 1]); + assert.strictEqual(state.meshColorBackup, null); + }); + + it('restores the ORIGINAL color across repeated overrides, not the last one', () => { + const mesh = createMockMesh(1, [1, 0, 0, 1]); // original: red + state.appendGeometryBatch([mesh] as any); + + state.updateMeshColors(new Map([[1, [0, 1, 0, 1] as [number, number, number, number]]]), { override: true }); + state.updateMeshColors(new Map([[1, [0, 0, 1, 1] as [number, number, number, number]]]), { override: true }); + + state.resetMeshColors(); + + assert.deepStrictEqual(state.geometryResult?.meshes[0].color, [1, 0, 0, 1]); + }); + + it('leaves the loader\'s IFC style colors alone — they are the model, not an override', () => { + // The deferred style/material pass in useIfcLoader goes through + // updateMeshColors WITHOUT `override`. If it were backed up, a host's + // RESET_COLORS would strip the model's own IFC colors back to the + // pre-style default. + const mesh = createMockMesh(1, [0.5, 0.5, 0.5, 1]); // pre-style default + state.appendGeometryBatch([mesh] as any); + + state.updateMeshColors(new Map([[1, [0.8, 0.6, 0.4, 1] as [number, number, number, number]]])); + + state.resetMeshColors(); + + assert.deepStrictEqual(state.geometryResult?.meshes[0].color, [0.8, 0.6, 0.4, 1]); + assert.strictEqual(state.meshColorBackup, null); + }); + + it('restores to the IFC style color, not to the pre-style default', () => { + const mesh = createMockMesh(1, [0.5, 0.5, 0.5, 1]); + state.appendGeometryBatch([mesh] as any); + // Load-time style pass, then a host override on top of it. + state.updateMeshColors(new Map([[1, [0.8, 0.6, 0.4, 1] as [number, number, number, number]]])); + state.updateMeshColors(new Map([[1, [1, 0, 0, 1] as [number, number, number, number]]]), { override: true }); + + state.resetMeshColors(); + + assert.deepStrictEqual(state.geometryResult?.meshes[0].color, [0.8, 0.6, 0.4, 1]); + }); + + it('does not touch pendingColorUpdates — the lens/IDS/clash overlay channel', () => { + const mesh = createMockMesh(1, [1, 0, 0, 1]); + state.appendGeometryBatch([mesh] as any); + + // Another subsystem's claim on the overlay channel. + state.setPendingColorUpdates(new Map([[1, [1, 1, 0, 1] as [number, number, number, number]]])); + state.updateMeshColors(new Map([[1, [0, 1, 0, 1] as [number, number, number, number]]]), { override: true }); + + state.resetMeshColors(); + + assert.deepStrictEqual(state.pendingColorUpdates?.get(1), [1, 1, 0, 1]); + }); + + it('is a no-op when nothing was ever overridden', () => { + const mesh = createMockMesh(1, [1, 0, 0, 1]); + state.appendGeometryBatch([mesh] as any); + // A load-time style bake still queued for the renderer must survive: a + // reset that clears it would drop the model's colors mid-load. + state.updateMeshColors(new Map([[1, [0.8, 0.6, 0.4, 1] as [number, number, number, number]]])); + + state.resetMeshColors(); + + assert.deepStrictEqual(state.geometryResult?.meshes[0].color, [0.8, 0.6, 0.4, 1]); + assert.deepStrictEqual(state.pendingMeshColorUpdates?.get(1), [0.8, 0.6, 0.4, 1]); + }); + }); + describe('setPendingColorUpdates', () => { it('should clone pending color updates map', () => { const updates = new Map(); diff --git a/apps/viewer/src/store/slices/dataSlice.ts b/apps/viewer/src/store/slices/dataSlice.ts index e1f8b15495..35579d6ccc 100644 --- a/apps/viewer/src/store/slices/dataSlice.ts +++ b/apps/viewer/src/store/slices/dataSlice.ts @@ -43,6 +43,18 @@ export interface DataSlice { pendingColorUpdates: Map | null; /** Persistent mesh color updates (IFC deferred style/material colors). */ pendingMeshColorUpdates: Map | null; + /** + * Pre-override colors for every entity an *overriding* `updateMeshColors` + * call has baked over, keyed by expressId — what `resetMeshColors` restores. + * First write per entity wins, so successive overrides never clobber the + * ORIGINAL color with an intermediate one. Null when nothing is overridden. + * + * Only `updateMeshColors(updates, { override: true })` records here. The + * loader's own deferred IFC style/material pass goes through the same action + * WITHOUT that flag, precisely so a later reset restores the model's IFC + * colors rather than stripping them back to the pre-style defaults. + */ + meshColorBackup: Map | null; // Actions setIfcDataStore: (result: IfcDataStore | null) => void; @@ -53,8 +65,21 @@ export interface DataSlice { * `geometryContentVersion` for why this is separate from setGeometryResult. */ bumpGeometryContentVersion: () => void; releaseGeometryMemory: () => void; - /** Persist mesh color changes in geometryResult (used for IFC style/material updates). */ - updateMeshColors: (updates: Map) => void; + /** + * Persist mesh color changes in geometryResult (used for IFC style/material + * updates). + * + * Pass `{ override: true }` when the colors are a *temporary* override on top + * of whatever the mesh already shows (the embed API's `SET_COLORS`): the + * displaced colors are captured in `meshColorBackup` so `resetMeshColors` + * can put them back. The loader's IFC style pass deliberately omits it — the + * colors it writes ARE the model's colors, and backing them up would make a + * later reset strip the model's styling. + */ + updateMeshColors: ( + updates: Map, + options?: { override?: boolean }, + ) => void; /** * Pending mesh removals for the renderer. Authoring actions * (split, delete) push globalIds here; `useGeometryStreaming` @@ -115,6 +140,18 @@ export interface DataSlice { setPendingColorUpdates: (updates: Map) => void; clearPendingColorUpdates: () => void; clearPendingMeshColorUpdates: () => void; + /** + * Undo every overriding `updateMeshColors` bake since the backup was last + * empty: restores `geometryResult.meshes[].color` from `meshColorBackup`, + * re-queues those restored colors on `pendingMeshColorUpdates` so the + * renderer re-uploads them, and clears the backup. + * + * Deliberately does NOT touch `pendingColorUpdates`. That is a different + * channel with different owners — the lens, IDS, clash and schedule overlays + * each install and release their own state there — and clearing it from here + * would destroy a claim this subsystem never made. + */ + resetMeshColors: () => void; updateCoordinateInfo: (coordinateInfo: CoordinateInfo) => void; } @@ -145,6 +182,7 @@ export const createDataSlice: StateCreator set((state) => { + updateMeshColors: (updates, options) => set((state) => { // Clone the Map to prevent external mutation const clonedUpdates = new Map(updates); @@ -286,11 +324,22 @@ export const createDataSlice: StateCreator { const newColor = clonedUpdates.get(mesh.expressId); if (newColor) { + if (meshColorBackup && !meshColorBackup.has(mesh.expressId)) { + meshColorBackup.set(mesh.expressId, mesh.color); + } return { ...mesh, color: newColor }; } return mesh; @@ -301,6 +350,7 @@ export const createDataSlice: StateCreator set({ pendingMeshColorUpdates: null }), + resetMeshColors: () => set((state) => { + const backup = state.meshColorBackup; + if (!backup || backup.size === 0) { + // Nothing was overridden — and nothing to clear either. Leaving + // `pendingMeshColorUpdates` alone matters: the loader's deferred IFC + // style pass queues there, and a reset arriving mid-load must not drop + // the model's own colors before the renderer has drained them. + return {}; + } + + if (!state.geometryResult) { + // Federation mode: no local geometryResult to restore colors on; still + // forward the restore to the renderer's pending queue. + return { pendingMeshColorUpdates: new Map(backup), meshColorBackup: null }; + } + + const restoredMeshes = state.geometryResult.meshes.map(mesh => { + const original = backup.get(mesh.expressId); + return original ? { ...mesh, color: original } : mesh; + }); + + return { + geometryResult: { + ...state.geometryResult, + meshes: restoredMeshes, + }, + pendingMeshColorUpdates: new Map(backup), + meshColorBackup: null, + }; + }), + setPendingMeshRemovals: (ids) => set((state) => { // Accumulate across calls — the streaming loop drains in one // pass per frame, but split / delete actions may fire several diff --git a/apps/viewer/src/store/types.ts b/apps/viewer/src/store/types.ts index bc8c2deefd..1da945cbd3 100644 --- a/apps/viewer/src/store/types.ts +++ b/apps/viewer/src/store/types.ts @@ -390,6 +390,18 @@ export interface CameraCallbacks { */ frameClashRegion?: (min: { x: number; y: number; z: number }, max: { x: number; y: number; z: number }) => void; orbit?: (deltaX: number, deltaY: number) => void; + /** + * Place the camera at an ABSOLUTE orientation (degrees, same convention as + * `cameraRotation` and the renderer's `Camera.getRotation`), keeping the + * current target and orbit distance. + * + * Every other orientation callback here is relative (`orbit`, `rotateLeft`, + * `rotateRight`) or names a direction (`setPresetView`), so a caller holding + * an angle pair — the embed API's `SET_CAMERA` — had nothing to call and the + * store write went nowhere (#2934). Driven from `setCameraRotation` in + * cameraSlice, mirroring how `setProjectionMode` drives its own callback. + */ + setCameraRotation?: (rotation: CameraRotation) => void; projectToScreen?: (worldPos: { x: number; y: number; z: number }) => { x: number; y: number } | null; /** * Unproject a screen pixel onto the horizontal plane at the diff --git a/packages/embed-protocol/src/index.ts b/packages/embed-protocol/src/index.ts index 81bef1750c..5aaf1a13fe 100644 --- a/packages/embed-protocol/src/index.ts +++ b/packages/embed-protocol/src/index.ts @@ -93,6 +93,16 @@ export interface InboundPayloads { SET_COLORS: { colorMap: Record }; RESET_COLORS: void; FIT_TO_VIEW: { ids?: number[] }; + /** + * Absolute camera orientation in degrees: `azimuth` horizontal (normalized + * into 0-360), `elevation` from the horizon (clamped just inside ±90°, where + * the view matrix degenerates). The target and the orbit distance are kept — + * this rotates the camera, it does not reframe; use `FIT_TO_VIEW` for that. + * + * `zoom` is NOT applied. It has no defined meaning on this side (factor? + * distance? relative to what?), and the viewer deliberately ignores it rather + * than guessing — see #2934. Treat it as reserved. + */ SET_CAMERA: { azimuth: number; elevation: number; zoom?: number }; SET_VIEW: { preset: ViewPreset }; SET_SECTION: { axis?: SectionAxis; position?: number; enabled?: boolean; flipped?: boolean }; diff --git a/packages/embed-sdk/src/index.ts b/packages/embed-sdk/src/index.ts index 6d57efd237..eb4cf6ce96 100644 --- a/packages/embed-sdk/src/index.ts +++ b/packages/embed-sdk/src/index.ts @@ -254,7 +254,11 @@ export class IFCLiteEmbed { return this.request('FIT_TO_VIEW', { ids }) as Promise; } - /** Set camera orientation */ + /** + * Set the camera's absolute orientation, in degrees, around whatever it is + * currently looking at. `zoom` is reserved and ignored by the viewer — see + * `InboundPayloads['SET_CAMERA']`. + */ setCamera(azimuth: number, elevation: number, zoom?: number): Promise { return this.request('SET_CAMERA', { azimuth, elevation, zoom }) as Promise; } diff --git a/packages/renderer/src/camera-absolute-rotation.test.ts b/packages/renderer/src/camera-absolute-rotation.test.ts new file mode 100644 index 0000000000..f8be971625 --- /dev/null +++ b/packages/renderer/src/camera-absolute-rotation.test.ts @@ -0,0 +1,170 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ + +/** + * `Camera.setRotation` — the ABSOLUTE orientation actuator. + * + * Every other orientation entry point on the camera is either relative + * (`orbit`, the viewer's 90° `rotateLeft`/`rotateRight` steppers) or names a + * direction rather than an angle (`setPresetView`). A host that says "put the + * camera at azimuth 120°, elevation 30°" — the embed API's `SET_CAMERA`, which + * had no actuator at all and only wrote a store field (#2934) — needs this one. + * + * The pinned contract is the round trip against `getRotation`, because that is + * what makes the command observable to the caller: the angles the camera + * reports back must be the angles that were asked for, and the pose must + * actually have moved. + */ + +import { describe, it } from 'node:test'; +import assert from 'node:assert'; + +import { Camera } from './camera.js'; +import { CAMERA_CONSTANTS as CC } from './constants.js'; +import type { Vec3 } from './types.js'; + +function len(v: Vec3): number { + return Math.sqrt(v.x * v.x + v.y * v.y + v.z * v.z); +} + +/** + * Drive a camera animation on a fake clock. Same helper shape as + * `camera-malformed-bounds-fit.test.ts` / `camera-preset-orbit.test.ts`: the + * animator's completion promise chains off `requestAnimationFrame`, which does + * not exist under `node:test`, so the animation is stepped by hand and the + * promise is never awaited. + */ +function withStubbedFrameClock(run: (advance: (ms: number) => void) => void): void { + const originalNow = Date.now; + const hadRaf = Object.prototype.hasOwnProperty.call(globalThis, 'requestAnimationFrame'); + const originalRaf = Reflect.get(globalThis, 'requestAnimationFrame'); + let now = 1_000; + + Date.now = () => now; + Reflect.set(globalThis, 'requestAnimationFrame', () => 0); + + try { + run((ms) => { now += ms; }); + } finally { + Date.now = originalNow; + if (hadRaf) { + Reflect.set(globalThis, 'requestAnimationFrame', originalRaf); + } else { + Reflect.deleteProperty(globalThis, 'requestAnimationFrame'); + } + } +} + +function distanceOf(camera: Camera): number { + const p = camera.getPosition(); + const t = camera.getTarget(); + return len({ x: p.x - t.x, y: p.y - t.y, z: p.z - t.z }); +} + +describe('Camera.setRotation (absolute orientation)', () => { + it('reports back exactly the angles it was given', () => { + const camera = new Camera(); + for (const [azimuth, elevation] of [[0, 0], [90, 30], [217.5, -42], [359, 12]] as const) { + camera.setRotation(azimuth, elevation); + const got = camera.getRotation(); + assert.ok(Math.abs(got.azimuth - azimuth) < 1e-6, `azimuth ${got.azimuth} != ${azimuth}`); + assert.ok(Math.abs(got.elevation - elevation) < 1e-6, `elevation ${got.elevation} != ${elevation}`); + } + }); + + it('actually moves the camera position', () => { + const camera = new Camera(); + const before = camera.getPosition(); + camera.setRotation(180, 45); + const after = camera.getPosition(); + assert.ok( + Math.abs(before.x - after.x) + Math.abs(before.y - after.y) + Math.abs(before.z - after.z) > 1e-3, + 'setRotation left the camera position untouched', + ); + }); + + it('is idempotent — repeating the same angles does not walk the camera', () => { + // The command is ABSOLUTE, so a host that re-sends its current orientation + // (a slider that fires on every frame) must not drift. A relative + // implementation would accumulate; only float noise from re-deriving the + // radius is allowed here. + const camera = new Camera(); + camera.setRotation(120, 25); + const first = camera.getPosition(); + for (let i = 0; i < 20; i++) camera.setRotation(120, 25); + const last = camera.getPosition(); + const drift = Math.abs(last.x - first.x) + Math.abs(last.y - first.y) + Math.abs(last.z - first.z); + assert.ok(drift < 1e-9, `drifted by ${drift} over 20 identical calls`); + }); + + it('preserves the orbit distance and the target', () => { + const camera = new Camera(); + camera.setTarget(10, 20, 30); + camera.setPosition(10, 20, 130); // distance 100 from the target + const distanceBefore = distanceOf(camera); + + camera.setRotation(75, -15); + + assert.ok(Math.abs(distanceOf(camera) - distanceBefore) < 1e-6, 'distance changed'); + assert.deepStrictEqual(camera.getTarget(), { x: 10, y: 20, z: 30 }); + }); + + it('normalizes an out-of-range azimuth into 0-360', () => { + const camera = new Camera(); + camera.setRotation(-90, 0); + assert.ok(Math.abs(camera.getRotation().azimuth - 270) < 1e-6); + }); + + it('clamps elevation just off the poles so the view matrix cannot degenerate', () => { + const camera = new Camera(); + // ±90° is the spherical singularity: `cross(forward, up)` collapses and the + // model flips. MIN_PHI is the same margin `orbit` clamps to. + const maxElevation = 90 - (CC.MIN_PHI * 180) / Math.PI; + camera.setRotation(0, 90); + assert.ok(Math.abs(camera.getRotation().elevation - maxElevation) < 1e-6); + camera.setRotation(0, -90); + assert.ok(Math.abs(camera.getRotation().elevation + maxElevation) < 1e-6); + }); + + it('supersedes an animation already in flight instead of being erased by it', () => { + // A host that sends SET_VIEW (which animates) and then SET_CAMERA must end + // up where SET_CAMERA asked. Without cancelling the tween, the very next + // `update()` writes the animation's interpolated pose over this one. + withStubbedFrameClock((advance) => { + const camera = new Camera(); + // setPresetView starts a tween that `update()` applies frame by frame. + camera.setPresetView('top', { min: { x: -10, y: -10, z: -10 }, max: { x: 10, y: 10, z: 10 } }); + + camera.setRotation(200, 20); + advance(16); + camera.update(16); + + const got = camera.getRotation(); + assert.ok(Math.abs(got.azimuth - 200) < 1e-6, `azimuth drifted to ${got.azimuth}`); + assert.ok(Math.abs(got.elevation - 20) < 1e-6, `elevation drifted to ${got.elevation}`); + }); + }); + + it('rejects non-finite angles instead of writing a NaN pose', () => { + const camera = new Camera(); + camera.setRotation(30, 10); + const pose = camera.getPosition(); + camera.setRotation(NaN, 10); + camera.setRotation(30, Infinity); + assert.deepStrictEqual(camera.getPosition(), pose); + }); + + it('recovers from a degenerate pose rather than propagating it', () => { + const camera = new Camera(); + // position === target: distance 0, so there is no orbit radius to preserve. + camera.setTarget(5, 5, 5); + camera.setPosition(5, 5, 5); + camera.setRotation(45, 20); + const got = camera.getRotation(); + assert.ok(Number.isFinite(got.azimuth) && Number.isFinite(got.elevation)); + assert.ok(distanceOf(camera) > 0, 'still degenerate after setRotation'); + assert.ok(Math.abs(got.azimuth - 45) < 1e-6); + assert.ok(Math.abs(got.elevation - 20) < 1e-6); + }); +}); diff --git a/packages/renderer/src/camera.ts b/packages/renderer/src/camera.ts index 309635c24e..e8da2e5736 100644 --- a/packages/renderer/src/camera.ts +++ b/packages/renderer/src/camera.ts @@ -20,11 +20,13 @@ import { FirstPersonNavigator } from './camera-first-person.js'; import { updateCameraMatrices } from './camera-matrices.js'; import { pickFitPolicy, type Bounds3, type FitPolicy, type PickFitPolicyOptions } from './camera-fit-policy.js'; import { + areFiniteNumbers, DEFAULT_ORTHO_SIZE, isUsableBounds, isUsableDistance, usableOrthoSize, } from './camera-guards.js'; +import { CAMERA_CONSTANTS } from './constants.js'; export class Camera { private state: CameraInternalState; @@ -458,6 +460,75 @@ export class Camera { return { azimuth, elevation }; } + /** + * Place the camera at an ABSOLUTE orientation around its current target, + * in the same angle convention {@link getRotation} reports — the exact + * inverse of it, so `setRotation(a, e)` then `getRotation()` returns + * `{ azimuth: a, elevation: e }` (modulo the normalisation and pole clamp + * below). + * + * This is the only absolute-orientation entry point on the camera. Everything + * else is relative (`orbit`, and the viewer's 90° rotate steppers built on it) + * or names a direction rather than an angle (`setPresetView`), which is why a + * host command that says "go to azimuth 120°, elevation 30°" had nothing to + * call and silently did nothing (#2934). + * + * The orbit radius and the target are preserved — this rotates the camera on + * its current sphere, it does not reframe. `up` is reset to world Y, matching + * `orbit`, so the reported azimuth comes back through `getRotation`'s + * position-based branch. + * + * @param azimuth Horizontal angle in degrees; normalised into [0, 360). + * @param elevation Vertical angle in degrees, 0 = horizon. Clamped to just + * inside ±90° (the same `MIN_PHI` margin `orbit` uses) — the exact poles + * collapse `cross(forward, up)` and flip the model. + */ + setRotation(azimuth: number, elevation: number): void { + // Angles are an input class of their own, and both of them reach the + // trigonometry below unguarded: a non-finite one writes a NaN position and + // destroys an otherwise valid pose. Same rejection `orbit` applies to its + // deltas — a rejected call changes nothing at all. + if (!areFiniteNumbers(azimuth, elevation)) return; + + // An in-flight tween or leftover inertia writes position/target on the next + // `update()` and would erase this pose a frame later — a host that sends + // SET_VIEW (animated) and then SET_CAMERA would end up at the preset. An + // absolute placement supersedes whatever motion is still running, so cancel + // it; this also drops the preset-view rotation cycle, which is correct + // after the camera has been reoriented out from under it. + this.animator.reset(); + + const target = this.state.camera.target; + const dir = { + x: this.state.camera.position.x - target.x, + y: this.state.camera.position.y - target.y, + z: this.state.camera.position.z - target.z, + }; + const current = Math.sqrt(dir.x * dir.x + dir.y * dir.y + dir.z * dir.z); + // A degenerate pose (position === target, or a non-finite one) has no orbit + // radius to preserve. Any positive radius yields a well-formed view matrix + // at the requested direction, which is strictly better than propagating the + // degeneracy — and leaves the caller's angles observable, which is the + // whole point of the command. + const distance = isUsableDistance(current, 1e-6) ? current : 1; + + const theta = ((((azimuth % 360) + 360) % 360) * Math.PI) / 180; + const poleMargin = CAMERA_CONSTANTS.MIN_PHI; + const phi = Math.max( + poleMargin, + Math.min(Math.PI - poleMargin, ((90 - elevation) * Math.PI) / 180), + ); + const sinPhi = Math.sin(phi); + + this.state.camera.position = { + x: target.x + distance * sinPhi * Math.sin(theta), + y: target.y + distance * Math.cos(phi), + z: target.z + distance * sinPhi * Math.cos(theta), + }; + this.state.camera.up = { x: 0, y: 1, z: 0 }; + this.updateMatrices(); + } + /** * Unproject screen coordinates to a ray in world space * @param screenX - X position in screen coordinates From 7aa31ef98e9dd84dc2853bb21cbf6e77d167dcbc Mon Sep 17 00:00:00 2001 From: Petru Conduraru Date: Sat, 22 Aug 2026 13:23:52 +0300 Subject: [PATCH 2/3] test(renderer): observe setRotation's up reset from a non-Y up pose MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every case in camera-absolute-rotation.test.ts started from a camera whose `up` was already world Y — the one state in which the reset at the end of `setRotation` cannot be observed. Verified by mutation: deleting `this.state.camera.up = { x: 0, y: 1, z: 0 }` left all nine tests green. It matters because `getRotation` derives azimuth from the UP vector whenever it has a horizontal component (`upLen > 0.01`) and only falls back to the position when up is vertical. A camera restored from a BCF viewpoint takes its up straight from the file (Viewport.tsx:926, `camera.setUp(viewpoint.up…)`), so a top-down viewpoint arrives with up = (0, 0, -1). Measured with the reset removed: `setRotation(120, 30)` writes the right position but `getRotation` reports azimuth 0 — the same "the command did nothing" symptom as #2934, one layer down. The new case sets that pose up explicitly, asserts the precondition (stale up reports azimuth 0), then pins the round trip and the re-seated up. The deletion mutation now fails. --- .../src/camera-absolute-rotation.test.ts | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/packages/renderer/src/camera-absolute-rotation.test.ts b/packages/renderer/src/camera-absolute-rotation.test.ts index f8be971625..91ca1383d0 100644 --- a/packages/renderer/src/camera-absolute-rotation.test.ts +++ b/packages/renderer/src/camera-absolute-rotation.test.ts @@ -146,6 +146,41 @@ describe('Camera.setRotation (absolute orientation)', () => { }); }); + it('re-seats a non-Y up vector, so the angles are observable from any prior pose', () => { + // Every other case here starts from a camera whose `up` is already world + // Y, which is the one state where the reset at the end of `setRotation` + // cannot be observed -- verified by mutation: deleting + // `this.state.camera.up = { x: 0, y: 1, z: 0 }` left the whole file green. + // + // `getRotation` derives azimuth from the UP vector whenever it has any + // horizontal component (`upLen > 0.01`), and only falls back to the + // position when up is vertical. A camera restored from a BCF viewpoint + // takes its up straight from the file (`Viewport.tsx`'s + // `camera.setUp(viewpoint.up...)`), so a top-down viewpoint arrives with + // up = (0, 0, -1). Without the reset the new position is written but the + // reported azimuth still comes from the stale up vector -- 0 instead of + // the 120 that was asked for, i.e. exactly the "the command did nothing" + // symptom of #2934, one layer down. + const camera = new Camera(); + camera.setTarget(0, 0, 0); + camera.setPosition(0, 100, 0); + camera.setUp(0, 0, -1); + assert.ok( + Math.abs(camera.getRotation().azimuth - 0) < 1e-6, + 'fixture precondition: the stale up vector reports azimuth 0', + ); + + camera.setRotation(120, 30); + + const got = camera.getRotation(); + assert.ok( + Math.abs(got.azimuth - 120) < 1e-6, + `azimuth ${got.azimuth} != 120 -- the stale up vector still drives the readout`, + ); + assert.ok(Math.abs(got.elevation - 30) < 1e-6, `elevation ${got.elevation} != 30`); + assert.deepStrictEqual(camera.getUp(), { x: 0, y: 1, z: 0 }); + }); + it('rejects non-finite angles instead of writing a NaN pose', () => { const camera = new Camera(); camera.setRotation(30, 10); From 57fec0f27ba151d578139308dbbfadbd253b24bf Mon Sep 17 00:00:00 2001 From: Petru Conduraru Date: Sat, 22 Aug 2026 15:08:29 +0300 Subject: [PATCH 3/3] docs(changeset): state RESET_COLORS' removed side effect, and its federation limit Two things an integrator reading the changeset would not learn. RESET_COLORS no longer clears pendingColorUpdates. The change is right -- SET_COLORS never wrote that channel -- but a host that had been sending RESET_COLORS to clear a lens, IDS, clash or schedule overlay was relying on that side effect, and it is gone. That is a behaviour change on a published surface, not only a fix. RESET_COLORS restores only the entities in the viewer's primary geometryResult, which addModel (modelSlice.ts) sets from the FIRST model only. In a federated embed both commands ack success and the later models stay recoloured. Verified by running, against the real slice via createDataSlice: backup after SET : [[1,[1,0,0,1]]] <- 999 absent pending after SET : [[1,[0,1,0,1]],[999,[0,1,0,1]]] <- 999 IS sent pending after RST : [[1,[1,0,0,1]]] <- 999 never restored And with geometryResult null, updateMeshColors returns before the backup capture, so meshColorBackup stays null and resetMeshColors is a silent no-op: NULL: pending after SET : [[1,[0,1,0,1]]] NULL: backup after SET : null NULL: pending after RST : [[1,[0,1,0,1]]] Documenting, not fixing: a real fix has to decide whether the backup key space becomes model-scoped (expressIds are per-model and the federation registry maps them to globalIds) or whether the command refuses what it cannot back up instead of acking it. That is a design call, not a patch. --- .../embed-set-camera-reset-colors-entity-hovered.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/.changeset/embed-set-camera-reset-colors-entity-hovered.md b/.changeset/embed-set-camera-reset-colors-entity-hovered.md index cc338edd4a..d7680f79c5 100644 --- a/.changeset/embed-set-camera-reset-colors-entity-hovered.md +++ b/.changeset/embed-set-camera-reset-colors-entity-hovered.md @@ -28,6 +28,19 @@ those and leaves the overlay channel alone. The loader's own IFC style pass is deliberately not treated as an override, so a reset restores the model's IFC colors rather than stripping them. +For integrators, that second half is a behaviour change on a published surface +and not only a fix: `RESET_COLORS` no longer clears `pendingColorUpdates`. A +host that had been sending it to clear a lens, IDS, clash or schedule overlay +was relying on a side effect that is now gone, and must clear that overlay +through the command that owns it. `RESET_COLORS` only undoes `SET_COLORS`. + +Also worth knowing before you rely on it: `RESET_COLORS` restores the entities +the viewer holds in its primary `geometryResult`, which is the FIRST loaded +model. In a federated embed with more than one model, `SET_COLORS` still +colours entities in the later models and `RESET_COLORS` does not restore them, +while both commands ack success. Single-model embeds — the common case — are +unaffected. + `ENTITY_HOVERED` was declared, exposed by the SDK, and never emitted — the SDK tests passed because they fabricated the event themselves. The viewer's hover pipeline was already there but gated behind a toolbar toggle the embed has no