diff --git a/doc/changelog.d/111.added.md b/doc/changelog.d/111.added.md new file mode 100644 index 00000000..5827a2dc --- /dev/null +++ b/doc/changelog.d/111.added.md @@ -0,0 +1 @@ +[Remote rendering 3.2b] save state reads from server camera diff --git a/src/ansys/visor/viewer/models/persist/scene/persisted_scene_state.py b/src/ansys/visor/viewer/models/persist/scene/persisted_scene_state.py index 241d5e93..526ca59d 100644 --- a/src/ansys/visor/viewer/models/persist/scene/persisted_scene_state.py +++ b/src/ansys/visor/viewer/models/persist/scene/persisted_scene_state.py @@ -75,7 +75,6 @@ class PersistedSceneState(BaseModel): cross_section_enabled: bool | None = None edges_enabled: bool | None = None bounding_box_enabled: bool | None = None - camera: VisorCameraState | None = None dataset_states: Dict[str, "PersistedDatasetState"] = Field(default_factory=dict) variable_states: Dict[str, "VisorVariableState"] = Field(default_factory=dict) model_config = ConfigDict(arbitrary_types_allowed=True) diff --git a/src/ansys/visor/viewer/vtk/scene/base.py b/src/ansys/visor/viewer/vtk/scene/base.py index 6743a527..5c87b773 100644 --- a/src/ansys/visor/viewer/vtk/scene/base.py +++ b/src/ansys/visor/viewer/vtk/scene/base.py @@ -157,6 +157,16 @@ async def get_state(self, timeout: float) -> PersistedViewerStateV1: The registry hands out live ``RuntimeDatasetState`` objects that the per-part setters mutate from the trame daemon thread, so each one is deep-copied under ``_vtk_lock``. The lock is taken after the ``await`` and never held across one. + + The camera is the second thing the browser's reply does not get to supply. + It comes from the renderer's record, which is authoritative, rather than + from the reply or from the pipeline ``vtkCamera``: the pipeline is the + record's projection, and reading it back would re-import whatever drift + VTK introduced -- ``ResetCamera`` rewrites ``clipping_range``. The + assignment is unconditional. A ``None`` record means no camera was ever + written, and writing that ``None`` through is what says so; the guard for + "absent says nothing" belongs to the load path, in :meth:`apply_state`, + not here. """ runtime_state = await self._get_runtime_state_async(timeout) @@ -165,6 +175,7 @@ async def get_state(self, timeout: float) -> PersistedViewerStateV1: dataset_id: dataset_state.model_copy(deep=True) for dataset_id, dataset_state in self._dataset_registry.runtime_state_dict.items() } + runtime_state.scene.camera = self._renderer.get_camera_state() runtime_state.scene.dataset_states = registry_dataset_states persisted = self._state_mapper.runtime_to_persisted(runtime_state) diff --git a/src/ansys/visor/visor-client/src/renderer/IRenderer.ts b/src/ansys/visor/visor-client/src/renderer/IRenderer.ts index 5e18cb7f..d490b024 100644 --- a/src/ansys/visor/visor-client/src/renderer/IRenderer.ts +++ b/src/ansys/visor/visor-client/src/renderer/IRenderer.ts @@ -25,6 +25,17 @@ export type VisorCameraState = Readonly<{ parallelScale: number; }>; +/** The seven camera fields that can be applied. No derived fields. */ +export type AppliedCameraState = Readonly<{ + position: readonly number[]; + focalPoint: readonly number[]; + viewUp: readonly number[]; + clippingRange: readonly number[]; + parallelProjection: boolean; + viewAngle: number; + parallelScale: number; +}>; + /** Descriptor consumed by setColorVariableAsync. */ export type ColorVariableDescriptor = Readonly<{ spectrumId: string; @@ -97,11 +108,11 @@ export interface IRenderer { setCameraViewAngleAsync(angle: number): Promise; setCameraParallelScaleAsync(scale: number): Promise; /** - * Apply an entire camera snapshot in one RPC. WasmRenderer implements it + * Apply the seven applied camera fields in one RPC. WasmRenderer implements it * by delegating to the seven per-field setters above. See §11 for the * Story 3.2 rationale (server-tracked camera + sync-back). */ - setCameraStateAsync(state: VisorCameraState): Promise; + setCameraStateAsync(state: AppliedCameraState): Promise; /** Frame the scene on the given bounds; used by scene-graph rebuilds. */ resetCameraAsync(bounds?: readonly number[]): Promise; diff --git a/src/ansys/visor/visor-client/src/renderer/NullRenderer.ts b/src/ansys/visor/visor-client/src/renderer/NullRenderer.ts index 07fffac7..9955a1d2 100644 --- a/src/ansys/visor/visor-client/src/renderer/NullRenderer.ts +++ b/src/ansys/visor/visor-client/src/renderer/NullRenderer.ts @@ -1,4 +1,5 @@ import { + AppliedCameraState, ColorVariableDescriptor, GeometryPickMode, IRenderer, @@ -79,7 +80,7 @@ export class NullRenderer implements IRenderer { // no-op } - async setCameraStateAsync(_state: VisorCameraState): Promise { + async setCameraStateAsync(_state: AppliedCameraState): Promise { // no-op } diff --git a/src/ansys/visor/visor-client/src/renderer/WasmRenderer.ts b/src/ansys/visor/visor-client/src/renderer/WasmRenderer.ts index 0cb6fb98..d9d75804 100644 --- a/src/ansys/visor/visor-client/src/renderer/WasmRenderer.ts +++ b/src/ansys/visor/visor-client/src/renderer/WasmRenderer.ts @@ -1,4 +1,5 @@ import { + AppliedCameraState, ColorVariableDescriptor, GeometryPickMode, IRenderer, @@ -177,7 +178,7 @@ export class WasmRenderer implements IRenderer { await this.#vtkScene.camera.setParallelScale(scale); } - async setCameraStateAsync(state: VisorCameraState): Promise { + async setCameraStateAsync(state: AppliedCameraState): Promise { // Sequential, not Promise.all, to preserve the observable ordering of // camera events any FPS/camera-changed listener sees (§11). await this.setCameraPositionAsync(state.position); diff --git a/tests/integration/test_save_load_state.py b/tests/integration/test_save_load_state.py index 1673c600..1d966a11 100644 --- a/tests/integration/test_save_load_state.py +++ b/tests/integration/test_save_load_state.py @@ -44,6 +44,7 @@ from ansys.visor.viewer.app.visor_vtk import VisorVTK from ansys.visor.viewer.core.metadata import ExtendedMetadata from ansys.visor.viewer.models.common.part_properties import PartProperties +from ansys.visor.viewer.models.common.visor_camera_state import VisorCameraState from ansys.visor.viewer.models.common.visor_ui_state import VisorUIState from ansys.visor.viewer.models.persist.dataset.persisted_dataset_state import PersistedDatasetState from ansys.visor.viewer.models.persist.persisted_viewer_state import PersistedViewerStateV1 @@ -100,6 +101,46 @@ def iface(): pass +# ------------------------------------------------------------------ # +# Camera literals +# +# Hand-written, and different in every field between the two, so the saved +# file names its source by value rather than by a recorded call. No value +# here originates from VTK. +# ------------------------------------------------------------------ # + +RECORD_CAMERA_POSITION = [11.0, 12.0, 13.0] +RECORD_CAMERA_CLIPPING_RANGE = [17.0, 18.0] +REPLY_CAMERA_POSITION = [21.0, 22.0, 23.0] +REPLY_CAMERA_CLIPPING_RANGE = [27.0, 28.0] + + +def _record_camera() -> VisorCameraState: + """The camera the server's record holds at save time.""" + return VisorCameraState( + position=RECORD_CAMERA_POSITION, + focal_point=[14.0, 15.0, 16.0], + view_up=[0.0, 1.0, 0.0], + clipping_range=RECORD_CAMERA_CLIPPING_RANGE, + parallel_projection=True, + view_angle=31.0, + parallel_scale=19.0, + ) + + +def _reply_camera() -> VisorCameraState: + """The camera the browser answers getState with.""" + return VisorCameraState( + position=REPLY_CAMERA_POSITION, + focal_point=[24.0, 25.0, 26.0], + view_up=[1.0, 0.0, 0.0], + clipping_range=REPLY_CAMERA_CLIPPING_RANGE, + parallel_projection=False, + view_angle=32.0, + parallel_scale=29.0, + ) + + # ================================================================== # # write_dataset / read_dataset round-trips # ================================================================== # @@ -488,4 +529,47 @@ def test_reloading_the_saved_state_restores_the_registry(self, file_io, iface, t assert record.spectrum_id == "POINT::pressure::1" assert record.spectrum_component == 0 + def test_saved_visor_json_carries_the_camera_record_not_the_browsers(self, iface, tmp_path): + """save_state writes the server's camera record, not the browser's reply. + + The record is seeded through ``sync_camera``, which also projects onto + the pipeline camera, so record and pipeline hold the same values here. + This case therefore discriminates the **record from the browser's + reply** and nothing more; separating the record from its own pipeline + projection is done in tests/unit/vtk/scene/test_base.py, against a + renderer double whose pipeline read answers with different numbers. + + No dataset is added, so ``finalize_scene``'s reset never runs and + cannot overwrite the seeded record with a VTK-derived one. + """ + iface._scene._renderer.sync_camera(_record_camera()) + + # The browser answers getState with a different camera in every field. + # A pass therefore proves the file came from the record. + frontend_state = RuntimeAppState.from_components( + dark_mode=False, + unit="m", + dataset_states={}, + camera=_reply_camera(), + ) + + async def _frontend_round_trip(timeout: float = 5.0): + return frontend_state + + iface._scene._get_runtime_state_async = _frontend_round_trip + iface._server_manager = MagicMock() + iface._server_manager.running = True + + asyncio.run(iface.save_state(str(tmp_path))) + + with open(os.path.join(str(tmp_path), "visor.json"), "r") as fh: + written = json.load(fh) + + # write_state dumps by_alias, so the camera's own fields are aliased. + camera = written["scene"]["camera"] + assert camera["position"] == RECORD_CAMERA_POSITION + assert camera["clippingRange"] == RECORD_CAMERA_CLIPPING_RANGE + assert camera["position"] != REPLY_CAMERA_POSITION + assert camera["clippingRange"] != REPLY_CAMERA_CLIPPING_RANGE + diff --git a/tests/unit/vtk/scene/test_base.py b/tests/unit/vtk/scene/test_base.py index 84752c15..979ae664 100644 --- a/tests/unit/vtk/scene/test_base.py +++ b/tests/unit/vtk/scene/test_base.py @@ -938,6 +938,228 @@ def test_get_state_snapshots_the_registry_rather_than_referencing_it(scene, regi assert snapshot.part_states[NODE_ID].opacity == 0.25 +# --------------------------------------------------------------------------- +# Save path — the camera +# +# The save takes the camera from the renderer's record. Three things could +# supply one and they are told apart here by value, not by call recording: +# +# * the record -> RECORD_* (what the save must write) +# * the browser's reply -> REPLY_* (what the save wrote before) +# * the pipeline camera -> PIPELINE_* (the record's projection, which +# re-imports VTK's own drift) +# +# The record and the pipeline agree in the running application except in +# clipping_range, and only after a ResetCamera, so reading the pipeline is a +# silent failure there. The three literal sets below differ in every field so +# that it is not silent here. +# +# These tests run the REAL state mapper -- not the _capture_persist_input +# helper, which stubs it -- because the assertion has to be on the object +# get_state returns. An assignment placed after runtime_to_persisted passes +# every assertion made against the runtime object and still writes the wrong +# file. The registry is replaced with an empty one so the mapper's per-dataset +# loop is empty and cannot contribute a failure. +# --------------------------------------------------------------------------- + +# Hand-written literals. Nothing here is computed the way the code computes +# it, and no value originates from VTK. +RECORD_POSITION = [11.0, 12.0, 13.0] +RECORD_FOCAL_POINT = [14.0, 15.0, 16.0] +RECORD_VIEW_UP = [0.0, 1.0, 0.0] +RECORD_CLIPPING_RANGE = [17.0, 18.0] +RECORD_PARALLEL_PROJECTION = True +RECORD_VIEW_ANGLE = 31.0 +RECORD_PARALLEL_SCALE = 19.0 + +REPLY_POSITION = [21.0, 22.0, 23.0] +REPLY_CLIPPING_RANGE = [27.0, 28.0] + +PIPELINE_POSITION = [31.0, 32.0, 33.0] +PIPELINE_CLIPPING_RANGE = [37.0, 38.0] + + +def _record_camera() -> VisorCameraState: + """The renderer's camera record, from hand-written literals.""" + return VisorCameraState( + position=RECORD_POSITION, + focal_point=RECORD_FOCAL_POINT, + view_up=RECORD_VIEW_UP, + clipping_range=RECORD_CLIPPING_RANGE, + parallel_projection=RECORD_PARALLEL_PROJECTION, + view_angle=RECORD_VIEW_ANGLE, + parallel_scale=RECORD_PARALLEL_SCALE, + ) + + +def _reply_camera() -> VisorCameraState: + """What the browser answers getState with. Never the right answer.""" + return VisorCameraState( + position=REPLY_POSITION, + focal_point=[24.0, 25.0, 26.0], + view_up=[1.0, 0.0, 0.0], + clipping_range=REPLY_CLIPPING_RANGE, + parallel_projection=False, + view_angle=32.0, + parallel_scale=29.0, + ) + + +def _pipeline_camera() -> VisorCameraState: + """What a read of the pipeline vtkCamera would return.""" + return VisorCameraState( + position=PIPELINE_POSITION, + focal_point=[34.0, 35.0, 36.0], + view_up=[0.0, 0.0, 1.0], + clipping_range=PIPELINE_CLIPPING_RANGE, + parallel_projection=False, + view_angle=33.0, + parallel_scale=39.0, + ) + + +class _CameraRecordRenderer: + """Hand-written renderer double for the save path's camera read. + + Deliberately not a MagicMock. It answers the pipeline read with numbers + of its own, so an implementation that read the pipeline instead of the + record fails on **values** rather than on AttributeError -- an + AttributeError would also be raised by an implementation that read nothing + at all, and the two are different defects. + + Records the scene's lock depth at the moment the record is read, so the + read can be asserted to happen with the lock *held* rather than merely + taken at some point. + """ + + def __init__(self, record, pipeline_camera, scene=None): + self._record = record + self._pipeline_camera = pipeline_camera + self._scene = scene + self.record_reads = 0 + self.pipeline_reads = 0 + self.depth_at_read = None + + def get_camera_state(self): + self.record_reads += 1 + if self._scene is not None: + self.depth_at_read = getattr(self._scene._vtk_lock, "depth", None) + return self._record + + def _read_pipeline_camera(self): + self.pipeline_reads += 1 + return self._pipeline_camera + + +def _save_scene(scene, record, reply_camera): + """Wire *scene* for a save whose record is *record* and whose browser + reply carries *reply_camera*. Returns the renderer double.""" + double = _CameraRecordRenderer(record, _pipeline_camera(), scene=scene) + scene._renderer = double + scene._dataset_registry = VisorDatasetRegistry() + + async def _get_runtime_state_async(timeout): + return RuntimeAppState.from_components( + dark_mode=False, + unit="m", + dataset_states={}, + camera=reply_camera, + ) + + scene._get_runtime_state_async = _get_runtime_state_async + return double + + +def test_get_state_takes_the_camera_from_the_record(scene): + """The saved camera is the record's, field for field. + + This is the assertion that pins the change. Reverted, the camera on the + returned state is the browser's reply and every field below differs. + + Asserted on what get_state RETURNS -- the object that reaches the writer + -- not on the runtime state it was built from. + """ + _save_scene(scene, _record_camera(), _reply_camera()) + + persisted = asyncio.run(scene.get_state(timeout=1.0)) + + camera = persisted.scene.camera + assert camera.position == RECORD_POSITION + assert camera.focal_point == RECORD_FOCAL_POINT + assert camera.view_up == RECORD_VIEW_UP + assert camera.clipping_range == RECORD_CLIPPING_RANGE + assert camera.parallel_projection == RECORD_PARALLEL_PROJECTION + assert camera.view_angle == RECORD_VIEW_ANGLE + assert camera.parallel_scale == RECORD_PARALLEL_SCALE + + +def test_get_state_discards_the_camera_the_browser_returned(scene): + """The browser's camera does not survive into the persisted state. + + Its own test rather than an extra assertion above: "wrote the record" and + "did not write the reply" are the same only while the round trip still + carries a camera at all, and the round trip is not being removed. + """ + _save_scene(scene, _record_camera(), _reply_camera()) + + persisted = asyncio.run(scene.get_state(timeout=1.0)) + + assert persisted.scene.camera.position != REPLY_POSITION + assert persisted.scene.camera.clipping_range != REPLY_CLIPPING_RANGE + + +def test_get_state_does_not_read_the_pipeline_camera(scene): + """The record is read; the pipeline is not. + + The one test that separates the record from its own projection. In the + running application the two agree except in clipping_range, and only + after a ResetCamera, so no manual check discriminates them reliably. + """ + double = _save_scene(scene, _record_camera(), _reply_camera()) + + persisted = asyncio.run(scene.get_state(timeout=1.0)) + + assert double.record_reads == 1 + assert double.pipeline_reads == 0 + assert persisted.scene.camera.position != PIPELINE_POSITION + assert persisted.scene.camera.clipping_range != PIPELINE_CLIPPING_RANGE + + +def test_get_state_writes_no_camera_when_the_record_is_empty(scene): + """Negative twin: an empty record writes no camera, reply notwithstanding. + + The record is None only for a scene that never held a dataset, since the + first one resets the camera and that reset writes the record. The reply + carries a perfectly valid camera, so this is the only test in the module + that a guarded assignment -- one that skipped the write when the record + was None -- would fail. + """ + _save_scene(scene, None, _reply_camera()) + + persisted = asyncio.run(scene.get_state(timeout=1.0)) + + assert persisted.scene.camera is None + + +def test_get_state_reads_the_camera_under_the_lock(scene): + """The lock is *held* at the moment the record is read. + + The record is a single attribute holding a whole object reference, but the + read sits in the same critical section as the registry snapshot and is + asserted the same way, on the precedent of + test_get_state_reads_the_registry_under_the_lock -- which also covers the + await, since the lock is taken after it and never held across it. + """ + scene._vtk_lock = _LockSpy() + double = _save_scene(scene, _record_camera(), _reply_camera()) + + asyncio.run(scene.get_state(timeout=1.0)) + + assert double.depth_at_read >= 1 + assert scene._vtk_lock.depth == 0 + assert scene._vtk_lock.enter_count == scene._vtk_lock.exit_count + + def test_apply_state_pushes_a_json_encodable_runtime_state(scene, registry): """What the bridge is handed survives JSON encoding.