[Don't merge] Frontend api cherrypick fix for release 25.10.0 #8971
Conversation
…visibility changed done while still loading the mesh (#8936) This PR fixes the Frontend mesh visibility setting function of the API. Some PR in the past added an additional nesting level to the meshes structure to support additional coordinates. This was not adjusted in the frontend mesh visiblity api function. The function itself has a check for whether the mesh is present but was missing this additional nesting level. This PR fixes this. The correct nesting structure is shown in: https://github.com/scalableminds/webknossos/blob/master/frontend/javascripts/viewer/model/reducers/annotation_reducer.ts#L420-L432 While working on this, I thought it would be nice to be able to script something that does some additional followup actions once the mesh is loaded. But the mesh loading function does not return a promise which resolves when the mesh is loaded. Therefore to add generality, I added a utility function to the frontend api that enables us to wait for a specific action. E.g. the mesh loading has a specific "finished" action which could be used in the case I described. Please feel free to tell me what you think about this little addition :) ### URL of deployed dev instance (used for testing): - https://___.webknossos.xyz ### Steps to test: - View a DS with a mesh file - Select the mesh file and its corresponding agglomerate view if needed - Locate a segment with its ID - Insert the mesh id and position in the following script and run it. ```javascript window.webknossos.apiReady(3).then(async (api) => { const segmentIdsAndPositions = [ [<mesh_id>, [<mesh_pos>]], ]; const sleep = (ms) => new Promise(resolve => setTimeout(resolve, ms)); for (const [segmentId, position] of segmentIdsAndPositions) { api.data.loadPrecomputedMesh(segmentId, position); api.data.setMeshVisibility(segmentId, false); } await sleep(5*1000); for (const [segmentId, position] of segmentIdsAndPositions) { api.data.setMeshVisibility(segmentId, true); } await sleep(5*1000); for (const [segmentId, position] of segmentIdsAndPositions) { api.data.removeMesh(segmentId); } await sleep(5*1000); for (const [segmentId, position] of segmentIdsAndPositions) { api.data.loadPrecomputedMesh(segmentId, position); } await sleep(5*1000); for (const [segmentId, position] of segmentIdsAndPositions) { api.data.resetMeshes(); } }); ``` - This should not crash (see console). Watch the 3d viewport to observe correctness. 1. Load mesh invisible 2. make visible again 3. remove mesh 4. load mesh again (visible) 5. remove all meshes - Optional: Test on master -> shouldn't work -> the `setMeshVisibility` part should print an error in the console. ### Issues: - fixes https://scm.slack.com/archives/C3PBDFGC8/p1758101435301069 ------ (Please delete unneeded items, merge only when none are left open) - [x] Added changelog entry (create a `$PR_NUMBER.md` file in `unreleased_changes` or use `./tools/create-changelog-entry.py`)
📝 WalkthroughWalkthroughAdds release notes for 25.10.0 and updates migrations ordering. Removes multiple unreleased changelog entries. Refactors frontend mesh APIs to key meshes by additionalCoordinates and use a single state snapshot. Adjusts mesh creation flow to initialize visibility from state and pass cached state to tracing. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/javascripts/viewer/api/api_latest.ts (2)
2578-2597: Consider improving the error message specificity.The error message at line 2594 states "Mesh for segment X was not found in State.localSegmentationData" but doesn't include the layer name or additional coordinates context. This could make debugging harder in ND datasets where meshes are keyed by additional coordinates.
Consider updating the error message:
throw new Error( - `Mesh for segment ${segmentId} was not found in State.localSegmentationData.`, + `Mesh for segment ${segmentId} in layer "${effectiveLayerName}" at additional coordinates "${additionalCoordKey}" was not found in State.localSegmentationData.`, );
2606-2625: Apply the same error message improvement here.Similar to
setMeshVisibility, the error message could include layer name and additional coordinates for better debugging context.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
CHANGELOG.released.md(1 hunks)MIGRATIONS.released.md(2 hunks)frontend/javascripts/viewer/api/api_latest.ts(4 hunks)frontend/javascripts/viewer/controller/segment_mesh_controller.ts(1 hunks)unreleased_changes/8796.md(0 hunks)unreleased_changes/8824.md(0 hunks)unreleased_changes/8850.md(0 hunks)unreleased_changes/8865.md(0 hunks)unreleased_changes/8876.md(0 hunks)unreleased_changes/8882.md(0 hunks)unreleased_changes/8885.md(0 hunks)unreleased_changes/8888.md(0 hunks)unreleased_changes/8895.md(0 hunks)unreleased_changes/8896.md(0 hunks)unreleased_changes/8901.md(0 hunks)unreleased_changes/8903.md(0 hunks)unreleased_changes/8909.md(0 hunks)unreleased_changes/8911.md(0 hunks)unreleased_changes/8915.md(0 hunks)unreleased_changes/8919.md(0 hunks)unreleased_changes/8925.md(0 hunks)unreleased_changes/8926.md(0 hunks)unreleased_changes/8927.md(0 hunks)unreleased_changes/8936.md(1 hunks)
💤 Files with no reviewable changes (19)
- unreleased_changes/8888.md
- unreleased_changes/8850.md
- unreleased_changes/8919.md
- unreleased_changes/8885.md
- unreleased_changes/8925.md
- unreleased_changes/8824.md
- unreleased_changes/8926.md
- unreleased_changes/8927.md
- unreleased_changes/8796.md
- unreleased_changes/8896.md
- unreleased_changes/8882.md
- unreleased_changes/8865.md
- unreleased_changes/8901.md
- unreleased_changes/8915.md
- unreleased_changes/8903.md
- unreleased_changes/8909.md
- unreleased_changes/8911.md
- unreleased_changes/8895.md
- unreleased_changes/8876.md
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/javascripts/viewer/controller/segment_mesh_controller.ts (1)
frontend/javascripts/viewer/model/accessors/volumetracing_accessor.ts (1)
getActiveSegmentationTracing(342-342)
frontend/javascripts/viewer/api/api_latest.ts (3)
frontend/javascripts/viewer/model/accessors/volumetracing_accessor.ts (1)
getRequestedOrVisibleSegmentationLayerEnforced(366-380)frontend/javascripts/viewer/model/accessors/flycam_accessor.ts (1)
getAdditionalCoordinatesAsString(288-298)frontend/javascripts/viewer/constants.ts (1)
EMPTY_OBJECT(392-392)
🔇 Additional comments (7)
unreleased_changes/8936.md (2)
4-5: LGTM!The documented mesh API fixes align with the code changes in
api_latest.tsandsegment_mesh_controller.ts. The changes correctly address:
- State-based mesh access using
additionalCoordinatesAsString- Visibility initialization when meshes are added
- Consistent state snapshot usage across mesh operations
1-2: Confirm presence of frontend action-reaction function
I didn’t find this new function in the PR; ensure it’s implemented or remove the changelog entry.frontend/javascripts/viewer/controller/segment_mesh_controller.ts (2)
272-272: Good use of captured state snapshot.Passing the locally captured
statetogetActiveSegmentationTracingensures consistency with the visibility check above and aligns with the refactoring pattern used inapi_latest.ts.
264-270: Guard missing metadata before.isVisiblelookup.Initialize or verify the entry at
state.localSegmentationData[layerName]?.meshes?.[additionalCoordinatesString]?.[segmentId]before reading.isVisible, as the?? truefallback can override a previously hidden mesh.frontend/javascripts/viewer/api/api_latest.ts (3)
2578-2597: Good refactoring for consistent state-based mesh access.The single state read and use of
getAdditionalCoordinatesAsStringfor keying provides consistency with the mesh controller changes and properly supports ND datasets.
2634-2650: LGTM! Good use of EMPTY_OBJECT fallback.The refactoring correctly uses
EMPTY_OBJECTas a safe fallback when the mesh data doesn't exist for the given additional coordinates, preventing potential errors while maintaining the same behavior.
60-60: Good addition of required import.The
getAdditionalCoordinatesAsStringimport is necessary for the new mesh keying pattern and is correctly imported from the flycam accessor.
|
can be closed, right? |
Don't merge. Cherry-picked #8936 into release 25.10.0.