Skip to content

feat: [Remote rendering 3.2a] server-tracked camera: record, load-path write, and re-serialization - #110

Open
LKasianAnsys wants to merge 10 commits into
mainfrom
feat/3.2a-server-tracked-camera
Open

LKasianAnsys wants to merge 10 commits into
mainfrom
feat/3.2a-server-tracked-camera

Conversation

@LKasianAnsys

@LKasianAnsys LKasianAnsys commented Sep 14, 2026

Copy link
Copy Markdown
Collaborator

Issue

Addresses #20

Context

This is the first of 4 PRs for user story 3.2 (#20) of the phased implementation plan (ADR here) for adding remote rendering in VISOR.

Until now the server never applied camera state to its copy of the VTK pipeline: the browser owned it, and when save_state was called, the server had to fetch it via a round trip to the client, as its own pipeline was stale.

This PR adds a camera record to each renderer, populates it from the loaded state in apply_state, and applies it to the pipeline camera via sync_camera. It also adds serialize_camera_state, called immediately after sync_camera inside the lock. That call re-serializes the render window's object graph so that the client receives the camera that was just written rather than the copy before the load. Without it, the loaded camera is correct on the server and then replaced on the canvas by the stale copy. See the note below; this is not specific to the camera and impacts the following stories in the remote rendering Phase 3 work.

To verify: load a saved state with an off-axis camera and refresh. The framing should persist. To see the failure mode, comment out the serialize_camera_state() call in apply_state and load again.

Nothing reads the record yet. reset_camera writes it, and the save path still uses the client round trip; that changes in PR 3.2b (#311). Gesture attribution on the client is in PR 3.2c, and the report back to the server is added in PR 3.2d.

Regression test xfail

The test_load_state_into_empty_scene regression test was marked xfail(run=False) on Linux. It started failing on this branch, but investigation showed that load_state into an empty scene has never rendered on Linux (#122, reproduced on main and the old tree on the internal repo before the remote rendering work). The failure was not visible before this branch. The test is not run on Linux, rather than expected to fail, because the faulty load leaves the shared server unusable for the tests that follow it. This will be revisited in #24, when the full save/load state workflow is verified on top of the server-authoritative viewer state.


Copilot summary

This pull request significantly refactors and documents the camera state management contract for renderers, clarifying the roles of the pipeline camera and the camera record, and ensuring consistent, testable behavior across both production and test implementations. It also introduces comprehensive tests for these behaviors.

Camera state management improvements:

  • The IRenderer interface now clearly distinguishes between the pipeline camera (the VTK camera object) and the camera record (the authoritative state), with detailed docstrings explaining the contract for reset_camera, get_camera_state, sync_camera, and the new serialize_camera_state method.
  • The NullRenderer now maintains a camera record, implementing the record half of the contract even though it has no pipeline camera, and provides detailed documentation for each method. [1] [2] [3]

VisorLocalRenderer implementation updates:

  • Implements the clarified contract: reset_camera writes the pipeline camera and then updates the record from it; sync_camera stores the given object and projects it onto the pipeline camera; serialize_camera_state refreshes the serialization cache for the camera without notifying the client. Private helpers _read_pipeline_camera and _apply_to_pipeline_camera are added for clarity and testability.

Scene loading and camera state:

  • When loading a persisted scene, the loaded camera is now written to the record and projected onto the pipeline camera before rendering, and the serialization cache is refreshed to ensure the client receives the correct state.

Testing improvements:

  • Introduces a hand-written camera double (_CameraDouble) for unit tests, ensuring that camera state read/write logic is exercised with controlled, non-default values, and that setter calls can be asserted for both order and value.
  • Updates the renderer fixture to use the camera double, ensuring tests are meaningful and not vacuously passing due to default values.

Imports and type hints:

  • Moves the VisorCameraState import out of TYPE_CHECKING blocks to allow runtime use in both production and test code. [1] [2] [3]

These changes together ensure that camera state is handled consistently, robustly, and transparently across the codebase, and that both production and test code reflect the same contract for camera management.

@github-actions github-actions Bot added test Work associated with testing added enhancement New feature or request labels Sep 14, 2026
@LKasianAnsys
LKasianAnsys force-pushed the feat/3.2a-server-tracked-camera branch 5 times, most recently from 60c8952 to f3da08b Compare September 15, 2026 03:56
@github-actions github-actions Bot added the maintenance Operation not directly changing the production code - e.g., updating a devops pipeline label Sep 15, 2026
@LKasianAnsys
LKasianAnsys force-pushed the feat/3.2a-server-tracked-camera branch from 0e76bfb to 88d9ee9 Compare September 15, 2026 16:20
@github-actions github-actions Bot removed the maintenance Operation not directly changing the production code - e.g., updating a devops pipeline label Sep 15, 2026
@LKasianAnsys
LKasianAnsys force-pushed the feat/3.2a-server-tracked-camera branch 2 times, most recently from d7298e1 to 500a079 Compare September 16, 2026 17:20
@LKasianAnsys
LKasianAnsys force-pushed the feat/3.2a-server-tracked-camera branch from 500a079 to d5b9247 Compare September 16, 2026 17:49
@LKasianAnsys
LKasianAnsys force-pushed the feat/3.2a-server-tracked-camera branch from 7a76263 to a4b1c6b Compare September 16, 2026 18:13

@ansBAkula ansBAkula left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LKasianAnsys Cool stuff! I tested it, and it seems to be working as described.

Just one comment regarding the apply_state method naming. The method does more than simply apply/load a state; it also restores and synchronizes the server camera from the previously saved camera state, making the server authoritative for camera settings. Given that broader responsibility, the current name may not fully reflect its behavior.

A more descriptive name might make sense here. What do you think?

@LKasianAnsys LKasianAnsys left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ansBAkula Thanks for testing it! And yes I see what you mean on apply_state, it was structured in a way that was a bit confusing. Added a reply inline and made a small change to the method structure - let me know what you think!

# One call per state class: updates the server's stored state and its VTK objects.
self._restore_part_states(runtime_app_state)
self._restore_camera_state(runtime_app_state)
# TODO: restore widget state, UI state, and variable states when they are synced back to the server.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ansBAkula yes this is a nice callout, on apply_state, and it's a good time for it because the rest of the stories in the state inversion work (phase 3) will build on this.

The method did read awkwardly. In story 3.1, the per-part visual state was made server-authoritative, and it added restore_part_states_from_runtime here. Then this PR had added the camera sync and restore logic, but it was inline rather than in its own similar method, so it wasn't clear that the per-part and camera state were being treated on par. After this user story, there are three more that will also be adding their own state to the list (widget state, UI state, and variable state).

I made a couple changes in this commit to make it more readable and to hopefully clarify, including putting the camera state restore/sync into its own method.

Once all part of the state are covered by the end of Phase 3, it will look like:

            runtime_app_state = self._state_mapper.persisted_to_runtime(state)

            # One call per state class: updates the server's stored state and its VTK objects.
            self._restore_part_states(runtime_app_state)
            self._restore_camera_state(runtime_app_state)
            self._restore_widget_state(runtime_app_state) # to be added in 3.3
            self._restore_ui_state(runtime_app_state) # to be added in 3.4
            self._restore_camera_state(runtime_app_state) # to be added in 3.5


            # The server's copy is now current; deliver it to the rendering backend.
            # wasm: set_state() to the browser; RCA: a rendered frame; headless: no-op.
            self._push_runtime_state(runtime_app_state)

To try to help clarify I renamed the following:

  • _restore_part_states_from_runtime -> _restore_part_states, We're in apply_state applying the persisted state, so the 'from_runtime' was confusing (even though it was technically true, it was applying the persisted state as converted to runtime). Dropped the suffix.
  • _apply_runtime_state_to_render -> _push_runtime_state. This better describes what this method does. It runs through the renderer. On the wasm path it pushes the state to the client. (RCA would render a frame)

Let me know if this makes sense to you. Open to feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

added enhancement New feature or request test Work associated with testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants