diff --git a/doc/changelog.d/167.maintenance.md b/doc/changelog.d/167.maintenance.md new file mode 100644 index 00000000..797b2deb --- /dev/null +++ b/doc/changelog.d/167.maintenance.md @@ -0,0 +1 @@ +[Remote rendering 3.6c] Reserialize actor state inside the renderer apply methods diff --git a/src/ansys/visor/viewer/renderer/base.py b/src/ansys/visor/viewer/renderer/base.py index 0b3b19ec..c55acf1c 100644 --- a/src/ansys/visor/viewer/renderer/base.py +++ b/src/ansys/visor/viewer/renderer/base.py @@ -96,17 +96,29 @@ def deregister_all(self) -> None: @abstractmethod def apply_visibility(self, node_id: int, visible: bool) -> None: - """Set actor visibility for *node_id*.""" + """Set actor visibility for *node_id*. + + On return, the state this renderer serves to the client reflects the + write, and the client is not notified. + """ @abstractmethod def apply_opacity(self, node_id: int, opacity: float) -> None: - """Set actor opacity for *node_id*.""" + """Set actor opacity for *node_id*. + + On return, the state this renderer serves to the client reflects the + write, and the client is not notified. + """ @abstractmethod def apply_diffuse_color( self, node_id: int, r: float, g: float, b: float ) -> None: - """Set the actor-property diffuse colour for *node_id*.""" + """Set the actor-property diffuse colour for *node_id*. + + On return, the state this renderer serves to the client reflects the + write, and the client is not notified. + """ @abstractmethod def apply_selected( @@ -118,6 +130,9 @@ def apply_selected( *diffuse_rgb* is the part's stored diffuse colour, used when deselecting to restore the property DiffuseColor alongside resetting Ambient/Diffuse. + + On return, the state this renderer serves to the client reflects the + write, and the client is not notified. """ @abstractmethod @@ -173,6 +188,9 @@ def reset_camera(self, bounds: list[float]) -> None: An implementation with no pipeline camera leaves the record at its previous value rather than clearing it, so that a reset cannot destroy a camera the frontend reported. + + On return, the state this renderer serves to the client reflects the + write, and the client is not notified. """ @abstractmethod @@ -189,6 +207,9 @@ def sync_camera(self, camera_state: "VisorCameraState") -> None: The record stores the object as given, without copying: callers rely on object identity through :meth:`get_camera_state`. + + On return, the state this renderer serves to the client reflects the + write, and the client is not notified. """ @abstractmethod @@ -200,24 +221,10 @@ def set_projection(self, parallel: bool) -> None: to the pipeline camera, so a raising VTK setter still leaves the record holding what was asked. A ``None`` record is not seeded: the pipeline write still happens, but the value is lost until the next - :meth:`reset_camera` imports it. Does not re-serialise; that is the - coordinator's job. - """ - - @abstractmethod - def serialize_camera_state(self) -> None: - """Make the state served to the client current for the camera. - - The camera alone. Writing the pipeline camera - makes the server correct: it does not make the state the client is - served correct, and the two are separate steps that can each silently - do nothing. + :meth:`reset_camera` imports it. - **Serialize only; do not notify.** Pushing to the client is - :meth:`flush_wasm_state`'s job and carries a rebuild race that the - load path deliberately refuses. - - No-op on a renderer that serves the client no VTK object state. + On return, the state this renderer serves to the client reflects the + write, and the client is not notified. """ # ------------------------------------------------------------------------ @@ -231,6 +238,9 @@ def set_edges_visible(self, visible: bool) -> None: Scene-wide, not per-node: edges are a single global toggle and the per-part surface that once mirrored it had no reader, no sender and no model field. + + On return, the state this renderer serves to the client reflects the + write, and the client is not notified. """ @abstractmethod @@ -244,6 +254,9 @@ def sync_cross_section_plane( """ Sync the cross-section plane origin and normal from the frontend back to server-side VTK objects. + + On return, the state this renderer serves to the client reflects the + write, and the client is not notified. """ @abstractmethod @@ -253,16 +266,6 @@ def get_cross_section_plane(self) -> "VisorCrossSectionState | None": written one yet. """ - @abstractmethod - def serialize_cross_section_state(self) -> None: - """Make the state served to the client current for the plane. - - Names its ids explicitly and never relies on a render following. - - **Serialize only; do not notify.** - - No-op on a renderer that serves the client no VTK object state. - """ @abstractmethod def set_bounding_box_visibility(self, visible: bool) -> None: diff --git a/src/ansys/visor/viewer/renderer/local_renderer.py b/src/ansys/visor/viewer/renderer/local_renderer.py index ba97e8f5..f2c2e8bc 100644 --- a/src/ansys/visor/viewer/renderer/local_renderer.py +++ b/src/ansys/visor/viewer/renderer/local_renderer.py @@ -13,6 +13,7 @@ from trame_server import Server from trame_vtklocal.widgets.vtklocal import LocalView +from vtkmodules.vtkCommonCore import vtkObject from vtkmodules.vtkCommonDataModel import vtkDataObject from vtkmodules.vtkInteractionStyle import vtkInteractorStyleTrackballCamera from vtkmodules.vtkRenderingCore import ( @@ -59,8 +60,8 @@ class VisorLocalRenderer(IRenderer): _orientation_widget: VisorOrientationWidget _cross_section_widget: VisorCrossSectionWidget _bounding_box_widget: VisorBoundingBox - # Registered pipelines keyed by scene-graph node id. Sole owner as of - # Increment I5; mutated exclusively via register_node / deregister_node. + # Registered pipelines keyed by scene-graph node id. Sole owner; + # mutated exclusively via register_node / deregister_node. _pipelines: dict[int, VtkNodePipeline] _last_camera_state: Optional["VisorCameraState"] _last_cross_section_state: Optional["VisorCrossSectionState"] @@ -166,6 +167,10 @@ def apply_visibility(self, node_id: int, visible: bool) -> None: Resolves the pipeline and delegates to :meth:`VtkNodePipeline.set_visibility`. An unknown *node_id* is a logged no-op, never a raise. + + Ends with :meth:`_serialize_vtk_object` on the actor, after the + write, so the state served to the client carries the new visibility. + Does not notify the client. """ pipe = self._pipelines.get(node_id) if pipe is None: @@ -174,6 +179,7 @@ def apply_visibility(self, node_id: int, visible: bool) -> None: ) return pipe.set_visibility(visible) + self._serialize_vtk_object(pipe.actor) def apply_opacity(self, node_id: int, opacity: float) -> None: """See :meth:`IRenderer.apply_opacity`. @@ -181,6 +187,10 @@ def apply_opacity(self, node_id: int, opacity: float) -> None: Resolves the pipeline and delegates to :meth:`VtkNodePipeline.set_opacity`. An unknown *node_id* is a logged no-op, never a raise. + + Ends with :meth:`_serialize_vtk_object` on the actor's property, + after the write, so the state served to the client carries the new + opacity. Does not notify the client. """ pipe = self._pipelines.get(node_id) if pipe is None: @@ -189,6 +199,7 @@ def apply_opacity(self, node_id: int, opacity: float) -> None: ) return pipe.set_opacity(opacity) + self._serialize_vtk_object(pipe.actor.GetProperty()) def apply_diffuse_color( self, node_id: int, r: float, g: float, b: float @@ -198,6 +209,10 @@ def apply_diffuse_color( Resolves the pipeline and delegates to :meth:`VtkNodePipeline.set_diffuse_color`. An unknown *node_id* is a logged no-op, never a raise. + + Ends with :meth:`_serialize_vtk_object` on the actor's property, + after the write, so the state served to the client carries the new + colour. Does not notify the client. """ pipe = self._pipelines.get(node_id) if pipe is None: @@ -206,6 +221,7 @@ def apply_diffuse_color( ) return pipe.set_diffuse_color(r, g, b) + self._serialize_vtk_object(pipe.actor.GetProperty()) def apply_selected( self, node_id: int, selected: bool, diffuse_rgb: list @@ -217,6 +233,10 @@ def apply_selected( through as given; supplying a default when the part has no stored colour is the coordinator's job, not the renderer's. An unknown *node_id* is a logged no-op, never a raise. + + Ends with :meth:`_serialize_vtk_object` on the actor's property, + after the write, so the state served to the client carries the new + lighting terms and colour. Does not notify the client. """ pipe = self._pipelines.get(node_id) if pipe is None: @@ -225,6 +245,7 @@ def apply_selected( ) return pipe.set_selected(selected, diffuse_rgb) + self._serialize_vtk_object(pipe.actor.GetProperty()) def apply_color_variable( self, @@ -289,10 +310,8 @@ def _serialize_part_state(self, node_id: int) -> None: *node_id* is a logged no-op. Names the mapper's id alone, derived with ``GetId`` on the object as - :meth:`serialize_camera_state` derives the camera's. An id of ``0`` - is one the store has never held (the ROOT sentinel); naming it - degrades to an error-logged no-op in VTK, so it is skipped here with - a debug line and the next full serialization carries the mapper. + :meth:`_serialize_camera_state` derives the camera's; the id lookup + and the unregistered-object skip are :meth:`_serialize_vtk_object`'s. """ pipe = self._pipelines.get(node_id) if pipe is None: @@ -300,23 +319,44 @@ def _serialize_part_state(self, node_id: int) -> None: "_serialize_part_state: no pipeline for node %s; skipping.", node_id ) return - mapper_id = self._object_manager.GetId(pipe.mapper) - if mapper_id == 0: + self._serialize_vtk_object(pipe.mapper) + + def _serialize_vtk_object(self, vtk_object: vtkObject) -> None: + """Re-serialize one VTK object into the state served to the client. + + ``vtklocal`` advertises each object's modification time off the live + object but serves its content from a serialization cache, so a write + without this publishes a new version number against the old content. + **Serialize only; do not notify.** + + An id of ``0`` is one the store has never held (the ROOT sentinel); + naming it degrades to an error-logged no-op in VTK, so it is skipped + here with a debug line and the next full serialization carries the + object. + """ + object_id = self._object_manager.GetId(vtk_object) + if object_id == 0: logger.debug( - "_serialize_part_state: mapper of node %s is not registered yet; skipping.", - node_id, + "_serialize_vtk_object: %s is not registered yet; skipping.", + vtk_object.GetClassName(), ) return - self._object_manager.UpdateStateFromObject(mapper_id) + self._object_manager.UpdateStateFromObject(object_id) # ------------------------------------------------------------------ # IRenderer: camera # ------------------------------------------------------------------ def reset_camera(self, bounds: list[float]) -> None: - """See :meth:`IRenderer.reset_camera`.""" + """See :meth:`IRenderer.reset_camera`. + + Ends with :meth:`_serialize_camera_state`, after the reset, so the + state served to the client carries the reset camera. Does not + notify the client. + """ self._vtk_renderer.ResetCamera(bounds) self._last_camera_state = self._read_pipeline_camera() + self._serialize_camera_state() def get_camera_state(self) -> Optional["VisorCameraState"]: """See :meth:`IRenderer.get_camera_state`.""" @@ -326,16 +366,20 @@ def sync_camera(self, camera_state: "VisorCameraState") -> None: """See :meth:`IRenderer.sync_camera`. Stores before projecting, so a raising VTK setter still leaves the - record holding what the earlier caller asked for. + record holding what the earlier caller asked for. Ends with + :meth:`_serialize_camera_state`, after the projection. Does not + notify the client. """ self._last_camera_state = camera_state self._apply_to_pipeline_camera(camera_state) + self._serialize_camera_state() def set_projection(self, parallel: bool) -> None: """See :meth:`IRenderer.set_projection`. Record first, pipeline second, for the reason :meth:`sync_camera` - gives. + gives. Ends with :meth:`_serialize_camera_state`, after the pipeline + write. Does not notify the client. """ if self._last_camera_state is not None: self._last_camera_state.parallel_projection = parallel @@ -344,9 +388,10 @@ def set_projection(self, parallel: bool) -> None: "set_projection: no camera record; applying to the pipeline only." ) self._vtk_renderer.GetActiveCamera().SetParallelProjection(parallel) + self._serialize_camera_state() - def serialize_camera_state(self) -> None: - """See :meth:`IRenderer.serialize_camera_state`. + def _serialize_camera_state(self) -> None: + """Make the state served to the client current for the camera. ``vtklocal`` advertises each object's modification time off the live VTK object but serves state out of a serialization cache, so a write @@ -423,9 +468,15 @@ def set_edges_visible(self, visible: bool) -> None: Fans out over every registered pipeline. Scene-wide, so there is no node id to resolve and no logged-no-op branch: a scene with no pipelines is a no-op by iteration, not by guard. + + Each pipeline's property is re-serialized with + :meth:`_serialize_vtk_object` right after its own write, so the state + served to the client carries the new edge flag on every part. Does + not notify the client. """ for pipe in self._pipelines.values(): pipe.set_edge_visibility(visible) + self._serialize_vtk_object(pipe.actor.GetProperty()) def set_cross_section_visibility(self, visible: bool) -> None: """No-op in Story 1.2. Phase 3 populates.""" @@ -439,22 +490,27 @@ def sync_cross_section_plane( ``set_normal`` each write the plane *and* the representation, so the clip function and the draggable handle cannot diverge on this path -- which is the defect the client's own set/get asymmetry has. + + Ends with :meth:`_serialize_cross_section_state`, after both writes, + so the state served to the client carries the new plane. Does not + notify the client. """ self._last_cross_section_state = VisorCrossSectionState( origin=list(origin), normal=list(normal) ) self._cross_section_widget.set_origin(origin) self._cross_section_widget.set_normal(normal) + self._serialize_cross_section_state() def get_cross_section_plane(self) -> Optional["VisorCrossSectionState"]: """See :meth:`IRenderer.get_cross_section_plane`.""" return self._last_cross_section_state - def serialize_cross_section_state(self) -> None: - """See :meth:`IRenderer.serialize_cross_section_state`. + def _serialize_cross_section_state(self) -> None: + """Make the state served to the client current for the plane. Two ids, named one at a time, each derived the way - :meth:`serialize_camera_state` derives the camera's: ``GetId`` on the + :meth:`_serialize_camera_state` derives the camera's: ``GetId`` on the object itself, not the id ``register_vtk_object`` returned. Deriving it this way means the equivalence of those two id spaces never has to be established. @@ -466,7 +522,7 @@ def serialize_cross_section_state(self) -> None: The widget id is deliberately not named: ``set_origin`` and ``set_normal`` do not write the widget, whose changing state is - enablement. See MC-6. + enablement. """ self._object_manager.UpdateStateFromObject( self._object_manager.GetId(self._cross_section_widget.plane) diff --git a/src/ansys/visor/viewer/renderer/null_renderer.py b/src/ansys/visor/viewer/renderer/null_renderer.py index 20eaeba7..17d9ca78 100644 --- a/src/ansys/visor/viewer/renderer/null_renderer.py +++ b/src/ansys/visor/viewer/renderer/null_renderer.py @@ -143,12 +143,6 @@ def set_projection(self, parallel: bool) -> None: "set_projection: no camera record; nothing to write." ) - def serialize_camera_state(self) -> None: - """See :meth:`IRenderer.serialize_camera_state`. - - No-op: this renderer serves the client no VTK object state. - """ - # ------------------------------------------------------------------ # Widget control (cross-section, bounding box, edges) @@ -180,11 +174,6 @@ def get_cross_section_plane(self) -> "VisorCrossSectionState | None": """ return self._last_cross_section_state - def serialize_cross_section_state(self) -> None: - """See :meth:`IRenderer.serialize_cross_section_state`. - - No-op: this renderer serves the client no VTK object state. - """ def set_bounding_box_visibility(self, visible: bool) -> None: pass diff --git a/src/ansys/visor/viewer/vtk/scene/base.py b/src/ansys/visor/viewer/vtk/scene/base.py index 738364e4..4b39f67c 100644 --- a/src/ansys/visor/viewer/vtk/scene/base.py +++ b/src/ansys/visor/viewer/vtk/scene/base.py @@ -652,7 +652,6 @@ def reset_camera(self): return 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. @@ -662,14 +661,13 @@ def sync_camera(self, camera_state: VisorCameraState) -> None: 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. + The renderer's write runs in one critical section and, on return, + leaves the state it serves current. The backend advertises a version + number read from the live VTK object while serving content from a + cache, so a write that left the cache stale would publish a new + version against old content: the client would then fetch the + *pre*-gesture camera and apply it over the one the user just set, and + a refresh would show the framing they moved away from. What this method deliberately does **not** do is notify. No ``render()``, no ``flush_wasm_state()``, no ``set_state``. A push here @@ -681,23 +679,21 @@ def sync_camera(self, camera_state: VisorCameraState) -> None: """ with self._vtk_lock: self._renderer.sync_camera(camera_state) - self._renderer.serialize_camera_state() def sync_cross_section_plane( self, origin: list[float], normal: list[float] ) -> None: """Record a cross-section plane the frontend reported, and project it. - Both halves in one critical section, re-serialisation part of the - write, no notify -- for the reasons :meth:`sync_camera` gives. The - plane has exactly one delivery channel to a rebuilt or reconnecting - client, the wasm state fetch that follows this re-serialisation, so an - id that is not re-serialised here is simply not delivered and the - user's drag reappears where it started after a page reload. + One critical section, the served state current on return, no notify + -- for the reasons :meth:`sync_camera` gives. The plane has exactly + one delivery channel to a rebuilt or reconnecting client, the wasm + state fetch, so a plane whose served state is stale is simply not + delivered and the user's drag reappears where it started after a page + reload. """ with self._vtk_lock: self._renderer.sync_cross_section_plane(origin, normal) - self._renderer.serialize_cross_section_state() def pick_geometry(self, actor_wasm_id, cell_id, mode, world_x, world_y, world_z) -> dict: """ @@ -994,21 +990,16 @@ def set_projection(self, parallel: bool) -> None: record and nowhere else, so ``get_state`` derives it rather than reading a second copy that could disagree. - Both halves run in one critical section and the re-serialisation is - part of the write, exactly as in :meth:`sync_camera`: 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 and the client fetches - and re-applies the pre-write camera. Because a projection flip is - visually obvious, omitting the re-serialise shows up as the view - snapping back. + The renderer's write runs in one critical section and, on return, + leaves the state it serves current, exactly as in + :meth:`sync_camera`. Because a projection flip is visually obvious, a + stale served camera shows up as the view snapping back. No notify. No ``render()``, no ``flush_wasm_state()``, no ``set_state``. """ with self._vtk_lock: self._renderer.set_projection(parallel) - self._renderer.serialize_camera_state() # ========================================================================= # UI panel layout — coordinator surface @@ -1078,16 +1069,15 @@ def _restore_camera_state(self, runtime_app_state: "RuntimeAppState") -> None: Restore the camera state from a runtime app state, on the load path. Write the loaded camera to the record and the pipeline camera, so a client rebuilt - from server state (refresh) gets it. Must precede the render step. The re-serialize - is required: the server advertises the camera's live MTime but serves its cached state, - so without it a client fetches the pre-load camera. A state with no camera leaves both - alone. + from server state (refresh) gets it. Must precede the render step. The renderer's + ``sync_camera`` leaves the served state current on return: the server advertises the + camera's live MTime but serves its cached state, so a stale cache would hand a client + the pre-load camera. A state with no camera leaves both alone. Callers must hold ``_vtk_lock``. """ if runtime_app_state.scene.camera is not None: self._renderer.sync_camera(runtime_app_state.scene.camera) - self._renderer.serialize_camera_state() def _restore_widget_state(self, runtime_app_state: "RuntimeAppState") -> None: """ @@ -1108,10 +1098,11 @@ def _restore_widget_state(self, runtime_app_state: "RuntimeAppState") -> None: The cross-section plane is restored here too, and both-or-neither: ``sync_cross_section_plane`` takes an origin and a normal together and the model allows either to be absent, so a half-plane says - nothing rather than half-applying. The re-serialisation follows the - write for the reason :meth:`sync_cross_section_plane` gives -- a - loaded plane that is written but not re-serialised leaves the client - fetching the pre-load one. + nothing rather than half-applying. The renderer's + ``sync_cross_section_plane`` leaves the served state current on + return, for the reason :meth:`sync_cross_section_plane` gives -- a + loaded plane whose served state is stale leaves the client fetching + the pre-load one. Callers must hold ``_vtk_lock``. """ @@ -1129,7 +1120,6 @@ def _restore_widget_state(self, runtime_app_state: "RuntimeAppState") -> None: cs = scene.cross_section if cs.origin is not None and cs.normal is not None: self._renderer.sync_cross_section_plane(cs.origin, cs.normal) - self._renderer.serialize_cross_section_state() def _restore_ui_state(self, runtime_app_state: "RuntimeAppState") -> None: """ diff --git a/tests/unit/renderer/test_local_renderer.py b/tests/unit/renderer/test_local_renderer.py index e6e0afeb..5ab9030a 100644 --- a/tests/unit/renderer/test_local_renderer.py +++ b/tests/unit/renderer/test_local_renderer.py @@ -9,11 +9,15 @@ 3. Per-part visual mutations -- visibility, opacity, diffuse colour, selection and colour variable all resolve the pipeline and delegate to VtkNodePipeline (their VTK effects are asserted in - tests/unit/vtk/test_node_pipeline.py); edge visibility and the - colour-variable range refresh stay no-ops. -4. Camera round-trip -- reset_camera, sync_camera / get_camera_state. + tests/unit/vtk/test_node_pipeline.py), then re-serialize the object they + wrote; edge visibility fans out over every pipeline and re-serializes + each property. +4. Camera -- reset_camera, sync_camera / get_camera_state, set_projection, + each re-serializing the active camera without notifying the client. 5. Render / flush delegation. 6. pick_geometry -- vertex, edge, face modes. +7. Cross-section plane -- the record, the two VTK writes, the two-id + re-serialization, and the seed. """ from __future__ import annotations @@ -23,6 +27,7 @@ import pytest from vtkmodules.vtkCommonCore import vtkFloatArray from vtkmodules.vtkFiltersSources import vtkSphereSource +from vtkmodules.vtkRenderingCore import vtkCamera, vtkRenderWindowInteractor from ansys.visor.viewer.core.visor_enums import VisorVtkVariableType from ansys.visor.viewer.models.common.visor_camera_state import VisorCameraState @@ -30,6 +35,7 @@ from ansys.visor.viewer.renderer.local_renderer import VisorLocalRenderer from ansys.visor.viewer.renderer.null_renderer import NullRenderer from ansys.visor.viewer.vtk.node_pipeline import VtkNodePipeline +from ansys.visor.viewer.vtk.widgets.visor_cross_section import VisorCrossSectionWidget # --------------------------------------------------------------------------- # Lightweight VTK-data stubs (no real VTK objects required for pick tests) @@ -191,6 +197,55 @@ def SetParallelScale(self, value): # noqa: N802 ACTIVE_CAMERA_WASM_ID = 8150002 WRONG_OBJECT_WASM_ID = 8150999 +# One literal per part object, so a re-serialization that names the actor +# where the property was written -- or the mapper, or another part's +# property -- fails by id rather than coinciding. +MAPPER_WASM_ID = 8150003 +ACTOR_WASM_ID = 8150004 +PROPERTY_WASM_ID = 8150005 +SECOND_PROPERTY_WASM_ID = 8150006 + + +def _real_pipeline() -> VtkNodePipeline: + """A real VtkNodePipeline over a sphere, so recorded values are real VTK reads.""" + source = vtkSphereSource() + source.Update() + return VtkNodePipeline.from_dataset(source.GetOutput()) + + +def _record_served(renderer, readers: dict) -> list: + """Key ``GetId`` on *readers*' objects and record ``(id, read())`` at each re-serialization. + + *readers* maps an id literal to ``(vtk_object, read)``. Any other object + resolves to ``WRONG_OBJECT_WASM_ID`` and is recorded with ``None``, so a + re-serialization of the wrong object shows up in the list by id. + """ + renderer._object_manager.GetId.side_effect = lambda obj: next( + (object_id for object_id, (o, _) in readers.items() if o is obj), + WRONG_OBJECT_WASM_ID, + ) + recorded = [] + renderer._object_manager.UpdateStateFromObject.side_effect = lambda object_id: ( + recorded.append( + (object_id, readers[object_id][1]() if object_id in readers else None) + ) + ) + return recorded + + +def _install_real_camera(renderer) -> vtkCamera: + """Make a real vtkCamera the active camera, keyed to ACTIVE_CAMERA_WASM_ID.""" + camera = vtkCamera() + renderer._vtk_renderer.GetActiveCamera.return_value = camera + renderer._object_manager.GetId.side_effect = ( + lambda obj: ACTIVE_CAMERA_WASM_ID + if obj is camera + else RENDER_WINDOW_WASM_ID + if obj is renderer._render_window + else WRONG_OBJECT_WASM_ID + ) + return camera + # --------------------------------------------------------------------------- @@ -478,6 +533,27 @@ def test_set_edges_visible_with_no_pipelines_is_a_no_op(self, renderer): assert mock_logger.debug.call_count == 0 assert mock_logger.warning.call_count == 0 + def test_set_edges_visible_serves_every_part_properly(self, renderer): + """Every part's property is re-serialized by its own id, each with the flag already written. + + Two real pipelines, each property keyed to its own literal, seeded with + edges off. Compared as a sorted list so the pin is on "every part, + once, with the new value", not on registry iteration order. + """ + first, second = _real_pipeline(), _real_pipeline() + first_prop, second_prop = first.actor.GetProperty(), second.actor.GetProperty() + first_prop.SetEdgeVisibility(0) + second_prop.SetEdgeVisibility(0) + renderer._pipelines.update({4: first, 9: second}) + recorded = _record_served(renderer, { + PROPERTY_WASM_ID: (first_prop, first_prop.GetEdgeVisibility), + SECOND_PROPERTY_WASM_ID: (second_prop, second_prop.GetEdgeVisibility), + }) + + renderer.set_edges_visible(True) + + assert sorted(recorded) == [(PROPERTY_WASM_ID, 1), (SECOND_PROPERTY_WASM_ID, 1)] + # =========================================================================== # 3b. Delegated apply bodies: visibility, opacity, diffuse colour, @@ -700,6 +776,96 @@ def test_clear_color_variable_serializes_the_mapper_after_the_write(self, render assert recorded == [(8150003, 0)] + # ------------------------------------------------------------------ + # What the client is served after each per-part write + # + # Real pipeline, ids keyed on object identity, the value read at the + # moment of re-serialization. Each seed differs from the value written, + # so a re-serialization placed before the write records the seed. + # ------------------------------------------------------------------ + + def test_apply_visibility_serves_the_written_visibility(self, renderer): + """The actor is re-serialized by its own id, once, with the visibility already written.""" + pipe = _real_pipeline() + actor = pipe.actor + actor.SetVisibility(1) + renderer._pipelines[4] = pipe + recorded = _record_served(renderer, { + ACTOR_WASM_ID: (actor, actor.GetVisibility), + }) + + renderer.apply_visibility(4, False) + + assert recorded == [(ACTOR_WASM_ID, 0)] + + def test_apply_opacity_serves_the_written_opacity(self, renderer): + """The property is re-serialized by its own id, once, with the opacity already written.""" + pipe = _real_pipeline() + prop = pipe.actor.GetProperty() + prop.SetOpacity(0.9) + renderer._pipelines[4] = pipe + recorded = _record_served(renderer, { + ACTOR_WASM_ID: (pipe.actor, pipe.actor.GetVisibility), + PROPERTY_WASM_ID: (prop, prop.GetOpacity), + }) + + renderer.apply_opacity(4, 0.25) + + assert recorded == [(PROPERTY_WASM_ID, 0.25)] + + def test_apply_diffuse_color_serves_the_written_color(self, renderer): + """The property is re-serialized by its own id, once, with the colour already written.""" + pipe = _real_pipeline() + prop = pipe.actor.GetProperty() + prop.SetDiffuseColor(0.1, 0.2, 0.3) + renderer._pipelines[4] = pipe + recorded = _record_served(renderer, { + ACTOR_WASM_ID: (pipe.actor, pipe.actor.GetVisibility), + PROPERTY_WASM_ID: (prop, prop.GetDiffuseColor), + }) + + renderer.apply_diffuse_color(4, 0.75, 0.5, 0.25) + + assert recorded == [(PROPERTY_WASM_ID, (0.75, 0.5, 0.25))] + + def test_apply_selected_serves_the_written_lighting_terms(self, renderer): + """The property is re-serialized by its own id, once, with every term set_selected writes. + + Diffuse, ambient, ambient colour and diffuse colour: the selected + branch writes all four, and each is seeded to a value it does not + write. The selection ambient colour is the production constant, + written out here as a literal. + """ + pipe = _real_pipeline() + prop = pipe.actor.GetProperty() + prop.SetDiffuse(1.0) + prop.SetAmbient(0.0) + prop.SetAmbientColor(0.9, 0.9, 0.9) + prop.SetDiffuseColor(0.1, 0.2, 0.3) + renderer._pipelines[4] = pipe + recorded = _record_served(renderer, { + ACTOR_WASM_ID: (pipe.actor, pipe.actor.GetVisibility), + PROPERTY_WASM_ID: ( + prop, + lambda: ( + prop.GetDiffuse(), + prop.GetAmbient(), + prop.GetAmbientColor(), + prop.GetDiffuseColor(), + ), + ), + }) + + renderer.apply_selected(4, True, [0.75, 0.5, 0.25]) + + assert len(recorded) == 1 + object_id, (diffuse, ambient, ambient_color, diffuse_color) = recorded[0] + assert object_id == PROPERTY_WASM_ID + assert diffuse == 0.5 + assert ambient == 0.5 + assert ambient_color == pytest.approx((0 / 255, 62 / 255, 111 / 255)) + assert diffuse_color == (0.75, 0.5, 0.25) + # =========================================================================== # 4. Camera @@ -800,49 +966,101 @@ def test_sync_camera_stores_the_object_without_copying(self, renderer): assert renderer.get_camera_state() is cam - def test_serialize_camera_state_updates_the_states_from_the_active_camera_id( - self, renderer - ): - """The re-serialise names the active camera, and nothing else. - - The id source is keyed on object identity, so every object other than - the active camera resolves to a different, equally distinctive - literal. Asserting ``ACTIVE_CAMERA_WASM_ID`` therefore fails if the - implementation names ``_render_window``, ``_vtk_renderer``, the - interactor or the picker, rather than coinciding with them. The - render window has a literal of its own, so the previous call shape - fails by name. Asserting against - ``GetId(renderer._vtk_renderer.GetActiveCamera())`` -- the expression - production reads -- would pass in all of those cases and pin nothing. - - The expected value is the bare id production passes: - ``UpdateStateFromObject`` takes a single id, not a sequence. + # -- what the client is served after each camera write ------------------ + # + # A real vtkCamera installed as the active camera, keyed to + # ACTIVE_CAMERA_WASM_ID; every other object resolves to a different + # literal, so naming the render window or the renderer fails by id. The + # value is read at the moment of re-serialization, and every seed differs + # from what the call writes. + + def test_sync_camera_serves_the_written_camera(self, renderer): + """The camera is re-serialized by its own id, once, with the projection already written.""" + camera = _install_real_camera(renderer) + camera.SetPosition(51.0, 52.0, 53.0) + camera.SetFocalPoint(54.0, 55.0, 56.0) + camera.SetParallelProjection(1) + camera.SetViewAngle(41.0) + camera.SetParallelScale(19.0) + recorded = [] + renderer._object_manager.UpdateStateFromObject.side_effect = lambda object_id: ( + recorded.append(( + object_id, + camera.GetPosition(), + camera.GetFocalPoint(), + camera.GetParallelProjection(), + camera.GetViewAngle(), + camera.GetParallelScale(), + )) + ) + + renderer.sync_camera(VisorCameraState( + position=[1.0, 2.0, 3.0], + focal_point=[4.0, 5.0, 6.0], + view_up=[0.0, 0.0, 1.0], + clipping_range=[7.0, 8.0], + parallel_projection=False, + view_angle=31.0, + parallel_scale=9.0, + )) + + assert recorded == [ + (ACTIVE_CAMERA_WASM_ID, (1.0, 2.0, 3.0), (4.0, 5.0, 6.0), 0, 31.0, 9.0) + ] + + def test_reset_camera_serves_the_reset_camera(self, renderer): + """The camera is re-serialized by its own id, once, after the reset wrote it. + + The VTK renderer is a MagicMock, so ``ResetCamera`` computes nothing; + its side effect writes a hand-written position instead, and that + literal is what must be served. No assertion rests on a value VTK + computed. """ - camera = renderer._vtk_renderer.GetActiveCamera() - renderer._object_manager.GetId.side_effect = ( - lambda obj: ACTIVE_CAMERA_WASM_ID - if obj is camera - else RENDER_WINDOW_WASM_ID - if obj is renderer._render_window - else WRONG_OBJECT_WASM_ID + camera = _install_real_camera(renderer) + camera.SetPosition(51.0, 52.0, 53.0) + renderer._vtk_renderer.ResetCamera.side_effect = ( + lambda bounds: camera.SetPosition(41.0, 42.0, 43.0) + ) + recorded = [] + renderer._object_manager.UpdateStateFromObject.side_effect = lambda object_id: ( + recorded.append((object_id, camera.GetPosition())) ) - renderer.serialize_camera_state() + renderer.reset_camera([0.0, 1.0, 0.0, 1.0, 0.0, 1.0]) + + assert recorded == [(ACTIVE_CAMERA_WASM_ID, (41.0, 42.0, 43.0))] - renderer._object_manager.UpdateStateFromObject.assert_called_with( - ACTIVE_CAMERA_WASM_ID + def test_set_projection_serves_the_written_projection(self, renderer): + """The camera is re-serialized by its own id, once, with the projection already written.""" + camera = _install_real_camera(renderer) + camera.SetParallelProjection(0) + recorded = [] + renderer._object_manager.UpdateStateFromObject.side_effect = lambda object_id: ( + recorded.append((object_id, camera.GetParallelProjection())) ) - def test_serialize_camera_state_does_not_notify_the_client(self, renderer): + renderer.set_projection(True) + + assert recorded == [(ACTIVE_CAMERA_WASM_ID, 1)] + + def test_sync_camera_does_not_notify_the_client(self, renderer): """Serialise without pushing -- the whole reason this is not a flush. - ``flush_wasm_state`` serialises *and* fires the client-side update, + ``LocalView.update`` serialises *and* fires the client-side update, which rebuilds the scene and races the ``set_state`` that follows on - the load path. An implementation written as ``flush_wasm_state()`` - would make the camera current and would look correct here in every - other respect; only this assertion separates them. + the load path. The scene-level no-notify tests stub the renderer's + ``render`` and ``flush_wasm_state`` but not ``LocalView.update``, so a + renderer that called it directly is caught only here. """ - renderer.serialize_camera_state() + renderer.sync_camera(VisorCameraState( + position=[1.0, 2.0, 3.0], + focal_point=[4.0, 5.0, 6.0], + view_up=[0.0, 0.0, 1.0], + clipping_range=[7.0, 8.0], + parallel_projection=False, + view_angle=31.0, + parallel_scale=9.0, + )) renderer._local_view.update.assert_not_called() @@ -1112,9 +1330,8 @@ class _CrossSectionWidgetDouble: ``set_origin`` and ``set_normal``, because what has to be pinned is their order relative to each other and not merely that each happened. - ``plane`` and ``plane_representation`` are two distinct objects, so an id - lookup keyed on identity can tell them apart and a re-serialise that named - the same object twice fails. + ``plane`` and ``plane_representation`` are two distinct objects, as on + the real widget. """ def __init__(self): @@ -1185,41 +1402,40 @@ def test_sync_cross_section_plane_writes_both_vtk_objects(self, renderer, widget ("set_normal", REPORTED_PLANE_NORMAL), ] - # -- serialize_cross_section_state -------------------------------------- + # -- what the client is served after the plane write -------------------- - def test_serialize_cross_section_state_names_the_plane_and_the_representation_ids( - self, renderer, widget - ): - """The re-serialise names exactly two objects: the plane and the handle. + def test_sync_cross_section_plane_serves_the_written_plane(self, renderer): + """The plane and the handle are each re-serialized by their own id, with the plane written. - This is the assertion that pins the increment. The id source is keyed - on object identity, so every object other than those two resolves to a - third, equally distinctive literal; asserting these two therefore - fails -- rather than coincides -- if production names the widget, the - render window or the renderer. + A real ``VisorCrossSectionWidget`` over a bare interactor, so the + plane and the representation are the real VTK objects production + writes through ``set_origin`` and ``set_normal``. The widget's own + ``update_bounds`` leaves a default plane that differs from the + reported one; nothing is asserted on that default. - Revert to a plane-only re-serialise and the call count is 1. That - revert leaves the server correct and the client saving a stale handle, - which no other assertion in this suite sees. - - No render is in this test at all, which is what pins the "never relies - on a render following" clause: the two calls are the whole mechanism. + Two entries, plane then handle. A plane-only re-serialization leaves + the client saving a stale handle; one placed before the writes records + the default plane. """ - renderer._object_manager.GetId.side_effect = ( - lambda obj: CROSS_SECTION_PLANE_WASM_ID - if obj is widget.plane - else CROSS_SECTION_REPRESENTATION_WASM_ID - if obj is widget.plane_representation - else WRONG_OBJECT_WASM_ID - ) + widget = VisorCrossSectionWidget(vtkRenderWindowInteractor()) + widget.update_bounds([-10.0, 10.0, -10.0, 10.0, -10.0, 10.0]) + renderer._cross_section_widget = widget + plane, representation = widget.plane, widget.plane_representation + recorded = _record_served(renderer, { + CROSS_SECTION_PLANE_WASM_ID: ( + plane, lambda: (plane.GetOrigin(), plane.GetNormal()) + ), + CROSS_SECTION_REPRESENTATION_WASM_ID: ( + representation, + lambda: (representation.GetOrigin(), representation.GetNormal()), + ), + }) - renderer.serialize_cross_section_state() + renderer.sync_cross_section_plane(REPORTED_PLANE_ORIGIN, REPORTED_PLANE_NORMAL) - calls = renderer._object_manager.UpdateStateFromObject.call_args_list - assert len(calls) == 2 - assert [call.args[0] for call in calls] == [ - CROSS_SECTION_PLANE_WASM_ID, - CROSS_SECTION_REPRESENTATION_WASM_ID, + assert recorded == [ + (CROSS_SECTION_PLANE_WASM_ID, ((4.0, 5.0, 6.0), (0.0, 1.0, 0.0))), + (CROSS_SECTION_REPRESENTATION_WASM_ID, ((4.0, 5.0, 6.0), (0.0, 1.0, 0.0))), ] # -- update_bounds: the seed -------------------------------------------- diff --git a/tests/unit/renderer/test_null_renderer.py b/tests/unit/renderer/test_null_renderer.py index 41809e93..e92e8aa7 100644 --- a/tests/unit/renderer/test_null_renderer.py +++ b/tests/unit/renderer/test_null_renderer.py @@ -95,30 +95,6 @@ def test_each_renderer_has_its_own_record(): assert second.get_camera_state() is None -# =========================================================================== -# Publishing is not recording -# =========================================================================== - -def test_serialize_camera_state_is_a_no_op_and_leaves_the_record_alone(): - """This renderer serves the client nothing, so it has nothing to refresh. - - Two things are asserted together because the method has exactly two ways - to be wrong here. It must not raise -- a bare ``pass`` inherited by - accident would satisfy that alone -- and it must not touch the record. - Publishing and recording are separate obligations: the writers of the - record are ``reset_camera`` and ``sync_camera``, and this is neither. An - implementation that cleared the record on serialise would be a silent data - loss on the renderer used to stand in for a second backend, and no other - test in the tree would notice. - """ - renderer = NullRenderer() - cam = _camera_state() - renderer.sync_camera(cam) - - renderer.serialize_camera_state() - - assert renderer.get_camera_state() is cam - # =========================================================================== # Projection: the record half owed in full, the pipeline half not at all diff --git a/tests/unit/vtk/scene/test_base.py b/tests/unit/vtk/scene/test_base.py index 620e362e..7b6b4b5a 100644 --- a/tests/unit/vtk/scene/test_base.py +++ b/tests/unit/vtk/scene/test_base.py @@ -1697,35 +1697,20 @@ def test_sync_camera_projects_the_camera_onto_the_pipeline(scene): 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. +def test_sync_camera_serializes_the_camera_exactly_once(scene): + """One re-serialization, the renderer's own, carrying the active camera'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. + The renderer's ``sync_camera`` leaves its served state current; the + coordinator adds no re-serialization of its own. A coordinator that + still re-serialized after the renderer call records the id twice. What + the client is served is pinned in tests/unit/renderer/test_local_renderer.py. """ - 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)) - ) + serialized = [] + scene._renderer._object_manager.UpdateStateFromObject = serialized.append scene.sync_camera(_gesture_camera()) - assert order == ["camera", ("serialize", ACTIVE_CAMERA_WASM_ID)] + assert serialized == [ACTIVE_CAMERA_WASM_ID] def test_sync_camera_holds_the_lock_across_both_halves(scene): @@ -1788,45 +1773,26 @@ def test_sync_camera_does_not_notify_the_client(scene): # reset_camera -- the re-serialisation # # ``VisorSceneBase.reset_camera`` calls ``self._renderer.reset_camera(...)`` -# then ``self._renderer.serialize_camera_state()``, both inside -# ``_vtk_lock``. Same shape as the ``sync_camera`` trigger section above: the -# re-serialisation runs after the reset, is spied on -# ``_object_manager.UpdateStateFromObject`` with the active-camera id, and -# the lock is held (depth >= 1) at both points. -# -# Own literals are unnecessary here -- reset_camera takes no camera argument, -# only the scene-graph bounds -- so what is pinned is order and lock depth, -# not a value. +# inside ``_vtk_lock``, and the renderer's reset leaves its served state +# current on return. Same shape as the ``sync_camera`` trigger section +# above: the coordinator adds no re-serialization of its own, so exactly one +# is spied on ``_object_manager.UpdateStateFromObject`` with the +# active-camera id, and the lock is held (depth >= 1) at both points. # =========================================================================== -def test_reset_camera_serializes_after_the_reset_with_the_active_camera_id(scene): - """The re-serialisation follows the reset, and carries production's id. - - Reverted -- the reset kept and the re-serialisation dropped -- the - pipeline camera is right and every other test in this module still - passes, while the client is served the pre-reset camera on its next - fetch. +def test_reset_camera_serializes_the_camera_exactly_once(scene): + """One re-serialization, the renderer's own, carrying the active camera's id. - The spy appends ``"reset"`` for the reset call and the two-tuple - ``("serialize", )`` for the re-serialisation; the tuple is the - recording format, not the argument. ```` is asserted as the bare id - production passes, since ``UpdateStateFromObject`` takes a single id. + A coordinator that still re-serialized after the renderer call records + the id twice. What the client is served is pinned in + tests/unit/renderer/test_local_renderer.py. """ - order = [] - real_reset = scene._renderer.reset_camera - - def _reset(bounds): - order.append("reset") - return real_reset(bounds) - - scene._renderer.reset_camera = _reset - scene._renderer._object_manager.UpdateStateFromObject = ( - lambda object_id: order.append(("serialize", object_id)) - ) + serialized = [] + scene._renderer._object_manager.UpdateStateFromObject = serialized.append scene.reset_camera() - assert order == ["reset", ("serialize", ACTIVE_CAMERA_WASM_ID)] + assert serialized == [ACTIVE_CAMERA_WASM_ID] def test_reset_camera_holds_the_lock_across_both_halves(scene): @@ -2609,38 +2575,20 @@ def _projection_save_scene(scene, record): # The coordinator # --------------------------------------------------------------------------- -def test_set_projection_applies_to_the_renderer_and_then_serializes(scene): - """The re-serialisation follows the write, and carries production's id. +def test_set_projection_serializes_the_camera_exactly_once(scene): + """One re-serialization, the renderer's own, carrying the active camera's id. - Reverted -- the write kept and the re-serialisation dropped -- the record - is right, the pipeline camera is right, and the client is served the - pre-toggle camera on its next fetch. - - Also pins *where* the re-serialisation lives. Written inside the - renderer's own ``set_projection`` instead of here, the lock probe below - would still pass; this spy sits on the coordinator's two calls, so a - renderer that serialised for itself would record the pair in the wrong - order or twice. - - The spy appends ``("set_projection", )`` for the write and the - two-tuple ``("serialize", )`` for the re-serialisation; the tuple is - the recording format, not the argument. + The renderer's ``set_projection`` leaves its served state current; the + coordinator adds no re-serialization of its own. A coordinator that + still re-serialized after the renderer call records the id twice. What + the client is served is pinned in tests/unit/renderer/test_local_renderer.py. """ - order = [] - real_set = scene._renderer.set_projection - - def _set(parallel): - order.append(("set_projection", parallel)) - return real_set(parallel) - - scene._renderer.set_projection = _set - scene._renderer._object_manager.UpdateStateFromObject = ( - lambda object_id: order.append(("serialize", object_id)) - ) + serialized = [] + scene._renderer._object_manager.UpdateStateFromObject = serialized.append scene.set_projection(PROJECTION_ON) - assert order == [("set_projection", True), ("serialize", ACTIVE_CAMERA_WASM_ID)] + assert serialized == [ACTIVE_CAMERA_WASM_ID] def test_set_projection_holds_the_lock_across_both_halves(scene): @@ -2650,7 +2598,7 @@ def test_set_projection_holds_the_lock_across_both_halves(scene): the story that catches the quiet failure. Three reverts, three distinct signatures: - * drop ``serialize_camera_state()`` -> ``serialize_depth`` is never + * drop the renderer's re-serialization -> ``serialize_depth`` is never recorded and this fails on the missing key; * move it below the ``with`` block -> ``serialize_depth`` is 0 while ``write_depth`` is 1, so both the non-zero and the equality assertions @@ -2829,39 +2777,24 @@ def _plane_save_scene(scene, record_plane): # The coordinator # --------------------------------------------------------------------------- -def test_sync_cross_section_plane_applies_to_the_renderer_and_then_serializes(scene): - """The re-serialisation follows the write, and there are two of them. +def test_sync_cross_section_plane_serializes_the_plane_exactly_twice(scene): + """Two re-serializations, the renderer's own: the plane and the handle. - Reverted -- the write kept and the re-serialisation dropped -- the record - is right, the server's own plane and representation are right, every other - test in this module still passes, and the client is served the pre-drag - plane on its next fetch. The user sees a page reload snap the plane back - to where it was before they dragged it. - - Two serialise calls, not one: ``serialize_cross_section_state`` names the - plane and the representation separately. Which object each names is - pinned in tests/unit/renderer/test_local_renderer.py, against a widget - double whose two objects have distinct ids; what is pinned here is that - the coordinator reaches that method at all, and reaches it after the - write. + The renderer's ``sync_cross_section_plane`` names the plane and the + representation separately and leaves its served state current; the + coordinator adds no re-serialization of its own. A coordinator that + still re-serialized after the renderer call records four. Which object + each names, and what the client is served, is pinned in + tests/unit/renderer/test_local_renderer.py against a real widget. """ - order = [] - real_sync = scene._renderer.sync_cross_section_plane - - def _sync(origin, normal): - order.append("plane") - return real_sync(origin, normal) - - scene._renderer.sync_cross_section_plane = _sync - scene._renderer._object_manager.UpdateStateFromObject = ( - lambda object_id: order.append("serialize") - ) + serialized = [] + scene._renderer._object_manager.UpdateStateFromObject = serialized.append scene.sync_cross_section_plane( GESTURE_CROSS_SECTION_ORIGIN, GESTURE_CROSS_SECTION_NORMAL ) - assert order == ["plane", "serialize", "serialize"] + assert len(serialized) == 2 def test_sync_cross_section_plane_holds_the_lock_across_both_halves(scene):