Fix multiple Frontend API mesh mesh related functions and apply mesh visibility changed done while still loading the mesh #8936
Conversation
…ibility api - Also add api to wait for a specific action, making async script easier implementable
📝 WalkthroughWalkthroughDataApi and UtilsApi were updated to use per-coordinate mesh maps keyed by additional coordinates; mesh operations (setMeshVisibility, removeMesh, resetMeshes) use a cached state and per-coordinate lookups. Segment mesh controller applies visibility when meshes are newly added. An event-bus wait-for-action utility was exposed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
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 |
|
@philippotto This is a small fix PR. Might be something easy. But feel free to re-assign |
…-mesh-visibility-FE-api
…/webknossos into fix-mesh-visibility-FE-api
MichaelBuessemeyer
changed the title
![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 (1)
frontend/javascripts/viewer/controller/segment_mesh_controller.ts (1)
258-264: Mesh visibility state verified; minor initialization inconsistency
- Verified: WebknossosState.localSegmentationData.meshes is a Record<string, Record<number, MeshInformation> | undefined> and MeshInformation includes isVisible; UPDATE_MESH_VISIBILITY writes to localSegmentationData[layerName].meshes[additionalCoordKey][id].isVisible and segment_mesh_controller reads that same path. (store.ts, annotation_reducer.ts, segment_mesh_controller.ts)
- Issue: maybeAddAdditionalCoordinatesToMeshState initializes meshes[additionalCoordKey] with [] (an array) instead of {} (an object). This works at runtime but diverges from the declared Record<number, MeshInformation> shape — change $set: [] → $set: {} for consistency and clarity. (frontend/javascripts/viewer/model/reducers/annotation_reducer.ts)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/javascripts/viewer/api/api_latest.ts(6 hunks)frontend/javascripts/viewer/controller/segment_mesh_controller.ts(1 hunks)unreleased_changes/8936.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/viewer/api/api_latest.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: knollengewaechs
PR: scalableminds/webknossos#8492
File: frontend/javascripts/viewer/model/sagas/skeletontracing_saga.ts:671-674
Timestamp: 2025-05-15T19:44:16.110Z
Learning: Visibility updates for user bounding boxes are intentionally handled separately from other property updates, with dedicated update actions for visibility changes.
🧬 Code graph analysis (1)
frontend/javascripts/viewer/controller/segment_mesh_controller.ts (1)
frontend/javascripts/viewer/model/accessors/volumetracing_accessor.ts (1)
getActiveSegmentationTracing(342-342)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (2)
unreleased_changes/8936.md (1)
1-6: LGTM!The changelog entry clearly documents both the added functionality and the bug fixes. The description properly covers the addition of the action-waiting function and the fixes to multiple mesh-related API functions.
frontend/javascripts/viewer/controller/segment_mesh_controller.ts (1)
266-266: Good optimization with cached state usage.Using the cached state from line 258 instead of calling
Store.getState()again is a good performance optimization and ensures consistency across the function execution.
|
Thanks for the fix! It's a bit unfortunate that TS did not complain about the incorrect access to the nested structure. Ideally, this should be guarded better, but that's out of scope for this PR probably.
Cool idea! However, I'm not sure whether this is the right direction to tackle the problem. For example, if a script loads several meshes in parallel, there is still no way to wait for a particular mesh to complete, right? Also, api users would need to know about the dispatched action types and also would need to rely on that (even though, it might be subject to change). Therefore, I would remove the utility function again and re-think the problem once there is a priority for it. Do you agree? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/javascripts/viewer/api/api_latest.ts (3)
153-153: Remove unused import (eventBus).Leftover from an earlier approach; it’s not referenced.
-import { eventBus } from "viewer/model/helpers/event_bus";
2608-2626: Per-coordinate removeMesh check is correct; align error message with context.Include layer and coord key for clarity.
- throw new Error( - `Mesh for segment ${segmentId} was not found in State.localSegmentationData.`, - ); + throw new Error( + `Mesh for segment ${segmentId} not found for layer "${effectiveLayerName}" and additionalCoordinates "${additionalCoordKey}".`, + );
2636-2646: Use cached state consistently; consider clarifying scope in docs.
- Use the local state variable when reading meshes to avoid mixing snapshots.
- Optional: Mention that resetMeshes operates on the current additionalCoordinates only.
- const segmentIds = Object.keys( - Store.getState().localSegmentationData[effectiveLayerName]?.meshes?.[additionalCoordKey] || - EMPTY_OBJECT, - ); + const segmentIds = Object.keys( + state.localSegmentationData[effectiveLayerName]?.meshes?.[additionalCoordKey] || + EMPTY_OBJECT, + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/javascripts/viewer/api/api_latest.ts(5 hunks)frontend/javascripts/viewer/controller/segment_mesh_controller.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/viewer/controller/segment_mesh_controller.ts
🧰 Additional context used
🧬 Code graph analysis (1)
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)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (2)
frontend/javascripts/viewer/api/api_latest.ts (2)
60-60: Importing getAdditionalCoordinatesAsString is correct.Matches the new meshes map keying strategy.
2580-2597: Include layer and coordinate key in error message- throw new Error( - `Mesh for segment ${segmentId} was not found in State.localSegmentationData.`, - ); + throw new Error( + `Mesh for segment ${segmentId} not found for layer "${effectiveLayerName}" and additionalCoordinates "${additionalCoordKey}".`, + );Verified that all mesh maps use getAdditionalCoordinatesAsString (with the default ";" separator) for keying.
Yeah, also felt a bit clunky for me. It might be helpful in some rare cases and only seems like a monkeypatch to be honest 👍 Should be removed now |
…/webknossos into fix-mesh-visibility-FE-api
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):
Steps to test:
This should not crash (see console). Watch the 3d viewport to observe correctness.
Optional: Test on master -> shouldn't work -> the
setMeshVisibilitypart should print an error in the console.Issues:
(Please delete unneeded items, merge only when none are left open)
$PR_NUMBER.mdfile inunreleased_changesor use./tools/create-changelog-entry.py)