Allow selective segment visibility regardless of proofreading tool #8281
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces a new feature for selective segment visibility in the frontend application. This enhancement allows users to control the visibility of segments through a toggle in the layer settings tab. The changes span multiple files, including the material factory, shaders, store configuration, and UI components. The implementation adds a new boolean property Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![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/oxalis/shaders/segmentation.glsl.ts (2)
358-392: Add documentation for the alpha increment calculation logic.The implementation is correct, but the complex visibility logic would benefit from documentation explaining:
- The different alpha values and their visual effects
- The interaction between proofreading and normal modes
- The purpose of each condition branch
Add a documentation block like this:
+ /* + * Calculates the alpha increment for segment visibility based on the segment state: + * + * Proofreading mode: + * - Active cell + hovered unmapped: +0.4 (highlight super-voxel) + * - Active cell + hovered: +0.15 (highlight non-hovered parts) + * - Hovered cell: +0.2 + * - Other cells: -alpha if selective visibility enabled + * + * Normal mode: + * - Hovered segment: +0.2 + * - Active cell with selective visibility: +0.15 + * - Other cells with selective visibility: -alpha (hide) + * - Default: no change + */ float getSegmentationAlphaIncrement(float alpha, bool isHoveredSegment, bool isHoveredUnmappedSegment, bool isActiveCell) {
358-392: Consider refactoring for improved readability.The nested conditional logic could be simplified using early returns and helper functions.
Consider this refactor:
- float getSegmentationAlphaIncrement(float alpha, bool isHoveredSegment, bool isHoveredUnmappedSegment, bool isActiveCell) { - if (isProofreading) { - if (isActiveCell) { - return (isHoveredUnmappedSegment - ? 0.4 - : (isHoveredSegment - ? 0.15 - : 0.0 - ) - ); - } else { - return (isHoveredSegment - ? 0.2 - : (selectiveVisibilityInProofreading ? -alpha : 0.0) - ); - } - } - - if (isHoveredSegment) { - return 0.2; - } else if (selectiveSegmentVisibility) { - return isActiveCell ? 0.15 : -alpha; - } else { - return 0.; - } - } + float getProofreadingAlphaIncrement(float alpha, bool isHoveredSegment, bool isHoveredUnmappedSegment, bool isActiveCell) { + if (isActiveCell) { + if (isHoveredUnmappedSegment) return 0.4; + if (isHoveredSegment) return 0.15; + return 0.0; + } + + if (isHoveredSegment) return 0.2; + return selectiveVisibilityInProofreading ? -alpha : 0.0; + } + + float getNormalModeAlphaIncrement(float alpha, bool isHoveredSegment, bool isActiveCell) { + if (isHoveredSegment) return 0.2; + if (!selectiveSegmentVisibility) return 0.0; + return isActiveCell ? 0.15 : -alpha; + } + + float getSegmentationAlphaIncrement(float alpha, bool isHoveredSegment, bool isHoveredUnmappedSegment, bool isActiveCell) { + return isProofreading + ? getProofreadingAlphaIncrement(alpha, isHoveredSegment, isHoveredUnmappedSegment, isActiveCell) + : getNormalModeAlphaIncrement(alpha, isHoveredSegment, isActiveCell); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/javascripts/oxalis/geometries/materials/plane_material_factory.ts(2 hunks)frontend/javascripts/oxalis/shaders/main_data_shaders.glsl.ts(4 hunks)frontend/javascripts/oxalis/shaders/segmentation.glsl.ts(1 hunks)frontend/javascripts/oxalis/store.ts(1 hunks)frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx(5 hunks)frontend/javascripts/types/schemas/dataset_view_configuration.schema.ts(1 hunks)
🔇 Additional comments (8)
frontend/javascripts/types/schemas/dataset_view_configuration.schema.ts (1)
92-92: LGTM: Configuration property added correctly.
The new selectiveSegmentVisibility property is appropriately added to the dataset view configuration with a sensible default value of false.
frontend/javascripts/oxalis/shaders/main_data_shaders.glsl.ts (1)
9-9: LGTM: Clean integration of the new shader module.
The changes correctly:
- Import the new shader module
- Declare the required uniform
- Replace the inline alpha calculation with the new modular function
Also applies to: 114-114, 297-302
frontend/javascripts/oxalis/store.ts (1)
333-333: LGTM: Property addition follows TypeScript best practices.
The new selectiveSegmentVisibility property is correctly typed and well-placed within the DatasetConfiguration interface.
frontend/javascripts/oxalis/geometries/materials/plane_material_factory.ts (2)
149-151: LGTM: New uniform property follows existing patterns.
The new selectiveSegmentVisibility uniform is correctly initialized to false and follows the same pattern as other uniform properties in the shader material.
568-578: LGTM: Listener setup is consistent with existing patterns.
The listener for selectiveSegmentVisibility is properly implemented:
- Uses the established
listenToStorePropertypattern. - Updates the uniform value when the store property changes.
- Correctly placed with other similar listeners.
frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx (3)
1589-1589: LGTM: Correctly added activeTool to props.
The activeTool is properly added to mapStateToProps to support the conditional disabling of selective visibility in proofreading mode.
1341-1341: LGTM: Added selectiveSegmentVisibility to saved settings.
The selectiveSegmentVisibility setting is correctly added to the list of settings that can be saved as default configuration.
902-928: LGTM: Well-implemented UI toggle for selective segment visibility.
The selective visibility toggle is properly implemented with:
- Clear tooltip explaining the override behavior in proofreading mode.
- Correct conditional disabling based on proofreading mode.
- Consistent styling and spacing with other settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
docs/ui/layers.md (2)
34-34: LGTM! Documentation is clear and well-placed.The explanation of the selective visibility feature is concise and helpful. Consider adding a period at the end of the sentence for consistency with other entries in this section.
- `Selective Visibility`: When activated, only segments are shown that are currently active or hovered. + `Selective Visibility`: When activated, only segments are shown that are currently active or hovered..🧰 Tools
🪛 LanguageTool
[uncategorized] ~34-~34: Loose punctuation mark.
Context: ...tween segments. -Selective Visibility: When activated, only segments are shown...(UNLIKELY_OPENING_PUNCTUATION)
34-34: Consider adding information about proofreading tool interaction.To provide complete context for users, consider mentioning that this feature is automatically disabled when switching to proofreading mode.
- `Selective Visibility`: When activated, only segments are shown that are currently active or hovered. + `Selective Visibility`: When activated, only segments are shown that are currently active or hovered. Note: This feature is automatically disabled when switching to proofreading mode, where the toolbar's selective visibility toggle takes precedence.🧰 Tools
🪛 LanguageTool
[uncategorized] ~34-~34: Loose punctuation mark.
Context: ...tween segments. -Selective Visibility: When activated, only segments are shown...(UNLIKELY_OPENING_PUNCTUATION)
frontend/javascripts/libs/input.ts (1)
574-579: Consider supporting double-clicks for middle and right mouse buttons.Currently, only left mouse button double-clicks are supported. Consider extending the functionality to support double-clicks for middle and right mouse buttons as well.
Apply this diff to add support for all mouse buttons:
doubleClick = (event: MouseEvent): void => { if (this.isHit(event)) { this.leftMouseButton.handleDoubleClick(event, false); + this.middleMouseButton.handleDoubleClick(event, false); + this.rightMouseButton.handleDoubleClick(event, false); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.unreleased.md(1 hunks)docs/ui/layers.md(1 hunks)frontend/javascripts/libs/input.ts(5 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/ui/layers.md
[uncategorized] ~34-~34: Loose punctuation mark.
Context: ...tween segments. - Selective Visibility: When activated, only segments are shown...
(UNLIKELY_OPENING_PUNCTUATION)
🔇 Additional comments (3)
CHANGELOG.unreleased.md (1)
16-16: LGTM! Clear and well-structured changelog entry.
The entry effectively communicates the new feature, its location, and functionality to users.
frontend/javascripts/libs/input.ts (2)
467-467: LGTM: Proper event listener cleanup.
The double-click event listener is correctly added in the constructor and properly cleaned up in the destroy method.
Also applies to: 520-520
574-579: Verify existing double-click handlers in the codebase.
Let's ensure there are no conflicting double-click handlers in the codebase that might interfere with this implementation.
✅ Verification successful
Based on the search results, I can now generate the final response:
Double-click implementation is consistent with existing patterns
The implementation in input.ts follows the established double-click handling patterns in the codebase:
- It's properly integrated with the existing input system that already supports double-click events
- The handler is correctly registered/unregistered using standard DOM events
- It follows the same pattern as other mouse button handlers in the codebase
- No conflicting implementations were found; other double-click handlers in the codebase are either:
- Component-specific handlers (slider, pricing enforcers)
- Tool-specific controls with proper guards
- UI elements like folder tree and dataset table
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing double-click handlers
echo "Searching for existing double-click event listeners..."
rg -i "dblclick|doubleclick" --type ts
echo "Searching for existing double-click method implementations..."
ast-grep --pattern 'function $FUNC($$$) {
$$$
dblclick
$$$
}'
Length of output: 2728
| handleDoubleClick(event: MouseEvent, triggeredByTouch: boolean): void { | ||
| // event.which is 0 on touch devices as there are no mouse buttons, interpret that as the left mouse button | ||
| // Safari doesn't support evt.buttons, but only evt.which is non-standardized | ||
| const eventWhich = event.which !== 0 ? event.which : 1; | ||
|
|
||
| if (eventWhich === this.which) { | ||
| if (this.moveDelta <= MOUSE_MOVE_DELTA_THRESHOLD) { | ||
| this.mouse.emitter.emit( | ||
| `${this.name}DoubleClick`, | ||
| this.mouse.lastPosition, | ||
| this.id, | ||
| event, | ||
| triggeredByTouch, | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Add null check for lastPosition in handleDoubleClick.
The method emits an event using this.mouse.lastPosition without checking if it's null, which could lead to runtime errors since lastPosition is declared as Point2 | null | undefined.
Apply this diff to add the safety check:
handleDoubleClick(event: MouseEvent, triggeredByTouch: boolean): void {
const eventWhich = event.which !== 0 ? event.which : 1;
if (eventWhich === this.which) {
if (this.moveDelta <= MOUSE_MOVE_DELTA_THRESHOLD) {
+ if (this.mouse.lastPosition == null) return;
this.mouse.emitter.emit(
`${this.name}DoubleClick`,
this.mouse.lastPosition,
this.id,
event,
triggeredByTouch,
);
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| handleDoubleClick(event: MouseEvent, triggeredByTouch: boolean): void { | |
| // event.which is 0 on touch devices as there are no mouse buttons, interpret that as the left mouse button | |
| // Safari doesn't support evt.buttons, but only evt.which is non-standardized | |
| const eventWhich = event.which !== 0 ? event.which : 1; | |
| if (eventWhich === this.which) { | |
| if (this.moveDelta <= MOUSE_MOVE_DELTA_THRESHOLD) { | |
| this.mouse.emitter.emit( | |
| `${this.name}DoubleClick`, | |
| this.mouse.lastPosition, | |
| this.id, | |
| event, | |
| triggeredByTouch, | |
| ); | |
| } | |
| } | |
| } | |
| handleDoubleClick(event: MouseEvent, triggeredByTouch: boolean): void { | |
| // event.which is 0 on touch devices as there are no mouse buttons, interpret that as the left mouse button | |
| // Safari doesn't support evt.buttons, but only evt.which is non-standardized | |
| const eventWhich = event.which !== 0 ? event.which : 1; | |
| if (eventWhich === this.which) { | |
| if (this.moveDelta <= MOUSE_MOVE_DELTA_THRESHOLD) { | |
| if (this.mouse.lastPosition == null) return; | |
| this.mouse.emitter.emit( | |
| `${this.name}DoubleClick`, | |
| this.mouse.lastPosition, | |
| this.id, | |
| event, | |
| triggeredByTouch, | |
| ); | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
Nice, works like a charm!
At first I was unsure whether having two settings, a general one and the one for proofreading, might be confusing, but it has definitely grown on me during the testing and I prefer it that way :)
I think I read somewhere that making this work in view mode is more involved since we don't remember the active segment there, right?
- I understand why you chose the location for the setting switch in the layers tab. However, I find it slightly unexpected that the setting is shown per segmentation layer, but changing it for one changes it for all of them. Another fitting spot without this issue would be the "Data Rendering" section of the Settings tab imo. What do you think?
| @@ -551,6 +571,12 @@ export class InputMouse { | |||
| } | |||
| }; | |||
|
|
|||
| doubleClick = (event: MouseEvent): void => { | |||
| if (this.isHit(event)) { | |||
| this.leftMouseButton.handleDoubleClick(event, false); | |||
There was a problem hiding this comment.
In line 343 it says {left,right}DoubleClick, but this looks like it only works for the left mouse button?
URL of deployed dev instance (used for testing):
Steps to test:
Issues:
(Please delete unneeded items, merge only when none are left open)