Set mesh opacity via frontend API #9011
Conversation
📝 WalkthroughWalkthroughThe DataApi.setSegmentColor(...) method was extended with an optional Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 0
🧹 Nitpick comments (2)
frontend/javascripts/viewer/api/api_latest.ts (2)
2758-2765: Consider enhancing the JSDoc for clarity.The JSDoc mentions "if the mesh is loaded" but doesn't explain what happens when the mesh isn't loaded. Additionally, documenting the expected value range for
meshOpacity(typically 0-1) would help API users.Apply this diff to improve the documentation:
/** * Set the RGB color of a segment (and its mesh) for a given segmentation layer. If layerName is not passed, - * the currently visible segmentation layer will be used. If the mesh is loaded, optionally update its opacity. + * the currently visible segmentation layer will be used. If meshOpacity is provided and a mesh is loaded for this + * segment, the mesh opacity will be updated (valid range: 0-1, where 0 is fully transparent and 1 is fully opaque). + * If no mesh is loaded, the opacity parameter is silently ignored. * * @example * api.data.setSegmentColor(3, [0, 1, 1]); + * + * @example + * // Set color and mesh opacity together + * api.data.setSegmentColor(3, [0, 1, 1], "segmentation", 0.5); */ setSegmentColor(segmentId: number, rgbColor: Vector3, layerName?: string, meshOpacity?: number) {
2782-2785: Add validation for meshOpacity parameter range.The
setSegmentColorAPI accepts an optionalmeshOpacityparameter (typenumber), but no validation exists to ensure it falls within the expected 0-1 range for Three.js material opacity. While the reducer gracefully handles cases where the mesh doesn't exist, adding range validation at the API entry point would prevent invalid values from propagating through the action-reducer-saga chain.Consider applying this diff:
if (meshOpacity != null) { + if (meshOpacity < 0 || meshOpacity > 1) { + throw new Error(`meshOpacity must be between 0 and 1, but received ${meshOpacity}`); + } Store.dispatch(updateMeshOpacityAction(effectiveLayerName, segmentId, meshOpacity)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/viewer/api/api_latest.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/javascripts/viewer/api/api_latest.ts (2)
frontend/javascripts/viewer/constants.ts (1)
Vector3(14-14)frontend/javascripts/viewer/model/actions/annotation_actions.ts (1)
updateMeshOpacityAction(236-242)
🔇 Additional comments (1)
frontend/javascripts/viewer/api/api_latest.ts (1)
100-100: LGTM!The import of
updateMeshOpacityActionis correctly added and matches the action creator definition in the annotation actions module.
|
@MichaelBuessemeyer I wasnt 100% sure about error handling– on the one hand, the segment ID is not validated, but the existence of the layer is. So I didnt understand whether we assume that the mesh is loaded. Let me know if you would like me to change that! (Although there isnt much explicit error handling in the segment_mesh_contoller, where I would have placed it...) |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/javascripts/viewer/api/api_latest.ts(3 hunks)unreleased_changes/9011.md(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/javascripts/viewer/api/api_latest.ts (2)
frontend/javascripts/viewer/constants.ts (1)
Vector3(14-14)frontend/javascripts/viewer/model/actions/annotation_actions.ts (1)
updateMeshOpacityAction(236-242)
⏰ 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)
- GitHub Check: frontend-tests
- GitHub Check: backend-tests
- GitHub Check: build-smoketest-push
🔇 Additional comments (3)
unreleased_changes/9011.md (1)
1-2: LGTM! Clear changelog entry.The changelog accurately documents the new optional
meshOpacityparameter added to the public API.frontend/javascripts/viewer/api/api_latest.ts (2)
100-100: LGTM! Import correctly added.The
updateMeshOpacityActionimport is properly added to the existing import statement.
2758-2765: LGTM! Backwards-compatible API extension.The optional
meshOpacityparameter is properly added to the function signature with updated documentation and example usage. The change maintains backwards compatibility.
|
|
||
| if (meshOpacity != null) { | ||
| Store.dispatch(updateMeshOpacityAction(effectiveLayerName, segmentId, meshOpacity)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Consider opacity validation and mesh existence check.
The implementation dispatches updateMeshOpacityAction without:
- Validating that
meshOpacityis in a valid range (typically 0-1 for opacity values) - Checking whether a mesh is loaded for the given segment
Per your PR comment, you noted uncertainty about whether to assume the mesh is loaded. For consistency with other mesh methods in this API (e.g., setMeshVisibility at lines 2579-2598 checks mesh existence and throws an error), consider similar validation here.
Run this script to compare error handling patterns in other mesh-related methods:
Additionally, consider validating the opacity range:
if (meshOpacity != null) {
+ if (meshOpacity < 0 || meshOpacity > 1) {
+ throw new Error(`meshOpacity must be between 0 and 1, but got ${meshOpacity}`);
+ }
Store.dispatch(updateMeshOpacityAction(effectiveLayerName, segmentId, meshOpacity));
}🏁 Script executed:
#!/bin/bash
# Check how other mesh methods handle mesh existence validation
rg -nP -A10 'setMeshVisibility|removeMesh' --type=ts frontend/javascripts/viewer/api/api_latest.tsLength of output: 2625
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Extract complete implementation context for updateMeshOpacity and setMeshVisibility
echo "=== setMeshVisibility implementation (lines 2579-2610) ==="
sed -n '2579,2610p' frontend/javascripts/viewer/api/api_latest.ts
echo ""
echo "=== updateMeshOpacity context (lines 2775-2790) ==="
sed -n '2775,2790p' frontend/javascripts/viewer/api/api_latest.ts
echo ""
echo "=== Check for opacity validation patterns in file ==="
rg -n 'opacity.*[0-1]|opacity.*range|opacity.*clamp|opacity.*min|opacity.*max' --type=ts frontend/javascripts/viewer/api/api_latest.ts | head -20Length of output: 1659
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Find the method that contains updateMeshOpacity at line 2782
echo "=== Method containing updateMeshOpacity (searching backwards from line 2782) ==="
sed -n '2700,2790p' frontend/javascripts/viewer/api/api_latest.ts | grep -n 'setMeshOpacity\|^\s*[a-zA-Z].*(\s*[a-zA-Z].*:\s*' | tail -5
echo ""
echo "=== Full context from line 2760 to 2790 to see method signature ==="
sed -n '2760,2790p' frontend/javascripts/viewer/api/api_latest.tsLength of output: 1354
Add mesh existence check and opacity validation in setSegmentColor method.
The implementation at line 2782-2785 in setSegmentColor dispatches updateMeshOpacityAction without checking mesh existence or validating the opacity value. This is inconsistent with other mesh methods like setMeshVisibility (line 2579) and removeMesh (line 2607), which both validate mesh existence and throw an error if the mesh is not found.
Add both checks:
- Validate that
meshOpacityis in the valid range (0-1) - Check that the mesh exists before dispatching, consistent with the established pattern
if (meshOpacity != null) {
+ if (meshOpacity < 0 || meshOpacity > 1) {
+ throw new Error(`meshOpacity must be between 0 and 1, but got ${meshOpacity}`);
+ }
+ const state = Store.getState();
+ const additionalCoordinates = state.flycam.additionalCoordinates;
+ const additionalCoordKey = getAdditionalCoordinatesAsString(additionalCoordinates);
+ if (state.localSegmentationData[effectiveLayerName]?.meshes?.[additionalCoordKey]?.[segmentId] != null) {
Store.dispatch(updateMeshOpacityAction(effectiveLayerName, segmentId, meshOpacity));
+ }
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/api/api_latest.ts around lines 2782 to 2785, the
code dispatches updateMeshOpacityAction without validating meshOpacity or
checking that the mesh exists; update it to first verify meshOpacity is a number
between 0 and 1 (inclusive) and throw an error or reject if out of range, then
check for the mesh's existence for the effectiveLayerName and segmentId (same
pattern as setMeshVisibility/removeMesh) and throw a descriptive error if not
found, and only call Store.dispatch(updateMeshOpacityAction(...)) when both
validations pass.
There was a problem hiding this comment.
@MichaelBuessemeyer I wasnt 100% sure about error handling– on the one hand, the segment ID is not validated, but the existence of the layer is. So I didn't understand whether we assume that the mesh is loaded. Let me know if you would like me to change that! (Although there isn't much explicit error handling in the segment_mesh_contoller, where I would have placed it...)
I'd say the segmentMeshController.setMeshOpacity in common_mesh_saga.tsx is safe.
But in the annotation reducer case "UPDATE_MESH_OPACITY" does not check whether the id exists in the dict and simply adds it 🤔. Also, setting the opacity of a not loaded mesh might hint at an error in the script. So maybe acutally throwing an error as suggested by the AI might make sense here.
Testing worked 🎉
Except that no error is thrown.
While adapting the code suggestion by the AI, could you please not double use the Store.getState in this function and instead reuse const state = Store.getState() 😇? (beeing 🐝 picky here)
Steps to test:
Issues:
(Please delete unneeded items, merge only when none are left open)
$PR_NUMBER.mdfile inunreleased_changesor use./tools/create-changelog-entry.py)Adapted wk-libs python client if relevant API parts change-> not needed, as the new function is backwards-compatible