Allow to change individual mesh opacity #8443
Conversation
📝 WalkthroughWalkthroughThe changes introduce support for RGBA colors and individual mesh opacity control across the codebase. Enhancements include new components and utility functions in the color picker, updates to controllers for handling opacity parameters, new action types and reducers to manage mesh opacity state, and sagas to orchestrate these changes. The store type is updated to include a readonly opacity property, and corresponding UI components and documentation are revised to integrate and explain the new opacity adjustment functionality. Changes
Assessment against linked issues
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 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 (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
changed the title
|
I realized that it is not possible to set the opacity of multiple meshes at once (e.g. by selecting multiple segments with loaded meshes and selecting "change segment color"). please let me know if thats needed! |
![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 (1)
frontend/javascripts/components/color_picker.tsx (1)
138-144: RGBA picker and inputs combined effectively.
This layout ensures users can manipulate color and opacity at once. Minor nit: remove the space between"100 %"to ensure consistent CSS.-<RgbaColorPicker color={value} onChange={setValue} style={{ width: "100 %" }} /> +<RgbaColorPicker color={value} onChange={setValue} style={{ width: "100%" }} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
frontend/javascripts/components/color_picker.tsx(3 hunks)frontend/javascripts/oxalis/constants.ts(1 hunks)frontend/javascripts/oxalis/controller/segment_mesh_controller.ts(3 hunks)frontend/javascripts/oxalis/model/actions/annotation_actions.ts(3 hunks)frontend/javascripts/oxalis/model/reducers/annotation_reducer.ts(4 hunks)frontend/javascripts/oxalis/model/sagas/mesh_saga.ts(3 hunks)frontend/javascripts/oxalis/store.ts(1 hunks)frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segment_list_item.tsx(7 hunks)frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx(3 hunks)
🧰 Additional context used
🧬 Code Definitions (5)
frontend/javascripts/oxalis/model/actions/annotation_actions.ts (1)
frontend/javascripts/types/api_flow_types.ts (1) (1)
AdditionalCoordinate(30-30)
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx (2)
frontend/javascripts/types/api_flow_types.ts (1) (1)
AdditionalCoordinate(30-30)frontend/javascripts/oxalis/model/actions/annotation_actions.ts (1) (1)
updateMeshOpacityAction(235-247)
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segment_list_item.tsx (4)
frontend/javascripts/types/api_flow_types.ts (1) (1)
AdditionalCoordinate(30-30)frontend/javascripts/oxalis/constants.ts (2) (2)
Vector4(14-14)Vector3(13-13)frontend/javascripts/components/color_picker.tsx (1) (1)
ChangeRGBAColorMenuItemContent(148-183)frontend/javascripts/oxalis/controller/segment_mesh_controller.ts (1) (1)
setMeshOpacity(279-293)
frontend/javascripts/oxalis/model/sagas/mesh_saga.ts (1)
frontend/javascripts/oxalis/model/actions/annotation_actions.ts (1) (1)
UpdateMeshOpacityAction(63-63)
frontend/javascripts/components/color_picker.tsx (1)
frontend/javascripts/oxalis/constants.ts (2) (2)
Vector4(14-14)Vector3(13-13)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (48)
frontend/javascripts/oxalis/constants.ts (1)
349-349: LGTM! Well-placed constant for mesh opacity.Adding a default mesh opacity constant with value 1 (representing full opacity) is appropriate and follows the established pattern in this file.
frontend/javascripts/oxalis/store.ts (1)
581-581: LGTM! Good addition of opacity property to mesh information.The readonly opacity property is a good addition to the BaseMeshInformation type, ensuring all mesh types will have consistent opacity handling.
frontend/javascripts/oxalis/model/reducers/annotation_reducer.ts (4)
5-5: Appropriate import for DEFAULT_MESH_OPACITY constant.The Constants import is needed for using DEFAULT_MESH_OPACITY in the reducer.
281-299: LGTM! Well-implemented reducer case for mesh opacity updates.The implementation follows the pattern used for other mesh properties updates, correctly extracting the action parameters and using immutability-helper to update state.
345-345: Good use of DEFAULT_MESH_OPACITY constant.Using the constant for ad-hoc meshes ensures consistency when initializing opacity.
391-391: Good use of DEFAULT_MESH_OPACITY constant.Using the constant for precomputed meshes ensures consistency when initializing opacity.
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx (3)
65-65: Appropriate import for mesh opacity action.The addition of updateMeshOpacityAction import aligns with the new feature implementation.
320-327: LGTM! Well-implemented mesh opacity change dispatcher.The changeMeshOpacity method correctly dispatches the updateMeshOpacityAction with all necessary parameters (segmentId, layerName, opacity, and additionalCoordinates).
769-769: Proper prop passing for mesh opacity functionality.Passing setMeshOpacity to SegmentListItem enables the component to change mesh opacity, completing the feature implementation chain.
frontend/javascripts/oxalis/model/actions/annotation_actions.ts (3)
63-63: Action type defined correctly for mesh opacity updates.The new
UpdateMeshOpacityActiontype follows the established pattern in the codebase for defining action types.
97-97: Union type correctly updated to include the new action type.Adding the new action type to the
AnnotationActionTypesunion ensures type safety when handling this action throughout the application.
235-247: Well-implemented action creator for updating mesh opacity.The
updateMeshOpacityActionfunction is correctly implemented, taking the necessary parameters (layerName, id, opacity, and optional additionalCoordinates) and returning a properly structured action object with the type "UPDATE_MESH_OPACITY".frontend/javascripts/oxalis/controller/segment_mesh_controller.ts (4)
261-277: Well-implemented color setter with opacity support.The
setMeshColormethod has been enhanced to optionally set opacity when specified. This implementation correctly iterates through all child meshes and applies the opacity, ensuring consistent behavior across all mesh representations.
279-293: Good implementation of dedicated mesh opacity setter.The new
setMeshOpacitymethod follows the same pattern as other mesh-related methods in the controller. It correctly handles ND datasets by setting the opacity for all additional coordinates.
391-391: Fixed opacity calculation for hovered meshes.The calculation of
targetOpacitynow correctly preserves the current opacity when a mesh is hovered, improving visual consistency.
436-436: Properly maintaining opacity during hover effect.This change ensures that the opacity setting is preserved when applying hover effects to mesh materials.
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segment_list_item.tsx (5)
16-19: Correctly imported RGBA color picker component.The new import provides access to the
ChangeRGBAColorMenuItemContentcomponent which supports opacity adjustment.
214-219: Well-defined prop type for mesh opacity setter.The new
setMeshOpacityprop is properly typed to accept the necessary parameters for updating mesh opacity.
425-427: Appropriate usage of Redux state for additional coordinates.This properly retrieves the additional coordinates from the Redux store to ensure opacity is applied in the correct context.
437-442: Well-implemented color vector with opacity.This code correctly combines the segment color with the mesh opacity (using a default if no mesh exists) to create a complete RGBA color vector.
470-495: Effective conditional rendering of RGBA color picker.The implementation correctly uses the RGBA color picker when a mesh is visible, enabling opacity adjustment. The callback properly updates both the segment color and the mesh opacity.
frontend/javascripts/oxalis/model/sagas/mesh_saga.ts (3)
49-49: Appropriate import of action type.The import of
UpdateMeshOpacityActionensures proper type checking in the saga handler.
1263-1269: Well-implemented saga handler for opacity changes.The
handleMeshOpacityChangefunction correctly retrieves the necessary state and utilizes thesegmentMeshControllerto set the mesh opacity. It also performs a proper check to ensure the mesh exists before attempting to update its opacity.
1306-1306: Saga correctly registered to handle opacity updates.The handler is properly registered to intercept and process "UPDATE_MESH_OPACITY" actions using the Redux-Saga takeEvery effect.
frontend/javascripts/components/color_picker.tsx (24)
1-1: Imports look good.
No issues found with importingInputNumberandPopoverfrom "antd".
4-4: VECTOR types usage is appropriate.
Importing bothVector3andVector4aligns well with RGBA support.
6-7: Extended imports fromreact-colorful.
Bringing inRgbaColorandRgbaColorPickeris a proper step for handling RGBA values.
9-10: Clear naming for constants.
COLOR_PICKER_WIDTHandRELATIVE_OPACITY_INPUT_WIDTHare intuitively named, enhancing code readability.
12-18: ReusablegetPopoverutility is well-defined.
The function implementation is straightforward, improving the maintainability of your popover logic.
20-27: Inline styles manage theming effectively.
Using a sharedinputStyleobject with variables for color and border is consistent across components.
29-35:getColorInputhelps modularize color input creation.
This addition reduces duplication and centralizes control of the HEX input styles.
51-51: Minimal style adjustment.
The inline margin improves layout; no further issues noted.
53-54: Consistent use ofgetColorInput.
Maintaining the same pattern in both color picker components is good for uniformity.
88-89: FinalgetPopoverusage is clean.
Returning the popover in one place improves clarity.
91-97:ThrottledRGBAColorPickersignature.
The prop names are descriptive, and the approach to handle color changes is well-structured.
98-107: Hex-to-RGBA conversion is well-implemented.
Reusing the existing utilities for color conversion is consistent with the rest of the codebase.
109-115: Informative note for opacity.
Showing a clarifying message about the 3D viewport ensures users understand the limitation.
117-133: Opacity input is well-controlled.
UsingInputNumberwith min=0, max=1, step=0.1 is user-friendly for fine-tuned opacity.
134-135: Separate hex value computation.
valueAsHexclarifies the offset between local RGBA state and the Hex input.
137-137: Inline styling for layout.
No concerns, the width is tied toCOLOR_PICKER_WIDTH, which’s consistent with the rest.
146-146: Function closure.
No actionable feedback on this line.
148-153:ChangeRGBAColorMenuItemContentstructure is sound.
Clear, descriptive props to manage color and disabled state.
154-158: Type definitions for RGBA.
Acceptingrgba: Vector4is consistent with your new opacity-driven logic.
159-165: Comprehensive local color object.
Calculatingcolorfrom the Vector4 ensures no confusion mapping alpha channels.
166-170: Correct early return forisDisabled.
The guard statement prevents unwanted state changes.
171-176: Undo state logic is consistent with existing color approach.
This prevents cluttering the undo stack on continuous slider changes.
178-180: Conditional rendering for content matches disabled state.
Nullifying the picker component is a succinct approach for read-only flows.
182-182: Popover usage.
ReturninggetPopoverfinishes the RGBA color menu flow elegantly with minimal overhead.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/javascripts/components/color_picker.tsx (1)
112-112: Typo in the information text.There's a small typo in the information text about opacity.
- Note that the opacity will only effect the mesh in the 3D viewport. + Note that the opacity will only affect the mesh in the 3D viewport.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.unreleased.md(1 hunks)docs/volume_annotation/segments_list.md(1 hunks)frontend/javascripts/components/color_picker.tsx(3 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
frontend/javascripts/components/color_picker.tsx (1)
frontend/javascripts/oxalis/constants.ts (2) (2)
Vector4(14-14)Vector3(13-13)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (11)
frontend/javascripts/components/color_picker.tsx (9)
1-1: Appropriate imports for RGBA color support.These imports add the necessary components and types for handling RGBA colors and opacity adjustments.
Also applies to: 4-4, 6-7
9-10: Well-defined constants for layout consistency.The constants provide consistent sizing and proportions for the color picker components, which improves UI predictability.
12-18: Good abstraction with thegetPopoverfunction.This utility function improves code organization by centralizing the popover creation logic, making the code more maintainable.
20-27: Consistent input styling.The shared styling for input elements ensures a consistent look and feel throughout the color picker components.
29-35: Good abstraction with thegetColorInputfunction.This utility function centralizes the creation of hex color input fields, which reduces code duplication.
50-54: Improved code structure by using abstracted components.Using the
getColorInputfunction here simplifies the JSX structure and improves readability.
88-88: Consistent usage of utility functions.Using the
getPopoverfunction here maintains consistency throughout the codebase.
91-146: Well-implementedThrottledRGBAColorPickercomponent.This component effectively handles RGBA color selection with opacity control. The throttled callback prevents excessive updates, and the helper functions provide clear separation of concerns.
The information text about opacity affecting only the 3D viewport is helpful for users.
148-183: Well-implementedChangeRGBAColorMenuItemContentcomponent.This component properly integrates the RGBA color picker with the application's state management. The approach to undo state management (creating a new state only on the first color change) is consistent with the existing RGB implementation, which is good for code consistency.
CHANGELOG.unreleased.md (1)
16-16: Clear and informative changelog entry.The changelog entry accurately describes the new feature and provides a reference to the PR number.
docs/volume_annotation/segments_list.md (1)
11-11: Clear documentation update.The documentation clearly explains that opacity adjustment is available only when the mesh is visible, which matches the implementation and PR description.
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segment_list_item.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/controller/segment_mesh_controller.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segment_list_item.tsx (1)
436-442: Consider extracting the color+opacity array creation into a utility function.This opacity-augmented color array computation might be cleaner as a utility function rather than inline code. This would improve readability and make the code more maintainable.
- const segmentColorWithMeshOpacity: Vector4 = [ - segmentColorRGBA[0], - segmentColorRGBA[1], - segmentColorRGBA[2], - mesh != null ? mesh.opacity : Constants.DEFAULT_MESH_OPACITY, - ]; + const getColorWithMeshOpacity = (color: Vector4, mesh?: MeshInformation | null): Vector4 => [ + color[0], + color[1], + color[2], + mesh != null ? mesh.opacity : Constants.DEFAULT_MESH_OPACITY, + ]; + + const segmentColorWithMeshOpacity = getColorWithMeshOpacity(segmentColorRGBA, mesh);frontend/javascripts/components/color_picker.tsx (1)
91-146: LGTM! Well-structured RGBA color picker implementation.The
ThrottledRGBAColorPickercomponent is well-designed with appropriate state management, throttled callbacks, and a clean UI layout. The division between color and opacity inputs is intuitive and the helper function for conversion between hex and RGBA is well-implemented.One minor improvement suggestion:
- const maybeGetInfoText = () => { + const getInfoText = () => {The "maybe" prefix typically implies conditional rendering, but this function always returns content.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
frontend/javascripts/components/color_picker.tsx(3 hunks)frontend/javascripts/oxalis/controller/segment_mesh_controller.ts(2 hunks)frontend/javascripts/oxalis/model/accessors/volumetracing_accessor.ts(3 hunks)frontend/javascripts/oxalis/model/actions/annotation_actions.ts(3 hunks)frontend/javascripts/oxalis/model/reducers/annotation_reducer.ts(4 hunks)frontend/javascripts/oxalis/model/sagas/mesh_saga.ts(3 hunks)frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segment_list_item.tsx(6 hunks)frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx(4 hunks)frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_renderers.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/javascripts/oxalis/model/reducers/annotation_reducer.ts
- frontend/javascripts/oxalis/model/actions/annotation_actions.ts
🧰 Additional context used
🧬 Code Definitions (4)
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segment_list_item.tsx (5)
frontend/javascripts/oxalis/constants.ts (2)
Vector4(14-14)Vector3(13-13)frontend/javascripts/oxalis/shaders/utils.glsl.ts (1)
rgbaToCSS(117-120)frontend/javascripts/types/api_flow_types.ts (1)
AdditionalCoordinate(30-30)frontend/javascripts/components/color_picker.tsx (2)
ChangeRGBAColorMenuItemContent(148-183)ChangeColorMenuItemContent(58-89)frontend/javascripts/oxalis/controller/segment_mesh_controller.ts (1)
setMeshOpacity(279-293)
frontend/javascripts/oxalis/model/sagas/mesh_saga.ts (1)
frontend/javascripts/oxalis/model/actions/annotation_actions.ts (1)
UpdateMeshOpacityAction(63-63)
frontend/javascripts/components/color_picker.tsx (1)
frontend/javascripts/oxalis/constants.ts (2)
Vector4(14-14)Vector3(13-13)
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx (2)
frontend/javascripts/oxalis/model/actions/annotation_actions.ts (1)
updateMeshOpacityAction(235-241)frontend/javascripts/oxalis/constants.ts (1)
Vector3(13-13)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (22)
frontend/javascripts/oxalis/model/accessors/volumetracing_accessor.ts (2)
681-689: Well-implemented mesh opacity accessor function.The new
getMeshOpacityfunction correctly retrieves the opacity value for a mesh based on segment ID and layer name, properly handling the case when mesh data isn't available.
708-709: Good integration of opacity retrieval in color function.Nicely done modifying
getSegmentColorAsRGBAto use the new opacity accessor with a fallback to the default constant. This change properly supports the new feature of individual mesh opacity settings.frontend/javascripts/oxalis/model/sagas/mesh_saga.ts (2)
1263-1266: Well-structured saga for handling mesh opacity updates.The new
handleMeshOpacityChangegenerator function correctly implements the Redux saga pattern for processing opacity change actions, delegating to the appropriate controller method.
1303-1303: Properly registered new saga event handler.Good job adding the event listener for the "UPDATE_MESH_OPACITY" action type within the meshSaga function.
frontend/javascripts/oxalis/controller/segment_mesh_controller.ts (3)
261-277: Enhanced setMeshColor function with opacity support.Good modification to support optional opacity parameter when setting mesh color. This change handles the opacity setting across all additional coordinates for ND datasets correctly.
279-293: Well-implemented mesh opacity setter.The new
setMeshOpacitymethod appropriately sets the opacity for all child meshes of a segment across all additional coordinates, which aligns with the PR objective of allowing opacity changes for individual meshes.
391-391: Fixed opacity calculation in updateMeshAppearance method.Good adjustment to calculate target opacity based on the current material's opacity rather than using a hardcoded value. This ensures consistent behavior when hovering over meshes with custom opacity values.
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_renderers.tsx (2)
132-134: Improved type safety for color handling.Good type annotation update for the
onSetColorcallback to explicitly useVector3, ensuring that only the first three components of the color are used for the tree color setting.
447-450: Enhanced type safety for tree group color setting.Similar improvement for the tree group color setter with proper type annotation, maintaining consistency across the codebase.
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segment_list_item.tsx (4)
469-513: LGTM! Well-implemented conditional color picker rendering.The implementation correctly uses either RGBA or RGB color pickers based on mesh visibility, allowing for opacity control only when a mesh is visible. This enables a consistent and intuitive user experience.
485-490: Well-structured opacity update logic.The code properly updates both the segment color and mesh opacity when the user changes the RGBA value in the color picker. The use of destructuring to separate the RGB and alpha components is clean and effective.
213-218: Clear type definition for the setMeshOpacity prop.The type definition for the new setMeshOpacity function is well-structured with clear parameter types, which enhances code readability and maintainability.
424-426: Good use of Redux selector for additional coordinates.Accessing the additional coordinates from the Redux store using a selector follows best practices for Redux state management.
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx (3)
320-322: LGTM! Well-implemented mesh opacity action dispatcher.The
changeMeshOpacitymethod correctly dispatches theupdateMeshOpacityActionwith all necessary parameters, following the established Redux patterns in the codebase.
1764-1764: LGTM! Proper prop passing to SegmentListItem.The
setMeshOpacityprop is correctly passed to the SegmentListItem component, connecting the UI element to the action dispatcher.
1098-1098: Good type safety improvement.Adding the explicit
Vector3type annotation to thecolorparameter improves type safety and makes the code more maintainable.frontend/javascripts/components/color_picker.tsx (6)
9-10: LGTM! Good use of constants for layout dimensions.The constants for color picker width and opacity input width make the layout more maintainable and easier to adjust in the future.
12-18: Well-implemented utility function for popover creation.Extracting the popover creation to a utility function reduces code duplication and ensures consistent styling across different popover instances.
29-35: Good extraction of color input creation.The
getColorInputfunction nicely encapsulates the creation of hex color input fields, improving code organization and reusability.
148-183: LGTM! Well-implemented RGBA color menu item.The
ChangeRGBAColorMenuItemContentfunction follows the same pattern as the RGB version, ensuring consistency. The handling of undo states and color conversions is properly implemented.
51-54: Consistent styling with the RGBA implementation.The styling updates to
ThrottledColorPickerensure consistency with the new RGBA implementation, and the use of thegetColorInpututility function reduces code duplication.
88-89: Good use of the utility function.Using the
getPopoverutility function here simplifies the code and ensures consistent styling.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/javascripts/components/color_picker.tsx (1)
109-115: Consider improving the opacity help text.The help text states that opacity only affects the mesh in the 3D viewport, but it might be helpful to provide more specific guidance or examples of how opacity changes are visible to users.
- Note that the opacity will only affect the mesh in the 3D viewport. + Note that the opacity value only affects the 3D mesh visualization and won't change the segment's appearance in 2D views.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/javascripts/components/color_picker.tsx(3 hunks)frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segment_list_item.tsx(6 hunks)frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx(3 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx (1)
frontend/javascripts/oxalis/model/actions/annotation_actions.ts (1)
updateMeshOpacityAction(235-241)
frontend/javascripts/components/color_picker.tsx (1)
frontend/javascripts/oxalis/constants.ts (2)
Vector4(14-14)Vector3(13-13)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (14)
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx (3)
65-65: LGTM: Successfully added the opacity action import.The import is well-placed and maintains the alphabetical order used in the import block.
320-322: LGTM: Good implementation of opacity change dispatcher.The
changeMeshOpacityfunction is implemented clearly and follows the same pattern as other action dispatchers in this file. It takes the segment ID, layer name, and opacity value, then dispatches the appropriate action.
1764-1764: LGTM: Properly passed the mesh opacity setter to SegmentListItem.The
changeMeshOpacityfunction is correctly passed as thesetMeshOpacityprop to theSegmentListItemcomponent, making it available for use in the segment context menu.frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segment_list_item.tsx (7)
16-19: LGTM: Added RGBA color picker import.The import of
ChangeRGBAColorMenuItemContentalongside the existingChangeColorMenuItemContentis correctly implemented and maintains the code structure.
57-57: LGTM: Added Constants import for default mesh opacity.The import is necessary to access the default mesh opacity and is properly placed.
213-218: LGTM: Added setMeshOpacity to Props interface.The
setMeshOpacityfunction is properly added to the Props interface with clear parameter types.
424-426: LGTM: Added additionalCoordinates state using useSelector.The additionalCoordinates state is properly retrieved from the Redux store for use in the opacity update.
437-442: LGTM: Created a combined color vector with mesh opacity.Creating a Vector4 that combines the RGB values with the mesh opacity (or default) is a good approach for passing to the color picker.
469-494: LGTM: Conditionally using RGBA color picker when mesh is visible.Good implementation of conditional rendering between
ChangeRGBAColorMenuItemContentandChangeColorMenuItemContentbased on mesh visibility. The onSetColor callback correctly updates both the segment color and mesh opacity.
495-513: LGTM: Fallback to standard RGB color picker when mesh is not visible.Proper fallback to the standard color picker when mesh is not visible, which only updates the segment color.
frontend/javascripts/components/color_picker.tsx (4)
9-10: LGTM: Added constants for layout dimensions.The constants
COLOR_PICKER_WIDTHandRELATIVE_OPACITY_INPUT_WIDTHprovide consistent dimensions across the picker components.
12-35: LGTM: Created utility functions for popovers and color inputs.The
getPopoverandgetColorInpututility functions are well-implemented abstractions that reduce code duplication and improve maintainability.
91-146: LGTM: Added ThrottledRGBAColorPicker component.The
ThrottledRGBAColorPickercomponent is well-structured with clear state management and proper UI components for RGBA color selection, including opacity input.
148-183: LGTM: Added ChangeRGBAColorMenuItemContent function.The
ChangeRGBAColorMenuItemContentfunction follows the same pattern as the existingChangeColorMenuItemContentbut is adapted for RGBA colors. It properly handles undo state management and color conversions.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segment_list_item.tsx (1)
476-476: Consider adding opacity to the undo state.When setting the mesh opacity, there is no parameter for
createsNewUndoStateunlike the segment color update. Consider adding undo support for opacity changes to maintain consistency with color changes.- setMeshOpacity(segment.id, visibleSegmentationLayer.name, color[3]); + setMeshOpacity(segment.id, visibleSegmentationLayer.name, color[3], createsNewUndoState);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segment_list_item.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.unreleased.md
🧰 Additional context used
🧬 Code Definitions (1)
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segment_list_item.tsx (3)
frontend/javascripts/oxalis/constants.ts (2)
Vector4(14-14)Vector3(13-13)frontend/javascripts/components/color_picker.tsx (2)
ChangeRGBAColorMenuItemContent(148-183)ChangeColorMenuItemContent(58-89)frontend/javascripts/oxalis/controller/segment_mesh_controller.ts (1)
setMeshOpacity(280-294)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (6)
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segment_list_item.tsx (6)
16-19: Well-structured import for new RGBA color picker component.The import of
ChangeRGBAColorMenuItemContentalongside the existingChangeColorMenuItemContentcorrectly prepares for the opacity feature implementation. This maintains a clean separation between RGB and RGBA color controls.
57-58: Good addition of the Constants import.This import provides access to
DEFAULT_MESH_OPACITYwhich is used as a fallback value when no mesh is available.
213-213: Proper Props extension for the new opacity feature.The addition of the
setMeshOpacitymethod to the Props interface correctly defines the contract for setting mesh opacity with the required parameters (segment ID, layer name, and opacity value).
427-434: Good implementation of RGBA color handling.Creating a
segmentColorWithMeshOpacityvector that combines the segment's RGB color with the mesh's opacity (or default opacity if not available) is a clean approach for managing the full RGBA value.
460-480: Effective conditional rendering of opacity controls.The implementation correctly uses
ChangeRGBAColorMenuItemContentonly when the mesh is visible, which is a good UX decision as opacity only makes sense for visible meshes. The callback properly separates the color update from the opacity setting.
481-498: Consistent implementation of color-only controls.The fallback to
ChangeColorMenuItemContentwhen the mesh is not visible maintains a consistent user experience while appropriately limiting functionality based on context.
| const createSegmentContextMenu = (): MenuProps => { | ||
| const segmentColorWithMeshOpacity: Vector4 = [ | ||
| segmentColorRGBA[0], | ||
| segmentColorRGBA[1], | ||
| segmentColorRGBA[2], | ||
| mesh != null ? mesh.opacity : Constants.DEFAULT_MESH_OPACITY, | ||
| ]; | ||
| return { | ||
| items: [ | ||
| getLoadPrecomputedMeshMenuItem( | ||
| segment, | ||
| currentMeshFile, | ||
| loadPrecomputedMesh, | ||
| hideContextMenu, | ||
| visibleSegmentationLayer != null ? visibleSegmentationLayer.name : null, | ||
| mappingInfo, | ||
| activeVolumeTracing, | ||
| ), | ||
| }, | ||
| { | ||
| key: "resetSegmentColor", | ||
| disabled: segment.color == null, | ||
| onClick: () => { | ||
| if (visibleSegmentationLayer == null) { | ||
| return; | ||
| } | ||
| updateSegment( | ||
| segment.id, | ||
| { | ||
| color: null, | ||
| }, | ||
| visibleSegmentationLayer.name, | ||
| true, | ||
| ); | ||
| getComputeMeshAdHocMenuItem( | ||
| segment, | ||
| loadAdHocMesh, | ||
| visibleSegmentationLayer != null, | ||
| hideContextMenu, | ||
| ), | ||
| getMakeSegmentActiveMenuItem( | ||
| segment, | ||
| setActiveCell, | ||
| activeCellId, | ||
| isEditingDisabled, | ||
| hideContextMenu, | ||
| ), | ||
| { | ||
| key: `changeSegmentColor-${segment.id}`, | ||
| label: mesh?.isVisible ? ( | ||
| <ChangeRGBAColorMenuItemContent | ||
| isDisabled={false} | ||
| title="Change Segment Color" | ||
| onSetColor={(color, createsNewUndoState) => { | ||
| if (visibleSegmentationLayer == null) { | ||
| return; | ||
| } | ||
| updateSegment( | ||
| segment.id, | ||
| { | ||
| color: color.slice(0, 3) as Vector3, | ||
| }, | ||
| visibleSegmentationLayer.name, | ||
| createsNewUndoState, | ||
| ); | ||
| setMeshOpacity(segment.id, visibleSegmentationLayer.name, color[3]); | ||
| }} | ||
| rgba={segmentColorWithMeshOpacity} | ||
| /> | ||
| ) : ( | ||
| <ChangeColorMenuItemContent | ||
| isDisabled={false} | ||
| title="Change Segment Color" | ||
| onSetColor={(color, createsNewUndoState) => { | ||
| if (visibleSegmentationLayer == null) { | ||
| return; | ||
| } | ||
| updateSegment( | ||
| segment.id, | ||
| { | ||
| color, | ||
| }, | ||
| visibleSegmentationLayer.name, | ||
| createsNewUndoState, | ||
| ); | ||
| }} | ||
| rgb={segmentColorRGBA.slice(0, 3) as Vector3} | ||
| /> | ||
| ), | ||
| }, | ||
| label: "Reset Segment Color", | ||
| }, | ||
| { | ||
| key: "removeSegmentFromList", | ||
| onClick: () => { | ||
| if (visibleSegmentationLayer == null) { | ||
| return; | ||
| } | ||
| removeSegment(segment.id, visibleSegmentationLayer.name); | ||
| hideContextMenu(); | ||
| { | ||
| key: "resetSegmentColor", | ||
| disabled: segment.color == null, | ||
| onClick: () => { | ||
| if (visibleSegmentationLayer == null) { | ||
| return; | ||
| } | ||
| updateSegment( | ||
| segment.id, | ||
| { | ||
| color: null, | ||
| }, | ||
| visibleSegmentationLayer.name, | ||
| true, | ||
| ); | ||
| }, | ||
| label: "Reset Segment Color", | ||
| }, | ||
| label: "Remove Segment From List", | ||
| }, | ||
| { | ||
| key: "deleteSegmentData", | ||
| onClick: () => { | ||
| if (visibleSegmentationLayer == null) { | ||
| return; | ||
| } | ||
| { | ||
| key: "removeSegmentFromList", | ||
| onClick: () => { | ||
| if (visibleSegmentationLayer == null) { | ||
| return; | ||
| } | ||
| removeSegment(segment.id, visibleSegmentationLayer.name); | ||
| hideContextMenu(); | ||
| }, | ||
| label: "Remove Segment From List", | ||
| }, | ||
| { | ||
| key: "deleteSegmentData", | ||
| onClick: () => { | ||
| if (visibleSegmentationLayer == null) { | ||
| return; | ||
| } | ||
|
|
||
| modal.confirm({ | ||
| content: `Are you sure you want to delete the data of segment ${getSegmentName( | ||
| segment, | ||
| true, | ||
| )}? This operation will set all voxels with id ${segment.id} to 0.`, | ||
| okText: "Yes, delete", | ||
| okType: "danger", | ||
| onOk: async () => { | ||
| await new Promise<void>((resolve) => | ||
| deleteSegmentData(segment.id, visibleSegmentationLayer.name, resolve), | ||
| ); | ||
|
|
||
| Toast.info( | ||
| <span> | ||
| The data of segment {getSegmentName(segment, true)} was deleted.{" "} | ||
| <a | ||
| href="#" | ||
| onClick={() => { | ||
| removeSegment(segment.id, visibleSegmentationLayer.name); | ||
| Toast.close(ALSO_DELETE_SEGMENT_FROM_LIST_KEY); | ||
| }} | ||
| > | ||
| Also remove from list. | ||
| </a> | ||
| </span>, | ||
| { key: ALSO_DELETE_SEGMENT_FROM_LIST_KEY }, | ||
| ); | ||
| }, | ||
| }); | ||
| modal.confirm({ | ||
| content: `Are you sure you want to delete the data of segment ${getSegmentName( | ||
| segment, | ||
| true, | ||
| )}? This operation will set all voxels with id ${segment.id} to 0.`, | ||
| okText: "Yes, delete", | ||
| okType: "danger", | ||
| onOk: async () => { | ||
| await new Promise<void>((resolve) => | ||
| deleteSegmentData(segment.id, visibleSegmentationLayer.name, resolve), | ||
| ); | ||
|
|
||
| hideContextMenu(); | ||
| Toast.info( | ||
| <span> | ||
| The data of segment {getSegmentName(segment, true)} was deleted.{" "} | ||
| <a | ||
| href="#" | ||
| onClick={() => { | ||
| removeSegment(segment.id, visibleSegmentationLayer.name); | ||
| Toast.close(ALSO_DELETE_SEGMENT_FROM_LIST_KEY); | ||
| }} | ||
| > | ||
| Also remove from list. | ||
| </a> | ||
| </span>, | ||
| { key: ALSO_DELETE_SEGMENT_FROM_LIST_KEY }, | ||
| ); | ||
| }, | ||
| }); | ||
|
|
||
| hideContextMenu(); | ||
| }, | ||
| disabled: | ||
| activeVolumeTracing == null || | ||
| !activeVolumeTracing.hasSegmentIndex || | ||
| // Not supported for fallback layers, yet. | ||
| activeVolumeTracing.fallbackLayer != null, | ||
| label: "Delete Segment's Data", | ||
| }, | ||
| disabled: | ||
| activeVolumeTracing == null || | ||
| !activeVolumeTracing.hasSegmentIndex || | ||
| // Not supported for fallback layers, yet. | ||
| activeVolumeTracing.fallbackLayer != null, | ||
| label: "Delete Segment's Data", | ||
| }, | ||
| ], | ||
| }); | ||
| ], | ||
| }; | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider adding bulk opacity setting for multiple selections.
As noted in the PR comments, it's currently not possible to set opacity for multiple meshes simultaneously. When multiple segments are selected, consider extending the multi-select menu to include opacity controls.
🌐 Web query:
What are the best practices for editing multiple object properties simultaneously in React applications?
💡 Result:
When editing multiple object properties simultaneously in React applications, following these best practices ensures maintainable, efficient, and predictable state management:
Group Related State
Use a single state object to manage interdependent properties rather than multiple useState hooks. This prevents synchronization issues and reduces redundant updates[1][5][10]:
// Preferred for related properties
const [user, setUser] = useState({
name: 'John',
email: '[email protected]',
address: '123 Main St'
});Immutable Updates
Always create new objects when modifying state to avoid direct mutation[4][9][11]:
const updateUser = (newName) => {
setUser(prev => ({
...prev,
name: newName
}));
};For nested objects, spread at each level:
setPerson(prev => ({
...prev,
artwork: {
...prev.artwork,
city: 'New Delhi'
}
}));Dynamic Property Updates
Use computed property names for form-like components handling multiple fields[4][8]:
const handleChange = (e) => {
const { name, value } = e.target;
setUser(prev => ({
...prev,
[name]: value
}));
};Avoid State Duplication
Derive values from existing state instead of storing duplicates[1][5]:
// Instead of storing selectedItem separately:
const [items] = useState(products);
const [selectedId, setSelectedId] = useState(0);
const selectedItem = items.find(item => item.id === selectedId);Performance Optimization
- Memoize expensive calculations with
useMemo - Lift state to the nearest common ancestor when sharing between components[2][6]
- Use context API for global state needs[3][7]
Comparison of Approaches
| Method | Use Case | Performance | Complexity |
|---|---|---|---|
| Single state object | Related properties/form fields | Optimal | Low |
| Multiple useState | Independent properties | Good | Medium |
| useReducer | Complex state transitions | Excellent | High |
| Context API | Cross-component sharing | Variable | Medium |
For nested array updates, use mapping with spread operators[7]:
const updateUserInArray = (userId, updates) => {
setUsers(prev => prev.map(user =>
user.id === userId ? { ...user, ...updates } : user
));
};When dealing with forms:
- Controlled components for complex validation[6]
- Uncontrolled components for simple forms[6]
- Hybrid approach for mixed needs[6]
Key principles include:
- Immutability for predictable state changes
- Single source of truth for related data
- Minimal state by deriving values when possible
- Proper encapsulation through component hierarchy
These patterns prevent common pitfalls like stale state, unnecessary re-renders, and synchronization bugs[1][4][11].
Citations:
- 1: https://react.dev/learn/choosing-the-state-structure
- 2: https://www.dhiwise.com/post/The%20Ultimate%20Guide%20to%20Managing%20State%20Between%20Components%20in%20React
- 3: https://sunneysood.hashnode.dev/react-best-practices
- 4: https://react.dev/learn/updating-objects-in-state
- 5: https://www.bacancytechnology.com/qanda/react/multiple-states-or-one-state-object-in-react
- 6: https://dev.to/ajones_codes/a-better-guide-to-forms-in-react-47f0
- 7: https://www.dhiwise.com/post/react-update-array-of-objects-in-state-a-developer-guide
- 8: https://umamaheswaran.com/2023/04/12/managing-multiple-states-in-react-strategies-and-best-practices/
- 9: https://www.telerik.com/blogs/react-basics-working-react-objects
- 10: https://www.youtube.com/watch?v=PLoxdzlfkuw
- 11: https://www.youtube.com/watch?v=7VJn2aFu2rI
Bulk Opacity Update for Multi-Selection Needed
The current implementation supports opacity adjustments on a per-segment basis only. When multiple segments are selected, consider extending the multi-select context menu with an option to adjust mesh opacity in bulk. As best practices suggest, ensure that any new bulk-edit functionality:
- Groups related properties into a unified state (or uses a reducer) to prevent synchronization issues.
- Applies immutable update patterns when changing multiple segment properties.
- Utilizes computed property names or a similar concise strategy to update all selected segments concurrently.
This enhancement will align the feature with proven React state management practices and improve user efficiency.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/javascripts/oxalis/controller/segment_mesh_controller.ts (1)
280-294: Consider adding validation for the opacity parameter.The new
setMeshOpacitymethod effectively updates opacity across all additional coordinates for ND datasets, but it doesn't validate the input value.setMeshOpacity(id: number, layerName: string, opacity: number): void { + // Ensure opacity is between 0 and 1 + const validOpacity = Math.max(0, Math.min(1, opacity)); // If in nd-dataset, set the opacity for all additional coordinates for (const recordsOfLayers of Object.values(this.meshesGroupsPerSegmentId)) { const meshDataForOneSegment = recordsOfLayers[layerName][id]; if (meshDataForOneSegment != null) { for (const lodGroup of Object.values(meshDataForOneSegment)) { for (const meshGroup of lodGroup.children) { meshGroup.children.forEach((child: MeshSceneNode) => { - child.material.opacity = opacity; + child.material.opacity = validOpacity; }); } } } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/oxalis/controller/segment_mesh_controller.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (5)
frontend/javascripts/oxalis/controller/segment_mesh_controller.ts (5)
20-20: Good use of a constant for the hover opacity factor.Using a named constant for the hover opacity factor makes the code more maintainable and understandable.
262-278: Enhanced method now supports opacity setting along with color.The method has been expanded to accept an optional opacity parameter, allowing for both color and opacity modifications in a single call. This is a good approach that maintains backward compatibility.
392-394: Smart opacity calculation for hover effect.The code now dynamically calculates the hover opacity based on the mesh's current opacity instead of using a hard-coded value. This approach ensures the hover effect is proportional to the mesh's set opacity level.
439-439: Preserves original opacity during highlighting.When a mesh is highlighted (hovered or selected as active), its original opacity is now preserved. This ensures that custom opacity settings aren't lost when interacting with meshes.
270-274: Code matches PR objectives.The implementation correctly applies the opacity setting across all dimensions for ND datasets through the nested loops that traverse all mesh hierarchies. This matches the PR objective stating that "for ND datasets, the opacity setting will apply across all dimensions."
There was a problem hiding this comment.
Thanks for digging through this 🙏
https://media1.giphy.com/media/v1.Y2lkPTc5MGI3NjExMmRlYXVkcGxubHp0ZWpvcnN5OWJmb3k3d3hibWk2Ymp3OXl5OGZxdSZlcD12MV9pbnRlcm5hbF9naWZfYnlfaWQmY3Q9Zw/3ov9jRPMChw9ZzVlUk/giphy.gif
(embedding seems not to work)
I left a few minor comments.
Moreover, I gave unifying the color pickers a rough try as I had the same feeling as Philipp while reviewing that it would be ideal to have them unified. I made a working version which is definitely harder to understand and wouldn't pick this version. For more details please find my comment -> in short: I'd stick with your duplicated version 👍
frontend/javascripts/oxalis/controller/segment_mesh_controller.ts
Outdated
Show resolved
Hide resolved
| if (localSegmentationData?.meshes == null) return; | ||
| const meshData = localSegmentationData.meshes[additionalCoordinateKey]; | ||
| if (meshData == null || meshData[segmentId] == null) return; |
There was a problem hiding this comment.
could you please explicitly return undefined in the early return cases? And maybe add a type return hint to the function 🤔. The other functions here seem to define their return type as well
There was a problem hiding this comment.
okay, I can do that. The benefit is that everyone knows that undefined is returned then? Is there another benefit?
I am asking because this is a new rule to me and I was really used to simply returning (and in the code base it seems to be done all the time) @MichaelBuessemeyer
There was a problem hiding this comment.
I think that in previous reviews Philipp said that it was nice if JS is able to simply infer the return types. this seems to be working well here (number|undefined). Not sure why the other function are doing it explicitly though. so can I leave it as it is or do you have a strong preference?
There was a problem hiding this comment.
okay, I can do that. The benefit is that everyone knows that undefined is returned then? Is there another benefit?
Nope there isn't :D. It is just personal preference. While reading the code of this file I simply noticed that the surrounding functions also declared their return type and thus, making it also here would be just more consistent. The explicit return is just to explicitly show: "Here we return undefined" but not necessary.
I think that in previous reviews Philipp said that it was nice if JS is able to simply infer the return types
Hmm, ok I'd say this is a matter of personal preference: An explicit return type helps the person reading the function signature by directly telling what to expect as a return type. Thus, no need to read all surrounding code near all return statements in the function.
Moreover, when refactoring a function an explicit return type can save you from accidentally returning the wrong type. When there is no explicit return type, this would not result in an error 🤔
But I think you have now here pros and cons: So feel free to decide on you own :).
There was a problem hiding this comment.
okay! thank you! 🙏
There was a problem hiding this comment.
Regarding return type declaration in the signature:
But I think you have now here pros and cons: So feel free to decide on you own :).
I agree with that 👍
Regarding explicit return statements:
If a function returns nothing (i.e., void), using simple returns (or even no return at all) is what I recommend.
If a function returns something (e.g., number), all branches should return a number (obviously).
now, the kicker: if a function returns somethings sometimes (e.g., the return type would be number | undefined), then I'm a strong advocate of having a return someThing in all code paths. A simple return without anything would also return undefined, but from personal taste, I find this inconsistent. this is also what @MichaelBuessemeyer mentioned here, I think:
could you please explicitly return undefined in the early return cases?
There was a problem hiding this comment.
I am not sure whether I understand this correctly.
E.g. if there are no meshes (localSegmentationData?.meshes == null), would you prefer returning undefined or something like -1?
I always thought it would be reasonable to return undefined (explicitly or not) in these cases, because no number is making any sense.
There was a problem hiding this comment.
Regarding explicit return statements:
If a function returns nothing (i.e., void), using simple returns (or even no return at all) is what I recommend.
If a function returns something (e.g., number), all branches should return a number (obviously).
now, the kicker: if a function returns somethings sometimes (e.g., the return type would be number | undefined), then I'm a strong advocate of having a return someThing in all code paths. A simple return without anything would also return undefined, but from personal taste, I find this inconsistent. this is also what @MichaelBuessemeyer mentioned here, I think:
Oh that's a much better explanation of the rule I had in mind but couldn't properly formulate 👍 🎉
E.g. if there are no meshes (localSegmentationData?.meshes == null), would you prefer returning undefined or something like -1?
I always thought it would be reasonable to return undefined (explicitly or not) in these cases, because no number is making any sense.
Nope, if a value doesn't make sense, feel free to return undefined as this is what undefined is designed for. But as the function getMeshOpacity here returns number | undefined we fall into the last case described by @philippotto
now, the kicker: if a function returns somethings sometimes (e.g., the return type would be
number | undefined), then I'm a strong advocate of having a return someThing in all code paths. A simple return without anything would also return undefined, but from personal taste, I find this inconsistent.
To extend this: In such a case where number | undefined is returned, it would be more consistent to have some kind of value after all return statements so all return statements return an explicit value. This means that the simple return statements in the function should become return undefined thus making the code more consistent.
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segment_list_item.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.unreleased.md (1)
19-19: Clarify Feature Description for Mesh Opacity AdjustmentThe changelog entry clearly indicates that mesh opacity can now be adjusted via the 'Change Segment Color' context menu. However, to fully align with the PR objective of enabling individual mesh opacity control, consider explicitly mentioning "individual" meshes if that is the intended functionality. This clarification can help users distinguish between per-mesh adjustments versus a global change when multiple meshes are present on the same segment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CHANGELOG.unreleased.md(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/javascripts/components/color_picker.tsx (1)
145-175: Consider implementing a unified color picker component.The
ChangeRGBAColorMenuItemContentfunction is very similar toChangeColorMenuItemContent. Consider refactoring to unify these into a single component that handles both RGB and RGBA colors based on a prop likesupportOpacity.While this would add complexity to the typing, it could reduce code duplication and make maintenance easier in the long run.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/javascripts/components/color_picker.tsx(3 hunks)frontend/javascripts/oxalis/controller/segment_mesh_controller.ts(4 hunks)frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segment_list_item.tsx(6 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
frontend/javascripts/components/color_picker.tsx (1)
frontend/javascripts/oxalis/constants.ts (2)
Vector4(14-14)Vector3(13-13)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (16)
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segment_list_item.tsx (5)
16-19: Import for RGBA color picker added correctly.The additional import of
ChangeRGBAColorMenuItemContentalongside the existingChangeColorMenuItemContentallows for handling opacity settings through RGBA colors.
25-25: Constants import added for default mesh opacity.This import is necessary to access the
DEFAULT_MESH_OPACITYconstant used as a fallback value when no mesh is present.
213-213: New setMeshOpacity prop added to support opacity adjustments.This method signature correctly takes the segment ID, layer name, and opacity value as parameters, allowing for precise control over individual mesh opacity.
428-434: Good implementation of segment color with mesh opacity.The code properly constructs a Vector4 that combines the RGB values from the segment color with the mesh opacity, or falls back to the default opacity when no mesh is available.
480-498: Conditional component rendering based on mesh visibility.Good implementation of conditional rendering that uses
ChangeRGBAColorMenuItemContentwhen the mesh is visible andChangeColorMenuItemContentwhen it's not. This ensures opacity controls are only shown when they can actually be applied.frontend/javascripts/oxalis/controller/segment_mesh_controller.ts (6)
20-20: Good hover opacity factor value.The 0.8 factor provides sufficient visual feedback for hover state without making the mesh nearly invisible.
262-277: Excellent code refactoring with applyOnMeshGroupChildren helper.This helper method effectively reduces code duplication and centralizes the logic for traversing and modifying meshes. This makes the code more maintainable and reduces the risk of bugs in duplicated traversal logic.
279-286: Good update to setMeshColor with optional opacity parameter.The method has been enhanced to accept an optional opacity parameter while maintaining backward compatibility. The implementation correctly uses the new helper method to apply changes consistently.
288-293: New setMeshOpacity method implemented correctly.This dedicated method for setting only the opacity of meshes provides good separation of concerns and reuses the helper method. The comment about setting opacity for all additional coordinates is important for understanding the behavior in ND datasets.
391-393: Improved hover opacity calculation.The new hover opacity calculation preserves the relative opacity by multiplying with the hover factor rather than using a fixed value. This provides more consistent visual feedback across meshes with different base opacity values.
438-438: Fix for material opacity preservation during hover.Using the previous opacity value instead of a hard-coded value ensures that the mesh's original opacity is restored when hovering ends.
frontend/javascripts/components/color_picker.tsx (5)
9-10: Good use of constants for layout dimensions.Defining these constants at the top level makes the layout values consistent and easier to update throughout the component.
12-18: Useful UI helper function for creating consistent popovers.The
getPopoverfunction helps maintain consistency in popover implementation and reduces code duplication.
20-27: Centralized input styling for consistent UI.Creating a shared
inputStyleobject ensures consistent styling across all inputs and makes style updates easier to maintain.
29-30: Good code maintainability comment.The comment reminds developers to keep both color picker components in sync when making changes, which is important for maintainability.
84-143: Well-implemented RGBA color picker with opacity input.The
ThrottledRGBAColorPickercomponent is well-structured with appropriate helper methods for each UI element. The opacity input is correctly implemented with appropriate min/max bounds and step value.The explanatory text about opacity affecting only the 3D viewport is a good user experience enhancement.
| label: mesh?.isVisible ? ( | ||
| <ChangeRGBAColorMenuItemContent | ||
| title="Change Segment Color" | ||
| onSetColor={(color, createsNewUndoState) => { | ||
| if (visibleSegmentationLayer == null) { | ||
| return; | ||
| } | ||
| updateSegment( | ||
| segment.id, | ||
| { | ||
| color: color.slice(0, 3) as Vector3, | ||
| }, | ||
| visibleSegmentationLayer.name, | ||
| createsNewUndoState, | ||
| ); | ||
| setMeshOpacity(segment.id, visibleSegmentationLayer.name, color[3]); | ||
| }} | ||
| rgba={segmentColorWithMeshOpacity} | ||
| /> | ||
| ) : ( |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider adding bulk opacity setting for multiple selections.
As noted in the PR comments, it's currently not possible to set opacity for multiple meshes simultaneously. When multiple segments are selected, consider extending the multi-select menu to include opacity controls.
🌐 Web query:
What are the best practices for editing multiple object properties simultaneously in React applications?
💡 Result:
When editing multiple object properties simultaneously in React, follow these best practices to ensure maintainable and efficient state management:
Immutable Updates with Spread Operator
Always create new object instances when updating state to preserve immutability. For flat objects:
const [user, setUser] = useState({
name: '',
email: '',
role: 'user'
});
// Update multiple properties
const handleChange = (field, value) => {
setUser(prev => ({ ...prev, [field]: value }));
};Nested Object Updates
For multi-level objects, create copies at each nesting level:
const [profile, setProfile] = useState({
personal: {
firstName: '',
lastName: ''
},
settings: {
theme: 'light',
notifications: true
}
});
const updateTheme = (newTheme) => {
setProfile(prev => ({
...prev,
settings: {
...prev.settings,
theme: newTheme
}
}));
};Dynamic Property Handling
Use computed property names for generic update functions:
const handleFormUpdate = (e) => {
const { name, value } = e.target;
setFormData(prev => ({
...prev,
[name]: value
}));
};Performance Optimization
- Batched Updates: Use functional updates for sequential state changes
- Memoization: Apply
React.memoto prevent unnecessary re-renders of child components - State Normalization: Flatten complex nested structures when possible[1][8]
Component Architecture
- Create reusable input components that accept
name,value, andonChangeprops - Separate state management logic into custom hooks for complex forms[3][7]
const FormInput = ({ name, value, onChange }) => (
<input
name={name}
value={value}
onChange={(e) => onChange(name, e.target.value)}
/>
);Advanced State Management
For complex scenarios with multiple interrelated properties:
- useReducer: Centralize state logic
- Context API: Share state across components
- Type Safety: Implement PropTypes/TypeScript[3][9]
const formReducer = (state, action) => {
switch (action.type) {
case 'UPDATE_FIELD':
return { ...state, [action.field]: action.value };
case 'RESET_FORM':
return initialState;
default:
return state;
}
};Key Considerations
- Avoid direct mutation of state objects
- Keep related state properties grouped together
- Use Immer library for complex nested updates[8]
- Remove redundant state that can be calculated during rendering
- Implement proper TypeScript interfaces for large objects[3][7]
By following these patterns, developers can ensure predictable state behavior while maintaining code scalability and testability. The approach should be tailored to the specific complexity of the component - simpler components may use basic spread operations, while complex forms benefit from reducer-based management[5][7].
Citations:
- 1: https://www.youtube.com/watch?v=PLoxdzlfkuw
- 2: https://umamaheswaran.com/2023/04/12/managing-multiple-states-in-react-strategies-and-best-practices/
- 3: https://fireart.studio/blog/9-react-best-practices-to-improve-your-react-code/
- 4: https://www.dhiwise.com/post/react-update-array-of-objects-in-state-a-developer-guide
- 5: https://www.bacancytechnology.com/qanda/react/multiple-states-or-one-state-object-in-react
- 6: https://sunneysood.hashnode.dev/react-best-practices
- 7: https://react.dev/learn/choosing-the-state-structure
- 8: https://react.dev/learn/updating-objects-in-state
- 9: https://www.telerik.com/blogs/react-basics-working-react-objects
Action: Enhance Multi-Select for Bulk Opacity Updates
The current implementation supports updating opacity on a per-segment basis only. To improve user experience when multiple segments are selected, please extend the multi-select menu to include a control that adjusts opacity for all selected segments simultaneously. When implementing this change, consider the following best practices for handling multiple property updates in React:
- Immutable State Updates: Use techniques like the spread operator or a reducer to ensure state changes are immutable when updating opacity for multiple segments.
- Centralized Update Logic: Consolidate the opacity update logic into a single function that iterates over all selected segments, applying updates consistently (similar to how the color update is applied per segment).
- Performance Considerations: Leverage batched updates and memoization to minimize unnecessary re-renders when multiple segments are updated at once.
File: frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segment_list_item.tsx (Lines 460-479)
There was a problem hiding this comment.
The code looks good 👍
Thanks for incorporating the feedback 🏎️
Testing also went well, but I noticed a new bug:
- When I set the opacity to something below 1 for a mesh, then reload the mesh in the segments tab, the mesh is initially rendered with an opacity of 1.
But I'd say this can be fixed in a follow up so this can be merged and already benefit our customers. But if you find the fix fast, also feel free to include it here 👍
I see! This shouldnt be hard to solve, but I just gave it a try for a bit and it didnt work. So I propose to do this in a follow-up :) Thank you for the review @MichaelBuessemeyer and @philippotto! |
…icit return type hint
4 participants
URL of deployed dev instance (used for testing):
notes
Steps to test:
TODOs:
Issues:
context/feedback/related slack threads
https://scm.slack.com/archives/C5AKLAV0B/p1741786286167279
https://scm.slack.com/archives/C5AKLAV0B/p1743083368239799
https://scm.slack.com/archives/C5AKLAV0B/p1742984955308689
(Please delete unneeded items, merge only when none are left open)