Allow merger mode when null-mapping is locked #8335
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a new optional parameter Changes
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
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 (
|
philippotto
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/javascripts/oxalis/model/reducers/settings_reducer.ts (1)
263-264: Consider using explicit boolean conversion.The double negation operator (
!!) for convertingisMergerModeMappingto boolean, while functional, could be more explicit.Consider using
Boolean(isMergerModeMapping)for clearer intent:-if (!isMappingActivationAllowed(state, mappingName, layerName, !!isMergerModeMapping)) +if (!isMappingActivationAllowed(state, mappingName, layerName, Boolean(isMergerModeMapping)))Also applies to: 267-268
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
frontend/javascripts/oxalis/api/api_latest.ts(3 hunks)frontend/javascripts/oxalis/merger_mode.ts(4 hunks)frontend/javascripts/oxalis/model/accessors/tool_accessor.ts(1 hunks)frontend/javascripts/oxalis/model/accessors/volumetracing_accessor.ts(1 hunks)frontend/javascripts/oxalis/model/actions/settings_actions.ts(2 hunks)frontend/javascripts/oxalis/model/reducers/settings_reducer.ts(2 hunks)frontend/javascripts/oxalis/model/reducers/volumetracing_reducer_helpers.ts(2 hunks)frontend/javascripts/oxalis/store.ts(1 hunks)frontend/javascripts/oxalis/view/action-bar/toolbar_view.tsx(1 hunks)
🔇 Additional comments (12)
frontend/javascripts/oxalis/model/reducers/volumetracing_reducer_helpers.ts (1)
23-23: LGTM! Clean import addition.The addition of
getMappingInfoimport aligns with the codebase's modular structure.frontend/javascripts/oxalis/model/actions/settings_actions.ts (2)
202-202: LGTM! Type definition extension.The addition of
isMergerModeMappingtoOptionalMappingPropertiesproperly extends the type system to support merger mode functionality.
209-215: LGTM! Action creator update.The
setMappingActionis correctly updated to include the newisMergerModeMappingproperty in both its parameters and return value.Also applies to: 226-226
frontend/javascripts/oxalis/model/reducers/settings_reducer.ts (1)
285-285: LGTM! State update.The
isMergerModeMappingproperty is correctly included in the state update.frontend/javascripts/oxalis/model/accessors/tool_accessor.ts (1)
39-39: LGTM! Improved error message.The updated error message provides clearer guidance by mentioning both options: enabling a segmentation layer or making it editable via the lock icon.
frontend/javascripts/oxalis/merger_mode.ts (1)
190-190: LGTM! Consistent implementation of merger mode mapping.The changes systematically add the
isMergerModeMappingflag to allsetMappingcalls, properly identifying mappings that are specifically related to merger mode operations.Also applies to: 242-244, 287-289, 419-419
frontend/javascripts/oxalis/store.ts (1)
427-427: LGTM! Well-typed property addition.The optional boolean property
isMergerModeMappingis properly added to theActiveMappingInfotype, enabling type safety for merger mode mapping operations.frontend/javascripts/oxalis/model/accessors/volumetracing_accessor.ts (1)
612-630: LGTM! Well-implemented mapping activation logic.The changes properly handle merger mode mapping activation by:
- Adding an optional
isMergerModeMappingparameter- Implementing clear conditions for when merger mode mappings are allowed
- Maintaining backward compatibility
frontend/javascripts/oxalis/view/action-bar/toolbar_view.tsx (1)
401-405: LGTM! Enhanced UI logic for merger mode.The changes improve the user experience by:
- Making the mapping lock check more precise with
isMappingLockedWithNonNull- Providing clearer guidance in the tooltip when merger mode is disabled
- Properly integrating with the backend changes
Also applies to: 408-410
frontend/javascripts/oxalis/api/api_latest.ts (3)
1684-1684: LGTM! The new optional parameter is well-defined.The addition of the optional
isMergerModeMappingparameter is correctly typed and follows TypeScript best practices.
1693-1698: LGTM! The parameter destructuring is well-organized.The destructuring of
isMergerModeMappingis properly placed alongside other option parameters, maintaining code consistency.
1714-1714: LGTM! The parameter is correctly passed to mapping properties.The
isMergerModeMappingparameter is properly included in the mapping properties, ensuring it's passed through to the mapping action. Let's verify the implementation:✅ Verification successful
Implementation verified: isMergerModeMapping is properly integrated
The parameter is correctly implemented across the entire flow:
- Properly typed and handled in actions
- Correctly processed in reducer with proper state management
- Properly validated in accessor with special handling for merger mode mappings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the isMergerModeMapping parameter is used correctly in related files. # Test: Check for the usage of isMergerModeMapping in mapping-related files echo "Checking for isMergerModeMapping usage in mapping-related files..." rg -l "isMergerModeMapping" "frontend/javascripts/oxalis/model/" # Test: Check for the parameter in action creators and reducers echo "Checking for the parameter in action creators and reducers..." rg -l "isMergerModeMapping" "frontend/javascripts/oxalis/model/actions/settings_actions.ts" "frontend/javascripts/oxalis/model/reducers/settings_reducer.ts"Length of output: 811
Script:
#!/bin/bash # Check the implementation details in each file echo "Checking settings_actions.ts implementation:" rg "isMergerModeMapping" frontend/javascripts/oxalis/model/actions/settings_actions.ts -B 2 -A 2 echo -e "\nChecking settings_reducer.ts implementation:" rg "isMergerModeMapping" frontend/javascripts/oxalis/model/reducers/settings_reducer.ts -B 2 -A 2 echo -e "\nChecking volumetracing_accessor.ts implementation:" rg "isMergerModeMapping" frontend/javascripts/oxalis/model/accessors/volumetracing_accessor.ts -B 2 -A 2Length of output: 1991
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.unreleased.md (1)
25-25: Improve clarity of the changelog entry.While the changelog entry accurately describes the feature, it could be clearer for users who might not be familiar with technical terms like "merger mode" and "mapping". Consider revising the entry to be more descriptive and user-focused.
-- Added the possibility to use the "merger mode" even when the user has annotated volume data in the current layer (as long as no other mapping is active). [#8335](https://github.com/scalableminds/webknossos/pull/8335) ++ Added the ability to merge segments (merger mode) in a layer that contains user annotations, provided no segment ID mapping is currently active. This allows users to continue merging segments even after they've started annotating. [#8335](https://github.com/scalableminds/webknossos/pull/8335)
philippotto
requested review from
MichaelBuessemeyer
and removed request for
daniel-wer
There was a problem hiding this comment.
Thanks for this PR 🙏
To me the code looks, ok. But it introduces another mapping case. Hopefully, this won't be needed soon once the identity mapping is live an a little more established.
During testing I noticed that when switching between active volume layers when the merger mode is active, the "merging" is not reapplied to the newly active layer. But imo this isn't such a great deal. It can be solved by re activating the merger mode. So I see not priority there (but still wanted to mention it)
That's all.. The testing worked perfectly 🎉
| @@ -20,7 +20,7 @@ import type { | |||
| SegmentMap, | |||
| VolumeTracing, | |||
| } from "oxalis/store"; | |||
| import { getMaximumSegmentIdForLayer } from "../accessors/dataset_accessor"; | |||
| import { getMappingInfo, getMaximumSegmentIdForLayer } from "../accessors/dataset_accessor"; | |||
There was a problem hiding this comment.
The first import here is unused
| import { getMappingInfo, getMaximumSegmentIdForLayer } from "../accessors/dataset_accessor"; | |
| import { getMaximumSegmentIdForLayer } from "../accessors/dataset_accessor"; |
There was a problem hiding this comment.
hm, I wonder why the linter didn't catch that? 🤔 do you have an idea?
There was a problem hiding this comment.
Oh I did not realize that the ci was green at this point. Well ...... I have no initial idea why this might be happening 🤔
|
@philippotto I just noticed that in your pr the materialization button for a merge mode annotation is missing. Do you think this is related? |
|
Thanks for taking over the review for this PR @MichaelBuessemeyer! I had it on my list for today (since I didn't notice the reassignment) but you were faster :) |
The button is only visible when a WK worker is set up. we could always show it and disable it (like we do it for other functionality), but it's unrelated to this PR. |
…ger-mode-brushing
URL of deployed dev instance (used for testing):
Steps to test:
Issues:
(Please delete unneeded items, merge only when none are left open)