feat: [Remote rendering 3.2a] server-tracked camera: record, load-path write, and re-serialization - #110
feat: [Remote rendering 3.2a] server-tracked camera: record, load-path write, and re-serialization#110LKasianAnsys wants to merge 10 commits into
Conversation
60c8952 to
f3da08b
Compare
0e76bfb to
88d9ee9
Compare
d7298e1 to
500a079
Compare
500a079 to
d5b9247
Compare
7a76263 to
a4b1c6b
Compare
ansBAkula
left a comment
There was a problem hiding this comment.
@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
left a comment
There was a problem hiding this comment.
@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. |
There was a problem hiding this comment.
@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 inapply_stateapplying 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!
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_statewas 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 viasync_camera. It also addsserialize_camera_state, called immediately aftersync_camerainside 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 inapply_stateand load again.Nothing reads the record yet.
reset_camerawrites 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_sceneregression test was markedxfail(run=False)on Linux. It started failing on this branch, but investigation showed thatload_stateinto an empty scene has never rendered on Linux (#122, reproduced onmainand 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:
IRendererinterface now clearly distinguishes between the pipeline camera (the VTK camera object) and the camera record (the authoritative state), with detailed docstrings explaining the contract forreset_camera,get_camera_state,sync_camera, and the newserialize_camera_statemethod.NullRenderernow 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]VisorLocalRendererimplementation updates:reset_camerawrites the pipeline camera and then updates the record from it;sync_camerastores the given object and projects it onto the pipeline camera;serialize_camera_staterefreshes the serialization cache for the camera without notifying the client. Private helpers_read_pipeline_cameraand_apply_to_pipeline_cameraare added for clarity and testability.Scene loading and camera state:
Testing improvements:
_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.Imports and type hints:
VisorCameraStateimport out ofTYPE_CHECKINGblocks 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.