Reload mesh with opacity #8622
Reload mesh with opacity #8622
Conversation
📝 WalkthroughWalkthroughThis change refactors mesh opacity handling in the viewer by ensuring that mesh opacity is preserved and reapplied when meshes are reloaded. It introduces explicit opacity parameters through mesh loading actions, updates reducers and controllers, and splits mesh-related sagas into specialized modules for ad-hoc and precomputed meshes. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
knollengewaechs
force-pushed
the
reload-mesh-with-opacity
branch
from
2b5ddea to
7245fc1
Compare
frontend/javascripts/viewer/model/actions/segmentation_actions.ts
Outdated
Show resolved
Hide resolved
frontend/javascripts/viewer/model/reducers/annotation_reducer.ts
Outdated
Show resolved
Hide resolved
frontend/javascripts/viewer/model/reducers/annotation_reducer.ts
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
frontend/javascripts/viewer/model_initialization.ts (1)
37-37: Consider consolidating imports from the same module.While this change is functionally correct, having both a named import (
constants) and default import (Constants) from the same module could lead to confusion.Consider refactoring to use a single import pattern if possible:
-import constants, { ControlModeEnum, type Vector3 } from "viewer/constants"; -import Constants from "viewer/constants"; +import Constants, { ControlModeEnum, type Vector3 } from "viewer/constants";Then update the usage on line 517 from
constants.GPU_FACTOR_MULTIPLIERtoConstants.GPU_FACTOR_MULTIPLIERand similar references.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/components/color_picker.tsx(3 hunks)frontend/javascripts/viewer/api/api_latest.ts(1 hunks)frontend/javascripts/viewer/controller/mesh_helpers.ts(2 hunks)frontend/javascripts/viewer/controller/segment_mesh_controller.ts(7 hunks)frontend/javascripts/viewer/model/actions/annotation_actions.ts(5 hunks)frontend/javascripts/viewer/model/actions/segmentation_actions.ts(3 hunks)frontend/javascripts/viewer/model/reducers/annotation_reducer.ts(5 hunks)frontend/javascripts/viewer/model/sagas/mesh_saga.ts(0 hunks)frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts(1 hunks)frontend/javascripts/viewer/model/sagas/meshes/common_mesh_saga.ts(1 hunks)frontend/javascripts/viewer/model/sagas/meshes/precomputed_mesh_saga.ts(1 hunks)frontend/javascripts/viewer/model/sagas/proofread_saga.ts(2 hunks)frontend/javascripts/viewer/model/sagas/root_saga.ts(2 hunks)frontend/javascripts/viewer/model_initialization.ts(2 hunks)frontend/javascripts/viewer/view/context_menu.tsx(2 hunks)frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segments_view.tsx(2 hunks)
💤 Files with no reviewable changes (1)
- frontend/javascripts/viewer/model/sagas/mesh_saga.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
frontend/javascripts/viewer/controller/mesh_helpers.ts (2)
frontend/javascripts/viewer/constants.ts (1)
Vector3(13-13)frontend/javascripts/libs/mjs.ts (1)
V3(398-398)
frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segments_view.tsx (1)
frontend/javascripts/viewer/model/actions/segmentation_actions.ts (1)
loadPrecomputedMeshAction(33-49)
🔇 Additional comments (37)
frontend/javascripts/components/color_picker.tsx (4)
4-4: LGTM: Import addition is necessary.The addition of
useEffectto the React imports is required for the state synchronization functionality added to both color picker components.
40-43: Excellent: Proper state synchronization added.This
useEffectensures the internal state stays synchronized with external prop changes, which is essential for controlled components that maintain internal state for performance reasons (throttling). The implementation is correct with proper dependency management.
52-52: Good fix: Ensures visual consistency.Changing the
HexColorInputto use the internalvaluestate instead of the externalcolorprop ensures visual consistency with theHexColorPicker. Both components now display the same color value, preventing temporary visual inconsistencies during throttled updates.
100-103: Consistent implementation: State sync for RGBA picker.This
useEffectapplies the same state synchronization pattern to the RGBA color picker, ensuring both color picker components behave consistently when external props change. The implementation mirrors the firstuseEffectand follows the same best practices.CHANGELOG.unreleased.md (1)
21-21: LGTM! Well-documented feature addition.The changelog entry clearly documents the new mesh opacity preservation feature and is properly formatted with the correct PR reference.
frontend/javascripts/viewer/view/context_menu.tsx (2)
39-46: LGTM! Import statement updated to support default mesh opacity constant.The import statement correctly switches to include Constants as a default import alongside the existing named imports, enabling access to
Constants.DEFAULT_MESH_OPACITYused later in the file.
965-965: LGTM! Opacity parameter properly integrated into mesh loading.The addition of
Constants.DEFAULT_MESH_OPACITYas the fifth parameter toloadPrecomputedMeshActioncorrectly implements the mesh opacity preservation feature. This ensures that when meshes are loaded from the context menu, they use the default opacity value consistently with the broader mesh opacity handling changes described in the PR.frontend/javascripts/viewer/model/sagas/proofread_saga.ts (2)
14-14: LGTM: Clean import addition for Constants object.The addition of the default
Constantsimport is correct and necessary to accessDEFAULT_MESH_OPACITYused later in the file.
178-187:✅ Verification successful
Verify function signature and constant availability.
The addition of
Constants.DEFAULT_MESH_OPACITYas a parameter aligns well with the PR objective of preserving mesh opacity during reloads. Using a default opacity constant for automatic mesh loading during proofreading is appropriate.Please verify that:
loadPrecomputedMeshActionexpects this opacity parameter in the 5th positionConstants.DEFAULT_MESH_OPACITYexists and has the correct type
🏁 Script executed:
#!/bin/bash # Description: Verify that Constants.DEFAULT_MESH_OPACITY exists and loadPrecomputedMeshAction accepts the opacity parameter # Check if Constants.DEFAULT_MESH_OPACITY is defined echo "Checking Constants.DEFAULT_MESH_OPACITY definition:" ast-grep --pattern 'export default { $$$ DEFAULT_MESH_OPACITY: $_, $$$ }' echo -e "\nChecking loadPrecomputedMeshAction function signature:" # Find the loadPrecomputedMeshAction function definition ast-grep --pattern 'export const loadPrecomputedMeshAction = ($_) => $$$' echo -e "\nAlternative function definition pattern:" ast-grep --pattern 'function loadPrecomputedMeshAction($$$) { $$$ }' echo -e "\nChecking action creator pattern:" ast-grep --pattern 'const loadPrecomputedMeshAction = ($_) => $$$'Length of output: 715
🏁 Script executed:
#!/bin/bash # Search for DEFAULT_MESH_OPACITY to locate its definition echo "Searching for DEFAULT_MESH_OPACITY occurrences:" rg -n "DEFAULT_MESH_OPACITY" echo -e "\nSearching for loadPrecomputedMeshAction occurrences:" rg -n "loadPrecomputedMeshAction"Length of output: 3674
🏁 Script executed:
#!/bin/bash # Display the definition and signature of loadPrecomputedMeshAction echo "=== loadPrecomputedMeshAction definition (with context) ===" rg -C10 "export const loadPrecomputedMeshAction" frontend/javascripts/viewer/model/actions/segmentation_actions.ts # Display the DEFAULT_MESH_OPACITY constant definition echo -e "\n=== DEFAULT_MESH_OPACITY definition ===" sed -n '300,320p' frontend/javascripts/viewer/constants.tsLength of output: 1451
Approved: DEFAULT_MESH_OPACITY correctly matches the action signature.
loadPrecomputedMeshAction’s fifth parameter isopacity: number, and the newConstants.DEFAULT_MESH_OPACITY(defined as1in constants.ts) satisfies this type.- No further updates are needed.
frontend/javascripts/viewer/api/api_latest.ts (1)
2524-2524: LGTM! Consistent opacity parameter addition.The addition of
Constants.DEFAULT_MESH_OPACITYcorrectly aligns with the updatedloadPrecomputedMeshActionsignature and supports the PR's objective of preserving mesh opacity during reload operations.frontend/javascripts/viewer/model/actions/segmentation_actions.ts (2)
10-10: LGTM! Proper optional opacity support for ad-hoc meshes.Adding the optional
opacityproperty toAdHocMeshInfomaintains backward compatibility while enabling opacity specification for ad-hoc meshes.
38-38: LGTM! Correct opacity parameter integration.The required
opacityparameter ensures that precomputed meshes always have an opacity value specified, which aligns with the PR's goal of preserving mesh opacity during reloads.Also applies to: 47-47
frontend/javascripts/viewer/model/sagas/root_saga.ts (2)
24-26: LGTM! Well-organized saga modularization.The refactoring from a single
meshSagato three specialized sagas (adHocMeshSaga,precomputedMeshSaga,commonMeshSaga) improves code organization and maintainability by separating concerns.
73-75: LGTM! Proper saga initialization.All three mesh sagas are correctly called in the
restartableSaga, ensuring they are properly initialized and run concurrently as part of the application's saga middleware.frontend/javascripts/viewer/model/reducers/annotation_reducer.ts (2)
356-365: LGTM: Proper opacity handling for ad hoc meshes.The changes correctly add opacity parameter handling for ad hoc meshes. The direct assignment from the action payload ensures that the mesh's opacity value is preserved in the Redux state.
402-411: LGTM: Appropriate fallback logic for precomputed mesh opacity.The implementation correctly handles opacity for precomputed meshes with a sensible fallback to
Constants.DEFAULT_MESH_OPACITYwhen opacity is not provided. This ensures backward compatibility while supporting the new opacity functionality.frontend/javascripts/viewer/model_initialization.ts (1)
894-894: LGTM: Proper default opacity parameter for precomputed mesh loading.The addition of
Constants.DEFAULT_MESH_OPACITYas a parameter toloadPrecomputedMeshActioncorrectly ensures that meshes loaded during initialization have the default opacity value, maintaining consistency with the new opacity-aware mesh loading flow.frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segments_view.tsx (2)
54-54: LGTM: Import statement updated to support default mesh opacity.The import statement change to include the default
Constantsobject is appropriate for accessingDEFAULT_MESH_OPACITYused in the dispatch function below.
245-252: LGTM: Default mesh opacity added to precomputed mesh loading.The addition of
Constants.DEFAULT_MESH_OPACITYas the opacity parameter ensures that precomputed meshes are loaded with a consistent default opacity value. This aligns with the PR objective of preserving and managing mesh opacity during loading operations.frontend/javascripts/viewer/controller/mesh_helpers.ts (1)
85-92: LGTM: Well-implemented distance sorting utility.The
sortByDistanceTofunction correctly handles bothVector3arrays andMeshChunkarrays by checking for thepositionproperty. The Euclidean distance calculation usingV3.length(V3.sub(...))is appropriate for spatial sorting.The type assertion in the return statement is justified given the function's polymorphic input types.
frontend/javascripts/viewer/model/actions/annotation_actions.ts (2)
330-348: LGTM: Opacity parameter added with backward compatibility.The addition of the optional
opacityparameter with a default value ofConstants.DEFAULT_MESH_OPACITYmaintains backward compatibility while enabling opacity specification for ad-hoc meshes. The parameter is correctly included in the returned action object.
350-368: LGTM: Required opacity parameter for precomputed meshes.The addition of the required
opacityparameter for precomputed meshes is appropriate, as these meshes should have their opacity explicitly specified during loading. This supports the PR objective of preserving mesh opacity during reload operations.frontend/javascripts/viewer/model/sagas/meshes/common_mesh_saga.ts (5)
32-57: LGTM: Robust mesh download implementation with proper error handling.The mesh download functionality correctly retrieves geometry, handles missing meshes with user feedback, and includes comprehensive error handling with both console logging and toast notifications.
93-102: LGTM: STL export includes proper segment metadata.The STL blob generation correctly embeds mesh marker and segment ID metadata using the established
stlMeshConstants, ensuring exported meshes retain their segment information.
124-177: LGTM: Comprehensive additional coordinate handling.The
handleAdditionalCoordinateUpdatefunction properly manages mesh visibility across coordinate changes using a continuous monitoring approach. The while-true loop is appropriate for this use case, and the implementation correctly updates both the Redux store and scene controller.
190-193: LGTM: Opacity change handling supports PR objectives.The
handleMeshOpacityChangefunction directly supports the PR objective of preserving mesh opacity by properly delegating opacity updates to the segment mesh controller.
209-219: LGTM: Well-organized saga with comprehensive event handling.The main saga function properly initializes dependencies and registers handlers for all relevant mesh-related actions, supporting the refactored mesh saga architecture mentioned in the PR objectives.
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts (3)
309-321: LGTM! Proper opacity handling with fallback.The function correctly extracts opacity from
meshExtraInfoand provides a sensible fallback toConstants.DEFAULT_MESH_OPACITY.
494-509: LGTM! Opacity correctly propagated to mesh controller.The opacity value is properly extracted from
meshExtraInfowith a fallback and passed to the mesh controller'saddMeshFromVerticesAsyncmethod.
665-677: Well-structured saga with proper initialization and action handling.The saga correctly:
- Buffers actions until the scene controller and WebKnossos are ready
- Handles all mesh-related actions (load, remove, refresh)
- Tracks edited cells for proper refresh behavior
frontend/javascripts/viewer/controller/segment_mesh_controller.ts (3)
20-20: LGTM! Import path correctly updated for saga refactoring.The import path has been properly updated to reflect the new modular saga structure.
169-184: Excellent opacity animation implementation.The tween animation now correctly animates to the specified opacity value instead of the hardcoded value of 1, properly supporting the mesh opacity feature.
109-136: Consistent opacity parameter propagation.The opacity parameter is properly added to
addMeshFromVerticesAsyncand correctly passed through toaddMeshFromGeometry, maintaining consistency across the mesh construction pipeline.frontend/javascripts/viewer/model/sagas/meshes/precomputed_mesh_saga.ts (4)
56-93: Smart caching implementation using Deferred promises.The caching mechanism efficiently prevents redundant mesh file fetches while properly handling forced refresh scenarios with the
mustRequestflag.
109-135: Proper opacity handling with sensible defaults.The function correctly extracts the opacity from the action and provides a fallback to
Constants.DEFAULT_MESH_OPACITY, ensuring consistent behavior.
405-421: Excellent progressive loading strategy.The eager chunk rendering provides immediate visual feedback while the final merged geometry is being prepared. The opacity parameter is correctly propagated through the mesh addition.
183-217: Comprehensive error handling with user-friendly feedback.The function properly handles various error cases:
- Missing mesh files
- Invalid segment ID (0)
- Mesh listing failures
Each case provides appropriate user feedback via Toast messages while logging detailed information for debugging.
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts
Outdated
Show resolved
Hide resolved
| export function isMeshSTL(buffer: ArrayBuffer): boolean { | ||
| const dataView = new DataView(buffer); | ||
| const isMesh = stlMeshConstants.meshMarker.every( | ||
| (marker, index) => dataView.getUint8(index) === marker, | ||
| ); | ||
| return isMesh; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify usage of the exported isMeshSTL function.
This function is exported but not used within this file. Please verify if it's being used elsewhere in the codebase.
🏁 Script executed:
#!/bin/bash
# Description: Find all usages of the isMeshSTL function across the codebase
# Search for imports and usages of isMeshSTL
rg -A 3 "isMeshSTL" --type ts --type tsxLength of output: 68
🏁 Script executed:
#!/bin/bash
# Re-run search for usages of isMeshSTL across .ts and .tsx files
rg --glob '*.ts' --glob '*.tsx' -n "isMeshSTL"Length of output: 174
Remove unused export isMeshSTL.
The function in
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts:73
is exported but never referenced anywhere else in the repository. Please either:
- Remove this dead code if it’s no longer needed
- Or add the intended import/use where it should be invoked
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts around
lines 73 to 79, the exported function isMeshSTL is not used anywhere in the
codebase. To fix this, either remove the isMeshSTL function entirely if it is no
longer needed, or identify the appropriate place in the code where this function
should be called and add the import and usage accordingly.
✅ Addressed in commit c4f41c3
There was a problem hiding this comment.
@philippotto do you agree that I should remove this?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts (1)
549-561: Past opacity issue has been resolved!The opacity is now properly retrieved from mesh info and passed to
refreshMeshWithMap, addressing the previous critical issue where opacity was not preserved during bulk refresh operations.
🧹 Nitpick comments (2)
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts (2)
53-72: Consider cleanup strategy for unbounded object growth.The
batchCounterPerSegmentobject could potentially grow unbounded as segment IDs accumulate over time without cleanup. While this may be acceptable for typical usage patterns, consider implementing a cleanup strategy for long-running sessions.const batchCounterPerSegment: Record<number, number> = {}; + +// Optional: Add cleanup logic to prevent unbounded growth +// This could be implemented as a periodic cleanup or size-based eviction
461-514: Consider exponential backoff bounds for retry logic.The retry mechanism uses exponential backoff (
2 ** retryCount), but withRETRY_WAIT_TIME = 5000msandMAX_RETRY_COUNT = 5, the final retry could wait up to 160 seconds. Consider if this aligns with user experience expectations.- yield* call(sleep, RETRY_WAIT_TIME * 2 ** retryCount); + // Cap the maximum wait time to prevent excessively long delays + const waitTime = Math.min(RETRY_WAIT_TIME * 2 ** retryCount, 30000); + yield* call(sleep, waitTime);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (6)
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts (6)
1-52: LGTM! Well-organized imports and clear separation of concerns.The imports are comprehensive and properly organized, clearly showing the dependencies for ad-hoc mesh operations. The separation from the original monolithic mesh saga into this specialized module aligns well with the PR objectives.
300-314: Excellent opacity preservation implementation!The opacity handling correctly extracts the value from
meshExtraInfoand provides a proper fallback toConstants.DEFAULT_MESH_OPACITY. This directly addresses the PR requirement to preserve mesh opacity during reloading operations.
484-501: Opacity properly propagated to mesh controller.The opacity value is correctly passed to
addMeshFromVerticesAsyncensuring that mesh chunks maintain their intended opacity throughout the loading process.
580-606: Comprehensive opacity handling for both mesh types.The refresh logic correctly handles opacity preservation for both precomputed meshes (via action dispatch) and ad-hoc meshes (via
refreshMeshWithMapcall), ensuring consistent behavior across different mesh types.
609-660: Robust mesh refresh implementation with proper opacity handling.The
refreshMeshWithMapfunction correctly accepts and uses the opacity parameter throughout the mesh reloading process. The logic properly handles mesh removal, chunk iteration, and opacity preservation in the reload call.
662-673: Well-structured saga with proper initialization sequence.The main saga correctly buffers actions until dependencies are ready and sets up appropriate event handlers for all mesh operations. The use of
actionChannelfor buffering and proper dependency initialization demonstrates good Redux-saga practices.
| } | ||
| } | ||
|
|
||
| function* refreshMeshes(): Saga<void> { |
There was a problem hiding this comment.
this function is not called, because in the segment view, separate refreshMeshActions are dispatched. @philippotto do you know if there is a reason for that? should I change this?
URL of deployed dev instance (used for testing):
This PR improves the reloading of meshes by keeping their previous opacity. It also refactors the code by splitting the mesh_saga into three modules.
Steps to test:
TODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)