diff --git a/doc/changelog.d/125.added.md b/doc/changelog.d/125.added.md new file mode 100644 index 00000000..bcdfef2d --- /dev/null +++ b/doc/changelog.d/125.added.md @@ -0,0 +1 @@ +[Remote rendering 3.2d] report settled camera gestures to the server diff --git a/src/ansys/visor/viewer/app/trame/local_app.py b/src/ansys/visor/viewer/app/trame/local_app.py index 23abd24f..cf5c0202 100644 --- a/src/ansys/visor/viewer/app/trame/local_app.py +++ b/src/ansys/visor/viewer/app/trame/local_app.py @@ -11,6 +11,8 @@ from ansys.visor.viewer.config import settings from ansys.visor.viewer.core.visor_enums import VisorVtkVariableType from ansys.visor.viewer.core.visor_logging import VisorDefaultLogger +from ansys.visor.viewer.models.common.visor_camera_state import VisorCameraState +from ansys.visor.viewer.models.runtime.requests.sync_camera_payload import SyncCameraPayload logger = VisorDefaultLogger(__name__) @@ -25,6 +27,10 @@ class ScenePartStateApi(Protocol): ``isinstance`` check anywhere against it. Declaring it here rather than importing the scene keeps this module free of any scene type, so the injected object remains LocalApp's only route to the scene. + + ``sync_camera`` is not per-part, and it is declared here anyway: the one + production injection site passes the whole scene coordinator, so a second + protocol would be the same object under a second name. """ def set_part_visibility(self, node_id: int, visible: bool) -> None: ... @@ -48,6 +54,8 @@ def set_part_color_variable( def clear_part_color_variable(self, node_id: int) -> None: ... + def sync_camera(self, camera_state: VisorCameraState) -> None: ... + # ---------------------------------------------------------------------- # Trigger payload models @@ -175,6 +183,7 @@ class LocalApp: set_part_selected: selects or deselects one part set_part_color_variable: colours one part by a scalar variable clear_part_color_variable: stops colouring one part by a scalar variable + sync_camera: records a settled camera reported by the frontend set_only_cookie: sets a cookie on the server (note: Trame server only allows a single cookie header) Protected Methods: _cleanup(): Cleans up the active actor in the visualization pipeline. @@ -416,6 +425,48 @@ def clear_part_color_variable(self, payload) -> None: return api.clear_part_color_variable(payload.node_id) + # ------------------------------------------------------------------ + # Camera trigger + # + # Frontend -> Backend. One report per settled camera window, never one + # per camera event: the debounce lives on the client, and the camera is + # read once, at settle. + # + # ``origin`` is decided at the input, on the client, and travels + # verbatim; the *server* decides what to do with it. A report that is + # not a gesture is an echo of a camera the application itself applied -- + # a load, a reset, a scene-details push -- and applying it would + # overwrite the record with a value the server had just sent. It is + # dropped here, with a log line, before the lock is taken and before the + # coordinator is even looked up: a dropped report is visible when + # diagnosing an echo, and a report the client never sent is not. + # + # Exactly one debug line per arrival on every path, so that counting + # arrivals in the log is a sound measurement. + # ------------------------------------------------------------------ + + @trigger("sync_camera") + @parse_payload(SyncCameraPayload) + def sync_camera(self, payload) -> None: + """Frontend -> Backend: a settled camera window reports its camera. + + A payload missing any of the seven camera fields, or carrying an + ``origin`` that is neither value, never reaches this body: it is a + logged warning from the payload decorator and nothing is delegated. + """ + if payload.origin != "gesture": + logger.debug("sync_camera: origin=%s; dropping.", payload.origin) + return + api = self._part_state_api("sync_camera") + if api is None: + return + logger.debug( + "sync_camera: origin=%s; applying position=%s.", + payload.origin, + payload.camera.position, + ) + api.sync_camera(payload.camera) + def set_only_cookie(self, key: str, value: str): """ Sets a cookie on the server. NOTE: there is a limitation diff --git a/src/ansys/visor/viewer/models/runtime/requests/sync_camera_payload.py b/src/ansys/visor/viewer/models/runtime/requests/sync_camera_payload.py new file mode 100644 index 00000000..3f235b1a --- /dev/null +++ b/src/ansys/visor/viewer/models/runtime/requests/sync_camera_payload.py @@ -0,0 +1,37 @@ +"""Model for the ``sync_camera`` trigger payload.""" + +from typing import Literal + +from pydantic import BaseModel, ConfigDict + +from ansys.visor.viewer.models.common.visor_camera_state import VisorCameraState + + +class SyncCameraPayload(BaseModel): + """ + Payload of the ``sync_camera`` trigger. + + ``origin`` travels on the wire and the server decides what to do with it: + the client sends both values and never suppresses a report it believes is + an echo. A report the server drops is visible in the log when diagnosing + an echo; one the client never sent is not. + + ``camera`` is a whole :class:`VisorCameraState` -- whole or absent, never + partial. All seven fields are required with no default, so a payload + missing any one of them fails validation and is a logged no-op at the + trigger boundary rather than a half-applied camera. + + Extra keys are ignored, which is deliberate rather than incidental: the + client's own camera snapshot type carries five derived display fields + (``distance``, ``orthographic``, ``orthographicScale``, ``unitsPerPixel``, + ``viewPortHeight``) beyond the seven applied ones, and a sender that + spread that whole object would still validate here. What pins the wire + shape is therefore a test on the payload the client builds, not this + model. + """ + + model_config = ConfigDict(populate_by_name=True) + + origin: Literal["gesture", "programmatic"] + camera: VisorCameraState + diff --git a/src/ansys/visor/viewer/vtk/scene/base.py b/src/ansys/visor/viewer/vtk/scene/base.py index 5c87b773..2a066af2 100644 --- a/src/ansys/visor/viewer/vtk/scene/base.py +++ b/src/ansys/visor/viewer/vtk/scene/base.py @@ -13,6 +13,7 @@ from ansys.visor.viewer.core.visor_enums import VisorVtkVariableType from ansys.visor.viewer.core.visor_logging import VisorDefaultLogger from ansys.visor.viewer.core.visor_types import VisorDatasetType +from ansys.visor.viewer.models.common.visor_camera_state import VisorCameraState from ansys.visor.viewer.models.persist.persisted_viewer_state import PersistedViewerStateV1 from ansys.visor.viewer.models.runtime.visor_scene_details import VisorSceneDetails from ansys.visor.viewer.renderer.base import IRenderer @@ -411,6 +412,35 @@ def reset_camera(self): self._renderer.reset_camera(self._scene_graph.bounds) self._renderer.serialize_camera_state() + def sync_camera(self, camera_state: VisorCameraState) -> None: + """Record a camera the frontend reported, and project it. + + The trigger path's coordinator method. It is the camera twin of the + per-part coordinator surface below: the trigger handler arrives on + trame's daemon thread and must route through a method that takes + ``_vtk_lock``, never call the renderer directly. + + Both halves run in one critical section, and the re-serialisation is + part of the write rather than an afterthought. The backend advertises + a version number read from the live VTK object while serving content + from a cache, so a write with no re-serialise publishes a new version + against old content: the client then fetches the *pre*-gesture camera + and applies it over the one the user just set, and a refresh shows the + framing they moved away from. The load path proved this in + Increment 2b; the trigger path has the same gap for the same reason. + + What this method deliberately does **not** do is notify. No + ``render()``, no ``flush_wasm_state()``, no ``set_state``. A push here + rebuilds the client, the rebuild re-delivers state, the reapply moves + the camera and emits further settle reports, and each report pushes + again. It would also race the rebuild against a half-written object + graph -- the hazard ``_apply_runtime_state_to_render`` already refuses + to reopen. Serialising without notifying is the whole point. + """ + with self._vtk_lock: + self._renderer.sync_camera(camera_state) + self._renderer.serialize_camera_state() + def pick_geometry(self, actor_wasm_id, cell_id, mode, world_x, world_y, world_z) -> dict: """ Frontend-trigger entry point for cell picking. Packs the world-space diff --git a/src/ansys/visor/visor-client/src/App.tsx b/src/ansys/visor/visor-client/src/App.tsx index bb2a8769..831904b3 100644 --- a/src/ansys/visor/visor-client/src/App.tsx +++ b/src/ansys/visor/visor-client/src/App.tsx @@ -131,7 +131,22 @@ function App() { requireWasmAnnotation(sceneDetails.vtkInfo.rendererAnnotation), wasmView.current.trameTriggerAsync ); - const newFrontend = new VisorFrontend(renderer, sceneDetails.vtkInfo.sceneGraph); + const newFrontend = new VisorFrontend( + renderer, + sceneDetails.vtkInfo.sceneGraph, + wasmView.current.trameTriggerAsync + ); + // Release the frontend being replaced, here and not later: the + // `VtkScene` behind both renderers is the same object across a + // rebuild, so an unreleased subscription stays live and the next + // gesture is reported once per surviving frontend. There is + // deliberately no `await` between the new frontend subscribing (the + // constructor above) and the old one releasing, so the window in + // which two subscriptions coexist contains no suspension point. + if (oldFrontend != null) { + oldFrontend.releaseCameraSettledListener(); + } + if (visorArgs.current.darkMode != null) { // Explicit Dash prop takes precedence over the server's dark_mode value. sceneDetails.appState.ui.setDarkTheme(visorArgs.current.darkMode); diff --git a/src/ansys/visor/visor-client/src/CameraSyncReporter.ts b/src/ansys/visor/visor-client/src/CameraSyncReporter.ts new file mode 100644 index 00000000..347606bb --- /dev/null +++ b/src/ansys/visor/visor-client/src/CameraSyncReporter.ts @@ -0,0 +1,121 @@ +import type { + CameraOrigin, + IRenderer, + TrameTriggerSender, + VisorCameraState, +} from './renderer/IRenderer'; + +/** + * The `sync_camera` trigger: the client's half of the server-tracked camera. + * + * This module exists apart from `VisorFrontend` so that it can be tested. A + * `VisorFrontend` cannot be constructed under jest -- it needs a real scene + * graph node, the module-global spectrum manager and a renderer that accepts + * `attachSceneGraph`, and it ends in `Object.freeze` -- so a listener body + * written inline there would be pinned by nothing, and the payload shape is + * precisely the part no server-side gate can check. `CameraGestureTracker` + * was split out for the same reason in the previous increment. + * + * What reaches here is already debounced: `addCameraSettledListener` fires + * once per settle window, never once per camera event, and it carries that + * window's origin. The camera is therefore read exactly once per settle -- + * the read is a multi-await round trip to the wasm camera, and paying it per + * event is what the debounce exists to avoid. + */ + +/** The payload of the `sync_camera` trigger. Mirrors the server's model. */ +export type SyncCameraPayload = Readonly<{ + origin: CameraOrigin; + camera: Readonly<{ + position: readonly number[]; + focalPoint: readonly number[]; + viewUp: readonly number[]; + clippingRange: readonly number[]; + parallelProjection: boolean; + viewAngle: number; + parallelScale: number; + }>; +}>; + +/** + * The part of a renderer this module uses. Narrower than `IRenderer` so the + * unit test can supply exactly these two members without a cast that would + * defeat the type check it is here to get. + */ +export type CameraSyncSource = Pick; + +/** + * Build the trigger payload from a settled camera snapshot. + * + * The seven applied fields are named one by one, deliberately, rather than + * spread from the snapshot. `VisorCameraState` carries five further derived + * display fields -- `distance`, `orthographic`, `orthographicScale`, + * `unitsPerPixel`, `viewPortHeight` -- which are meaningless to the server's + * record. Spreading would put all twelve on the wire, and the server would + * accept it silently: pydantic ignores unknown keys, so every server-side + * gate would stay green while the wire contract quietly became "whatever the + * client's snapshot type happens to hold today". Naming the seven is the only + * place that shape is decided, which is why the test asserts the key set. + * + * `parallelProjection` is carried verbatim. `getCameraStateAsync` has already + * narrowed the wasm camera's raw value to a boolean; this module does not + * re-derive it. + * + * `origin` is carried verbatim too, for both values. The client never + * suppresses a report it believes is an echo -- the server decides, and logs + * what it dropped. + */ +export function buildSyncCameraPayload( + origin: CameraOrigin, + camera: VisorCameraState +): SyncCameraPayload { + return { + origin, + camera: { + position: camera.position, + focalPoint: camera.focalPoint, + viewUp: camera.viewUp, + clippingRange: camera.clippingRange, + parallelProjection: camera.parallelProjection, + viewAngle: camera.viewAngle, + parallelScale: camera.parallelScale, + }, + }; +} + +/** + * Subscribe to settled camera windows and report each one to the server. + * + * Returns the remover from `addCameraSettledListener`, **verbatim**. The + * caller owns it and must call it when the frontend holding it is replaced: + * the `VtkScene` survives a client rebuild, so a frontend that is discarded + * without releasing leaves its subscription live and every later gesture is + * reported once per rebuild that has ever happened. Each of those reports is + * individually valid, which is why no gate can see the fault. + * + * The sender is required. A frontend with no transport is not a state this + * path supports: it would subscribe, read the camera on every settle, build a + * payload and drop it, which is indistinguishable at runtime from a working + * wire that the server is ignoring. + * + * A failed send is logged and swallowed, never rethrown. The settle callback + * returns `void` and is invoked from a timer, so a rejection escaping it has + * no caller to receive it and would surface as an unhandled rejection. The + * log prefix is fixed and greppable because it is the only signal that a + * report was lost -- the view looks identical either way. + */ +export function attachCameraSyncReporter( + renderer: CameraSyncSource, + send: TrameTriggerSender +): () => void { + return renderer.addCameraSettledListener((origin: CameraOrigin) => { + void (async () => { + try { + const camera = await renderer.getCameraStateAsync(); + await send('sync_camera', buildSyncCameraPayload(origin, camera)); + } catch (err) { + console.error(`[VISOR] sync_camera trigger send failed: origin='${origin}'`, err); + } + })(); + }); +} diff --git a/src/ansys/visor/visor-client/src/VisorFrontend.tsx b/src/ansys/visor/visor-client/src/VisorFrontend.tsx index d29e1c92..9841b837 100644 --- a/src/ansys/visor/visor-client/src/VisorFrontend.tsx +++ b/src/ansys/visor/visor-client/src/VisorFrontend.tsx @@ -9,6 +9,8 @@ import { TreeViewUtil } from './treeview/TreeView.tsx'; import { StateInput } from './state/appstate/VisorStateCommon.tsx'; import VisorVtkSceneNode from './state/appstate/vtkInfo/VisorVtkSceneNode.tsx'; import { IRenderer, VisorCameraState } from './renderer/IRenderer'; +import type { TrameTriggerSender } from './renderer/IRenderer'; +import { attachCameraSyncReporter } from './CameraSyncReporter'; import { Panel_TopRight_Util } from './components/ui-panels/Panel_TopRight_Util.tsx'; import { Panel_TopLeft_Util } from './components/ui-panels/Panel_TopLeft_Util.tsx'; import { OrientationWidget } from './widgets/orientationWidget.ts'; @@ -17,7 +19,11 @@ import { UiScaffoldUtil } from './components/UiScaffold.tsx'; export type { VisorCameraState } from './renderer/IRenderer'; export class VisorFrontend { - constructor(renderer: IRenderer, sceneGraphNode: VisorVtkSceneNode) { + constructor( + renderer: IRenderer, + sceneGraphNode: VisorVtkSceneNode, + triggerSender: TrameTriggerSender + ) { const spectrumManager = getSpectrumManager(); const sceneGraph = CreateVisorSceneGraph(sceneGraphNode, spectrumManager, renderer); spectrumManager.finishAddingDataArrayMetadata(); @@ -73,6 +79,26 @@ export class VisorFrontend { }; this.domElement = renderer.domElement; this.getCameraStateAsync = () => renderer.getCameraStateAsync(); + // Report each settled camera window to the server. The remover is + // held rather than discarded because the `VtkScene` behind the + // renderer survives a client rebuild: a frontend replaced without + // releasing leaves its subscription live, and every later gesture is + // then reported once per rebuild that has ever happened. Every one of + // those reports is individually valid, so nothing fails -- the record + // is simply written several times and no gate can tell. + // + // Assigned to a `#private` field and exposed through a field-assigned + // method: `Object.freeze(this)` below does not reach `#private` state, + // but it would make a public field assigned after construction throw + // in the browser and in no gate. + this.#cameraSettledRemover = attachCameraSyncReporter(renderer, triggerSender); + this.releaseCameraSettledListener = () => { + const remover = this.#cameraSettledRemover; + // Cleared first, so a second call is a no-op rather than a second + // removal against a map the next frontend now owns. + this.#cameraSettledRemover = null; + remover?.(); + }; this.defaultActorColor = []; this.setSpectrumRangeAsync = async (spectrumId, component, min, max) => { const spectrum = spectrumManager.globalSpectrumCollection.getSpectrum(spectrumId); @@ -511,7 +537,14 @@ export class VisorFrontend { } #unit: string; + #cameraSettledRemover: (() => void) | null; darkMode: boolean; + /** + * Release this frontend's settled-camera subscription. Called by the + * rebuild on the frontend it is replacing, before the replacement is + * handed out. Idempotent. + */ + releaseCameraSettledListener: () => void; render: () => Promise; resizeAsync: () => Promise; sceneGraph: VisorSceneNodeExtended; diff --git a/src/ansys/visor/visor-client/src/jest-tests/CameraGestureTracker.test.js b/src/ansys/visor/visor-client/src/jest-tests/CameraGestureTracker.test.js index 5d777c56..4895803e 100644 --- a/src/ansys/visor/visor-client/src/jest-tests/CameraGestureTracker.test.js +++ b/src/ansys/visor/visor-client/src/jest-tests/CameraGestureTracker.test.js @@ -45,7 +45,8 @@ describe('CameraGestureTracker', () => { const mouseOut = () => canvasDiv.dispatchEvent(new MouseEvent('mouseout', { bubbles: true })); const mouseMove = () => canvasDiv.dispatchEvent(new MouseEvent('mousemove', { bubbles: true })); const wheel = () => canvas.dispatchEvent(new Event('wheel')); - const keyUp = (key) => window.dispatchEvent(new KeyboardEvent('keyup', { key })); + const keyDown = (key, opts = {}) => + window.dispatchEvent(new KeyboardEvent('keydown', { key, ...opts })); const blur = () => window.dispatchEvent(new Event('blur')); // ---- the debounce ------------------------------------------------------ @@ -160,8 +161,8 @@ describe('CameraGestureTracker', () => { expect(onSettled).toHaveBeenCalledWith('programmatic'); }); - test('an event within 300 ms of a z keyup reports gesture', () => { - keyUp('z'); + test('an event within 300 ms of a z keydown reports gesture', () => { + keyDown('z'); jest.advanceTimersByTime(299); tracker.noteCameraEvent(); @@ -170,8 +171,8 @@ describe('CameraGestureTracker', () => { expect(onSettled).toHaveBeenCalledWith('gesture'); }); - test('an event within 300 ms of an r keyup reports gesture', () => { - keyUp('r'); + test('an event within 300 ms of an r keydown reports gesture', () => { + keyDown('r'); jest.advanceTimersByTime(299); tracker.noteCameraEvent(); @@ -180,8 +181,29 @@ describe('CameraGestureTracker', () => { expect(onSettled).toHaveBeenCalledWith('gesture'); }); - test('a keyup that is not z or r does not arm input', () => { - keyUp('a'); + test('a keydown that is not z or r does not arm input', () => { + keyDown('a'); + tracker.noteCameraEvent(); + + jest.advanceTimersByTime(300); + + expect(onSettled).toHaveBeenCalledWith('programmatic'); + }); + + test('a z keydown held with Ctrl, Meta or Alt does not arm input', () => { + keyDown('z', { ctrlKey: true }); + keyDown('r', { metaKey: true }); + keyDown('z', { altKey: true }); + tracker.noteCameraEvent(); + + jest.advanceTimersByTime(300); + + expect(onSettled).toHaveBeenCalledWith('programmatic'); + }); + + test('a repeat z/r keydown does not arm input', () => { + keyDown('z', { repeat: true }); + keyDown('r', { repeat: true }); tracker.noteCameraEvent(); jest.advanceTimersByTime(300); @@ -223,7 +245,7 @@ describe('CameraGestureTracker', () => { }); /** - * The wasm canvas's own wheel handler and VtkScene's window keyup handler are + * The wasm canvas's own wheel handler and VtkScene's window keydown handler are * registered before the tracker is constructed, so the camera event raised by a * single wheel notch or a single z/r press reaches noteCameraEvent() before the * tracker's own handler has marked input active. @@ -288,12 +310,12 @@ describe('CameraGestureTracker, when the camera event precedes the tracker handl expect(onSettled).toHaveBeenCalledWith('gesture'); }); - test('a single r keyup reports gesture', () => { - standInBefore(window, 'keyup'); + test('a single r keydown reports gesture', () => { + standInBefore(window, 'keydown'); tracker = new CameraGestureTracker(canvasDiv, canvas); tracker.addSettledListener(onSettled); - window.dispatchEvent(new KeyboardEvent('keyup', { key: 'r' })); + window.dispatchEvent(new KeyboardEvent('keydown', { key: 'r' })); jest.advanceTimersByTime(300); expect(onSettled).toHaveBeenCalledTimes(1); diff --git a/src/ansys/visor/visor-client/src/jest-tests/CameraSyncReporter.test.ts b/src/ansys/visor/visor-client/src/jest-tests/CameraSyncReporter.test.ts new file mode 100644 index 00000000..b5ff9633 --- /dev/null +++ b/src/ansys/visor/visor-client/src/jest-tests/CameraSyncReporter.test.ts @@ -0,0 +1,201 @@ +import { attachCameraSyncReporter, buildSyncCameraPayload } from '../CameraSyncReporter'; +import type { CameraOrigin, TrameTriggerSender, VisorCameraState } from '../renderer/IRenderer'; + +/** + * The `sync_camera` trigger, client side. + * + * Two things are pinned here and nowhere else. The first is the payload: the + * server accepts unknown keys, so a sender that spread its whole camera + * snapshot would pass every server-side gate while putting twelve fields on + * the wire instead of seven. The key set is therefore asserted directly, + * against a hand-written list. + * + * The second is the release. `VtkScene` survives a client rebuild, so a + * frontend replaced without releasing its subscription leaves it live, and + * the next gesture is reported once per surviving frontend. Every one of + * those reports is individually valid: nothing errors, nothing renders wrong, + * and no gate but this one can see it. + * + * Expected values are hand-written literals. The same seven camera values + * appear in `tests/unit/app/test_local_app_sync_camera.py`, written out there + * as well, so the two stacks are compared against one set of numbers rather + * than against each other. + */ + +const TRIGGER_NAME = 'sync_camera'; + +/** + * A settled camera snapshot, typed as the renderer's own read-side type so + * that the double cannot drift from what `getCameraStateAsync` really + * returns. It carries twelve fields: the seven applied ones and five derived + * display ones. Five of the twelve must not reach the wire. + */ +function makeCameraSnapshot(): VisorCameraState { + return { + // Derived display fields -- none of these belongs on the wire. + distance: 20.0, + orthographic: true, + orthographicScale: 21.0, + unitsPerPixel: 22.0, + viewPortHeight: 23.0, + // The seven applied fields. + position: [11.0, 12.0, 13.0], + focalPoint: [14.0, 15.0, 16.0], + viewUp: [0.0, 1.0, 0.0], + clippingRange: [17.0, 18.0], + parallelProjection: true, + viewAngle: 35.0, + parallelScale: 19.0, + }; +} + +/** + * A renderer double whose `addCameraSettledListener` is a real subscription + * with a real remover, so that releasing one subscriber is observably + * different from releasing none. `settle` fans out to whoever is still + * subscribed, which is exactly what `CameraGestureTracker` does at the end of + * a settle window. + */ +function makeRendererDouble() { + const listeners = new Map<() => void, (origin: CameraOrigin) => void>(); + const getCameraStateAsync = jest.fn(async () => makeCameraSnapshot()); + return { + getCameraStateAsync, + addCameraSettledListener: (callback: (origin: CameraOrigin) => void) => { + const remover = () => { + listeners.delete(remover); + }; + listeners.set(remover, callback); + return remover; + }, + settle(origin: CameraOrigin) { + for (const callback of listeners.values()) { + callback(origin); + } + }, + subscriberCount: () => listeners.size, + }; +} + +/** A sender that records its calls and resolves. */ +function makeSender() { + return jest.fn(async () => undefined) as unknown as jest.Mock & TrameTriggerSender; +} + +/** + * Let the settle callback's async body run to completion. The callback is + * synchronous and starts a promise chain; the camera read and the send are + * separate microtask turns. + */ +async function flush() { + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); +} + +describe('the sync_camera report', () => { + test('a gesture settle sends one trigger with the seven-field payload', async () => { + const renderer = makeRendererDouble(); + const sender = makeSender(); + attachCameraSyncReporter(renderer, sender); + + renderer.settle('gesture'); + await flush(); + + expect(sender).toHaveBeenCalledTimes(1); + expect(sender).toHaveBeenCalledWith(TRIGGER_NAME, { + origin: 'gesture', + camera: { + position: [11.0, 12.0, 13.0], + focalPoint: [14.0, 15.0, 16.0], + viewUp: [0.0, 1.0, 0.0], + clippingRange: [17.0, 18.0], + parallelProjection: true, + viewAngle: 35.0, + parallelScale: 19.0, + }, + }); + }); + + test('a programmatic settle sends the origin verbatim rather than suppressing', async () => { + // The client does not decide what is an echo. The server drops it and + // logs the drop, which is what makes an echo diagnosable at all. + const renderer = makeRendererDouble(); + const sender = makeSender(); + attachCameraSyncReporter(renderer, sender); + + renderer.settle('programmatic'); + await flush(); + + expect(sender).toHaveBeenCalledTimes(1); + expect(sender.mock.calls[0][1]).toMatchObject({ origin: 'programmatic' }); + }); + + test('the payload carries exactly the seven applied camera fields', () => { + // Asserted against a written-out list, not against the snapshot the + // builder was handed: the failure this catches is a spread, and a + // spread agrees with any expectation derived from the source object. + const payload = buildSyncCameraPayload('gesture', makeCameraSnapshot()); + + expect(Object.keys(payload).sort()).toEqual(['camera', 'origin']); + expect(Object.keys(payload.camera).sort()).toEqual([ + 'clippingRange', + 'focalPoint', + 'parallelProjection', + 'parallelScale', + 'position', + 'viewAngle', + 'viewUp', + ]); + }); + + test('the camera is read once per settle, not once per event', async () => { + const renderer = makeRendererDouble(); + attachCameraSyncReporter(renderer, makeSender()); + + renderer.settle('gesture'); + await flush(); + + expect(renderer.getCameraStateAsync).toHaveBeenCalledTimes(1); + }); + + test('a sender that rejects does not escape the settle callback', async () => { + const renderer = makeRendererDouble(); + const sender = jest.fn(async () => { + throw new Error('transport down'); + }) as unknown as jest.Mock & TrameTriggerSender; + const consoleError = jest.spyOn(console, 'error').mockImplementation(() => {}); + attachCameraSyncReporter(renderer, sender); + + renderer.settle('gesture'); + await flush(); + + expect(consoleError).toHaveBeenCalled(); + consoleError.mockRestore(); + }); +}); + +describe('a replaced frontend releases its subscription', () => { + test('two reporters over one renderer, the first released, send one trigger', async () => { + // The rebuild sequence App.tsx performs: the replacement subscribes, + // then the frontend being replaced releases, with no suspension point + // in between. Without the release both subscriptions are live and the + // next gesture is reported twice -- twice with the same camera, both + // valid, which is why this is the only place it can be caught. + const renderer = makeRendererDouble(); + const firstSender = makeSender(); + const secondSender = makeSender(); + + const releaseFirst = attachCameraSyncReporter(renderer, firstSender); + attachCameraSyncReporter(renderer, secondSender); + releaseFirst(); + + expect(renderer.subscriberCount()).toBe(1); + + renderer.settle('gesture'); + await flush(); + + expect(firstSender).not.toHaveBeenCalled(); + expect(secondSender).toHaveBeenCalledTimes(1); + }); +}); diff --git a/src/ansys/visor/visor-client/src/wasm/CameraGestureTracker.js b/src/ansys/visor/visor-client/src/wasm/CameraGestureTracker.js index ba9536b6..694c2147 100644 --- a/src/ansys/visor/visor-client/src/wasm/CameraGestureTracker.js +++ b/src/ansys/visor/visor-client/src/wasm/CameraGestureTracker.js @@ -13,23 +13,23 @@ * - The report is tagged `gesture` if the user was providing input while the * events were coming in, and `programmatic` if not (for example, a camera the * server pushed). "Providing input" means a mouse button is held on the - * canvas, or a wheel event or z/r keyup happened within the last CAMERA_SETTLE_MS. + * canvas, or a wheel event or z/r keydown happened within the last CAMERA_SETTLE_MS. * * - That input check runs on every event as it arrives, and the result is * remembered until the report. It cannot run when the timer fires, * because by then the user has let go of the mouse and every drag would * look programmatic. * - * - The wasm wheel handler and `VtkScene`'s keyup handler run before this + * - The wasm wheel handler and `VtkScene`'s keydown handler run before this * tracker's own listeners. So for a single wheel notch or a single z/r press, * the camera event can arrive before the tracker has noticed the input. - * To cover that, a wheel event or a z/r keyup arriving while a report is + * To cover that, a wheel event or a z/r keydown arriving while a report is * pending marks that report `gesture` as well. */ /** * The settle debounce, in milliseconds, and equally the window during which a - * wheel event or a z/r keyup counts as active input. + * wheel event or a z/r keydown counts as active input. * * Tests pin the literal 300 and must not import this constant, so that changing * it here fails a test rather than silently redefining what the tests assert. @@ -39,10 +39,10 @@ export const CAMERA_SETTLE_MS = 300; /** - * Keys whose keyup arms the input window. + * Keys whose keydown arms the input window. * * Kept in step with the `z` / `r` cases of `VtkScene.#setupCamera`'s window - * `keyup` handler, which is what actually mutates the camera. If a key is + * `keydown` handler, which is what actually mutates the camera. If a key is * added there, add it here. * * @type{ReadonlyArray} @@ -86,7 +86,13 @@ export default class CameraGestureTracker { const onWheel = () => { this.#markImpulse(); }; - const onKeyUp = /**@param {KeyboardEvent} e*/ (e) => { + const onKeyDown = /**@param {KeyboardEvent} e*/ (e) => { + // Ctrl+R, Cmd+R and auto-repeat must not move the camera. + // VtkScene applies the same rule: change both together. + if (e.ctrlKey || e.metaKey || e.altKey || e.repeat) { + return; + } + if (e.key != null && CAMERA_KEYS.includes(e.key.toLowerCase())) { this.#markImpulse(); } @@ -96,7 +102,7 @@ export default class CameraGestureTracker { this.#addListener(canvasDiv, 'mouseup', onMouseUp, true); this.#addListener(canvasDiv, 'mouseout', onInputLost, true); this.#addListener(canvas, 'wheel', onWheel, { passive: true }); - this.#addListener(window, 'keyup', onKeyUp, false); + this.#addListener(window, 'keydown', onKeyDown, false); this.#addListener(window, 'blur', onInputLost, false); } diff --git a/src/ansys/visor/visor-client/src/wasm/RemoteVtkScene.js b/src/ansys/visor/visor-client/src/wasm/RemoteVtkScene.js index 84dd322f..3a0ed10b 100644 --- a/src/ansys/visor/visor-client/src/wasm/RemoteVtkScene.js +++ b/src/ansys/visor/visor-client/src/wasm/RemoteVtkScene.js @@ -161,6 +161,10 @@ export default class RemoteVtkScene { wasmHandler.bindCanvasToDOM(renderWindowId, canvasDiv); const canvas = canvasDiv.getElementsByTagName('canvas')[0]; canvas.style.cssText = `position:absolute;left:0;top:0;width:100%;height:100%;`; + // vtk-wasm creates the canvas with tabindex="0". Focused, it takes key events and browser + // shortcuts stop working (Ctrl+R, Ctrl+F). Visor's keys listen on window, so the + // canvas never needs focus. + canvas.removeAttribute('tabindex'); //////////////////////////////////////////////////////////////////////////////////////// //////////////////////////////////////////////////////////////////////////////////////// diff --git a/src/ansys/visor/visor-client/src/wasm/VtkScene.js b/src/ansys/visor/visor-client/src/wasm/VtkScene.js index e8e95acf..81d71a0e 100644 --- a/src/ansys/visor/visor-client/src/wasm/VtkScene.js +++ b/src/ansys/visor/visor-client/src/wasm/VtkScene.js @@ -496,10 +496,15 @@ export default class VtkScene { ); // TODO: need to remove this event listener when the user disposes the WasmView object - window.addEventListener('keyup', async (e) => { + window.addEventListener('keydown', async (e) => { + // Ctrl+R, Cmd+R and auto-repeat must not move the camera. + // CameraGestureTracker applies the same rule: change both together. + if (e.ctrlKey || e.metaKey || e.altKey || e.repeat) { + return; + } switch (e.key.toLowerCase()) { // NOTE: the z/r key list is mirrored in CameraGestureTracker, - // which treats a keyup on either as active user input. Adding + // which treats a keydown on either as active user input. Adding // a camera-mutating key here means adding it there too. case 'z': await renderer.ResetCamera(); diff --git a/tests/unit/app/test_local_app_sync_camera.py b/tests/unit/app/test_local_app_sync_camera.py new file mode 100644 index 00000000..9cb2ed81 --- /dev/null +++ b/tests/unit/app/test_local_app_sync_camera.py @@ -0,0 +1,272 @@ +"""Unit tests for ``LocalApp.sync_camera`` -- the camera trigger. + +A module of its own rather than an addition to ``test_local_app.py``: that +module's ``TRIGGER_NAMES`` list drives nine parametrised tests whose meaning is +"one of the six per-part triggers", and ``sync_camera`` is not one of them. It +carries a whole camera rather than a node id, and it drops a well-formed +payload on its own judgement, which no per-part trigger does. + +Coverage targets +---------------- +1. A report tagged ``gesture`` reaches the coordinator exactly once, with the + seven camera values it arrived with. +2. A report tagged ``programmatic`` reaches the coordinator not at all -- + asserted against the whole mock, not against one method name, so that a + drop that leaks through under any other name still fails. +3. Each arrival produces exactly one debug line, on both paths, so that + counting arrivals in the server log is a sound measurement. This is what + manual check MC-3 reads. +4. A camera missing any of its seven fields is a logged no-op: whole or + absent, never partial. +5. An ``origin`` that is neither value is a logged no-op. +6. The client's own twelve-field camera snapshot validates, and exactly the + seven applied fields land. Extra keys are ignored, not rejected. +7. With no coordinator injected, the trigger is a logged no-op. +8. The trigger name survives decoration and is registered with the server. + +Every payload here is a hand-written camelCase literal. The same seven camera +values appear in ``visor-client/src/jest-tests/CameraSyncReporter.test.ts``, +written out there as well, so that the two stacks are compared against one set +of numbers rather than against each other. +""" + +from unittest.mock import MagicMock, patch + +import pytest + +from ansys.visor.viewer.app.trame.local_app import LocalApp + +# The seven applied camera fields, as the wire carries them. Shared, by value +# and not by import, with the client-side test of the same trigger. +CAMERA_POSITION = [11.0, 12.0, 13.0] +CAMERA_FOCAL_POINT = [14.0, 15.0, 16.0] +CAMERA_VIEW_UP = [0.0, 1.0, 0.0] +CAMERA_CLIPPING_RANGE = [17.0, 18.0] +CAMERA_PARALLEL_PROJECTION = True +CAMERA_VIEW_ANGLE = 35.0 +CAMERA_PARALLEL_SCALE = 19.0 + + +def _camera() -> dict: + """The seven-field camera the client sends, as a literal dict.""" + return { + "position": [11.0, 12.0, 13.0], + "focalPoint": [14.0, 15.0, 16.0], + "viewUp": [0.0, 1.0, 0.0], + "clippingRange": [17.0, 18.0], + "parallelProjection": True, + "viewAngle": 35.0, + "parallelScale": 19.0, + } + + +class MockController: + """Minimal trame controller stand-in that records added handlers.""" + + def __init__(self): + self.handlers = {} + self.add_call_count = 0 + + def add(self, event): + self.add_call_count += 1 + + def decorator(fn): + self.handlers[event] = fn + return fn + + return decorator + + +@pytest.fixture +def mock_server(): + """Provide a mock Trame server.""" + server = MagicMock() + server.controller = MockController() + server.http_headers.set_header = MagicMock() + server.name = "TestServer" + server._www = None + return server + + +@pytest.fixture +def api(): + """Stand-in for the injected scene coordinator.""" + return MagicMock(name="scene_part_state_api") + + +@pytest.fixture +def app(mock_server, api): + """LocalApp with a coordinator injected.""" + return LocalApp( + server=mock_server, + get_scene_details_json=MagicMock(), + handle_save_state_response=MagicMock(), + standalone=True, + scene_part_state_api=api, + ) + + +@pytest.fixture +def app_without_api(mock_server): + """LocalApp with no coordinator injected.""" + return LocalApp( + server=mock_server, + get_scene_details_json=MagicMock(), + handle_save_state_response=MagicMock(), + standalone=True, + ) + + +# =========================================================================== +# Delegation +# =========================================================================== + +def test_sync_camera_gesture_delegates_the_camera_once(app, api): + """A gesture report reaches the coordinator once, with its own values. + + The camera is asserted field by field against literals rather than by + comparing against a model the test builds the way the code does. + """ + result = app.sync_camera({"origin": "gesture", "camera": _camera()}) + + assert result is None + api.sync_camera.assert_called_once() + (camera,), _ = api.sync_camera.call_args + assert camera.position == CAMERA_POSITION + assert camera.focal_point == CAMERA_FOCAL_POINT + assert camera.view_up == CAMERA_VIEW_UP + assert camera.clipping_range == CAMERA_CLIPPING_RANGE + assert camera.parallel_projection == CAMERA_PARALLEL_PROJECTION + assert camera.view_angle == CAMERA_VIEW_ANGLE + assert camera.parallel_scale == CAMERA_PARALLEL_SCALE + + +def test_sync_camera_programmatic_delegates_nothing(app, api): + """A programmatic report does not touch the coordinator at all. + + Asserted as the empty call list of the whole mock rather than as + ``sync_camera.assert_not_called()``. The failure this guards against is + an echo reaching the scene by *any* route, and a per-method assertion + would pass while some other method carried it. + """ + app.sync_camera({"origin": "programmatic", "camera": _camera()}) + + assert api.mock_calls == [] + + +# =========================================================================== +# Logging -- one line per arrival, which is what MC-3 counts +# =========================================================================== + +def test_sync_camera_gesture_logs_one_debug_line_with_origin_and_position(app): + """An applied arrival is one line, naming the origin and the position.""" + with patch("ansys.visor.viewer.app.trame.local_app.logger") as log: + app.sync_camera({"origin": "gesture", "camera": _camera()}) + + assert log.debug.call_count == 1 + args = log.debug.call_args.args + assert "gesture" in args + assert CAMERA_POSITION in args + assert log.warning.call_count == 0 + + +def test_sync_camera_programmatic_logs_one_debug_line_with_the_origin(app): + """A dropped arrival is one line too, so drops are countable.""" + with patch("ansys.visor.viewer.app.trame.local_app.logger") as log: + app.sync_camera({"origin": "programmatic", "camera": _camera()}) + + assert log.debug.call_count == 1 + assert "programmatic" in log.debug.call_args.args + assert log.warning.call_count == 0 + + +# =========================================================================== +# Validation -- whole or absent, never partial +# =========================================================================== + +def test_sync_camera_with_a_six_field_camera_is_a_logged_no_op(app, api): + """A camera missing one of the seven never reaches the handler body.""" + camera = _camera() + del camera["parallelScale"] + + with patch("ansys.visor.viewer.app.trame.local_app.logger") as log: + result = app.sync_camera({"origin": "gesture", "camera": camera}) + + assert result is None + assert api.mock_calls == [] + assert log.warning.call_count == 1 + + +def test_sync_camera_with_an_unrecognised_origin_is_a_logged_no_op(app, api): + """``origin`` is validated, not merely compared against "gesture". + + Without the ``Literal`` on the model a typo would validate and then fall + into the drop branch, where it would be indistinguishable from a genuine + programmatic report -- and a typo in the *other* direction would be + indistinguishable from a genuine gesture. + """ + with patch("ansys.visor.viewer.app.trame.local_app.logger") as log: + result = app.sync_camera({"origin": "Gesture", "camera": _camera()}) + + assert result is None + assert api.mock_calls == [] + assert log.warning.call_count == 1 + + +def test_sync_camera_accepts_the_clients_twelve_field_snapshot(app, api): + """The five derived fields are ignored; exactly the seven land. + + The client's camera snapshot type carries twelve fields. Were the client + to spread that whole object onto the wire, this is what the server would + do with it: accept it silently. The test records that, so the next + session knows the wire shape is pinned on the *client* side and not here. + """ + camera = _camera() + camera.update( + distance=20.0, + orthographic=True, + orthographicScale=21.0, + unitsPerPixel=22.0, + viewPortHeight=23.0, + ) + + app.sync_camera({"origin": "gesture", "camera": camera}) + + api.sync_camera.assert_called_once() + (applied,), _ = api.sync_camera.call_args + assert set(applied.model_dump().keys()) == { + "position", + "focal_point", + "view_up", + "clipping_range", + "parallel_projection", + "view_angle", + "parallel_scale", + } + assert applied.position == CAMERA_POSITION + + +# =========================================================================== +# No coordinator, and registration +# =========================================================================== + +def test_sync_camera_is_a_logged_no_op_when_no_coordinator_injected(app_without_api): + """With nothing injected the trigger logs and returns without raising.""" + with patch("ansys.visor.viewer.app.trame.local_app.logger") as log: + result = app_without_api.sync_camera({"origin": "gesture", "camera": _camera()}) + + assert result is None + assert log.debug.call_count == 1 + + +def test_sync_camera_trigger_name_is_registered_after_decoration(app, mock_server): + """``sync_camera`` survives the payload decorator and takes a raw dict.""" + names = [call.args[0] for call in mock_server.trigger.call_args_list] + functions = [ + call.args[0] for call in mock_server.trigger.return_value.call_args_list + ] + registered = dict(zip(names, functions)) + + assert "sync_camera" in registered + assert registered["sync_camera"]({"origin": "programmatic", "camera": _camera()}) is None + diff --git a/tests/unit/vtk/scene/test_base.py b/tests/unit/vtk/scene/test_base.py index 979ae664..b4cda515 100644 --- a/tests/unit/vtk/scene/test_base.py +++ b/tests/unit/vtk/scene/test_base.py @@ -1424,6 +1424,158 @@ def test_apply_state_serializes_the_camera_under_the_lock(scene): assert scene._vtk_lock.enter_count == scene._vtk_lock.exit_count +# =========================================================================== +# Trigger path -- the camera +# +# ``VisorSceneBase.sync_camera`` is the coordinator method the ``sync_camera`` +# trigger routes through. The handler arrives on trame's daemon thread and +# must not touch the renderer directly, so what is asserted here is the whole +# critical section: the write, the projection, the re-serialisation that +# follows the write, the lock, and -- as its own test -- the notify that must +# not happen. +# +# Its own literals, distinct from the load path's above, so that a failure +# names the path that broke. The same seven values are written out again in +# the client-side test of this trigger. +# =========================================================================== + +GESTURE_POSITION = [11.0, 12.0, 13.0] +GESTURE_FOCAL_POINT = [14.0, 15.0, 16.0] +GESTURE_VIEW_UP = [0.0, 1.0, 0.0] +GESTURE_CLIPPING_RANGE = [17.0, 18.0] +GESTURE_PARALLEL_PROJECTION = True +GESTURE_VIEW_ANGLE = 35.0 +GESTURE_PARALLEL_SCALE = 19.0 + + +def _gesture_camera() -> VisorCameraState: + """The camera a settled gesture reports, from hand-written literals.""" + return VisorCameraState( + position=GESTURE_POSITION, + focal_point=GESTURE_FOCAL_POINT, + view_up=GESTURE_VIEW_UP, + clipping_range=GESTURE_CLIPPING_RANGE, + parallel_projection=GESTURE_PARALLEL_PROJECTION, + view_angle=GESTURE_VIEW_ANGLE, + parallel_scale=GESTURE_PARALLEL_SCALE, + ) + + +def test_sync_camera_writes_the_camera_record(scene): + """Store half: a reported camera becomes the server's record.""" + camera = _gesture_camera() + + scene.sync_camera(camera) + + assert scene._renderer.get_camera_state() is camera + + +def test_sync_camera_projects_the_camera_onto_the_pipeline(scene): + """Apply half: the record reaches the server's pipeline camera. + + Separate from the store half on the same grounds as the load path's pair: + either can silently do nothing while the other works, and a record that is + never projected leaves the client rebuilding from the pre-gesture camera. + """ + scene.sync_camera(_gesture_camera()) + + camera = scene._renderer._vtk_renderer.GetActiveCamera.return_value + camera.SetPosition.assert_called_once_with(GESTURE_POSITION) + camera.SetFocalPoint.assert_called_once_with(GESTURE_FOCAL_POINT) + camera.SetViewUp.assert_called_once_with(GESTURE_VIEW_UP) + camera.SetClippingRange.assert_called_once_with(GESTURE_CLIPPING_RANGE) + camera.SetParallelProjection.assert_called_once_with(GESTURE_PARALLEL_PROJECTION) + camera.SetViewAngle.assert_called_once_with(GESTURE_VIEW_ANGLE) + camera.SetParallelScale.assert_called_once_with(GESTURE_PARALLEL_SCALE) + + +def test_sync_camera_serializes_after_the_write_with_the_active_camera_id(scene): + """The re-serialisation follows the write, and carries production's id. + + This is the assertion that pins the increment. Reverted -- the write kept + and the re-serialisation dropped -- the record is right, the pipeline + camera is right, every other test in this module still passes, and the + client is served the pre-gesture camera on its next fetch. The user sees + a refresh snap back to the framing they moved away from. + + The spy appends ``"camera"`` for the write and the two-tuple + ``("serialize", )`` for the re-serialization; the tuple is the + recording format, not the argument. ```` is asserted as the bare id + production passes, since ``UpdateStateFromObject`` takes a single id. + """ + order = [] + real_sync = scene._renderer.sync_camera + + def _sync(camera_state): + order.append("camera") + return real_sync(camera_state) + + scene._renderer.sync_camera = _sync + scene._renderer._object_manager.UpdateStateFromObject = ( + lambda object_id: order.append(("serialize", object_id)) + ) + + scene.sync_camera(_gesture_camera()) + + assert order == ["camera", ("serialize", ACTIVE_CAMERA_WASM_ID)] + + +def test_sync_camera_holds_the_lock_across_both_halves(scene): + """Both halves run inside one critical section. + + Its own test rather than another assertion on the ordering test, on the + precedent of ``test_reset_camera_holds_the_lock``: lock depth and call + order fail for different reasons and want to be readable apart. The + trigger handler runs on trame's daemon thread while the VTK objects it + mutates belong to the caller's thread, and a re-serialisation outside the + lock would read the object graph while another thread was free to mutate + it. That failure is intermittent and never reproduces under a gate. + """ + scene._vtk_lock = _LockSpy() + observed = {} + real_sync = scene._renderer.sync_camera + + def _sync(camera_state): + observed["write_depth"] = scene._vtk_lock.depth + return real_sync(camera_state) + + scene._renderer.sync_camera = _sync + scene._renderer._object_manager.UpdateStateFromObject = ( + lambda object_id: observed.update(serialize_depth=scene._vtk_lock.depth) + ) + + scene.sync_camera(_gesture_camera()) + + assert observed["write_depth"] >= 1 + assert observed["serialize_depth"] >= 1 + assert scene._vtk_lock.depth == 0 + assert scene._vtk_lock.enter_count == scene._vtk_lock.exit_count + + +def test_sync_camera_does_not_notify_the_client(scene): + """Serialise only. No render, no flush, no delegated push. + + A notify here would look correct and would be a loop: the push rebuilds + the client, the rebuild re-delivers state, the reapply moves the camera + and emits further settle reports, and each report pushes again. It would + also race the rebuild against a half-written object graph, which is the + hazard ``_apply_runtime_state_to_render`` already refuses to reopen. + + Nothing else can catch this. Every gate passes with a notify in place, + and the symptom in the browser is a rebuild storm that looks like a + network problem. + """ + notifications = [] + scene._renderer.render = lambda: notifications.append("render") + scene._renderer.render_window_only = lambda: notifications.append("render_window") + scene._renderer.flush_wasm_state = lambda: notifications.append("flush") + scene._apply_runtime_state_to_render = lambda state: notifications.append("bridge") + + scene.sync_camera(_gesture_camera()) + + assert notifications == [] + + # =========================================================================== # reset_camera -- the re-serialisation #