Allow to restrict floodfill to a bounding box #8267
Conversation
…en; also disable animations for the context menu
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces a new feature allowing the flood fill tool to be restricted to a specified bounding box. The changes span multiple files in the frontend, adding a toggle in the toolbar to enable bounding box restriction for flood fill operations. A new constant and configuration option have been added to manage this functionality, with modifications to the flood fill saga and data cube handling to support the restricted fill mode. The implementation ensures that flood fill operations can be constrained within user-defined or automatically generated bounding boxes. 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 (
|
…triction is user imposed; also fix close link in toast
…ox is not too large
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: 2
🧹 Nitpick comments (9)
frontend/javascripts/oxalis/model/sagas/volume/floodfill_saga.tsx (1)
35-102: Ensure proper handling of failure reasons ingetBoundingBoxForFloodFillIn the
getBoundingBoxForFloodFillfunction, when returning failure reasons, ensure that the failure messages are user-friendly and informative. This will enhance the user's understanding when the flood fill operation cannot proceed.frontend/javascripts/test/sagas/volumetracing/volumetracing_saga_integration.spec.ts (1)
Line range hint
330-347: Improve test reliability with explicit undo actionsWhen performing undo and redo operations in the test, ensure that each action is awaited properly to prevent race conditions and enhance test stability.
Apply this diff to await undo and redo actions explicitly:
- await dispatchUndoAsync(Store.dispatch); + await dispatchUndoAsync(Store.dispatch); + await t.context.api.tracing.save(); - await dispatchRedoAsync(Store.dispatch); + await dispatchRedoAsync(Store.dispatch); + await t.context.api.tracing.save();frontend/stylesheets/trace_view/_action_bar.less (1)
71-74: Consider using a more semantic class nameThe class name
.undo-redo-buttonsuggests specific usage, but the functionality (preventing width flickering) could be useful for other buttons. Consider a more generic name like.fixed-width-buttonor.no-flicker-button.CHANGELOG.unreleased.md (1)
16-16: Enhance the changelog entry with more detailsConsider expanding the changelog entry to provide more comprehensive information about the feature:
-The fill tool can now be adapted so that it only acts within a specified bounding box. Use the new "Restrict Floodfill" mode for that in the toolbar. [#8267](https://github.com/scalableminds/webknossos/pull/8267) +The fill tool can now be restricted to operate within a specified bounding box. Enable the new "Restrict Floodfill" toggle in the toolbar when the fill tool is selected. If no bounding box exists when using this mode, a notification will be displayed. This feature helps prevent unintended fill operations outside the area of interest. [#8267](https://github.com/scalableminds/webknossos/pull/8267)frontend/javascripts/oxalis/default_state.ts (1)
87-87: LGTM! Consider adding JSDocThe new configuration property is well-named and appropriately placed. Consider adding JSDoc documentation to describe its purpose and behavior:
+ /** When enabled, restricts flood fill operations to the current bounding box */ isFloodfillRestrictedToBoundingBox: false,frontend/javascripts/oxalis/constants.ts (1)
336-338: Consider adding more detailed documentation.While the comment explains the purpose, it would be helpful to document:
- Why the value of 10 was chosen
- What "more lax" means in terms of actual behavior
- The impact on performance and memory usage
frontend/javascripts/oxalis/model/bucket_data_handling/data_cube.ts (2)
501-509: Consider adding more detailed documentation for the dynamic bounding box expansionWhile the implementation is solid, the comment could be more explicit about:
- The purpose of allowing multiple additions of the same bucket
- The implications of the "neighbour volume shape" concept
- How this relates to the new bounding box restriction feature
695-724: Consider enhancing the progress loggingThe progress logging implementation could be improved:
- Consider using a constant for the progress update interval (1000000)
- Consider adding an estimate of total progress (e.g., percentage complete)
- Consider adding more context about the operation in the progress message
- if (labeledVoxelCount % 1000000 === 0) { - console.log(`Labeled ${labeledVoxelCount} Vx. Continuing...`); - - await progressCallback( - false, - `Labeled ${labeledVoxelCount / 1000000} MVx. Continuing...`, - ); + const PROGRESS_UPDATE_INTERVAL = 1000000; + if (labeledVoxelCount % PROGRESS_UPDATE_INTERVAL === 0) { + const message = `Flood fill: Labeled ${(labeledVoxelCount / 1000000).toFixed(1)} MVx`; + console.log(message); + await progressCallback(false, message);frontend/javascripts/oxalis/view/action-bar/toolbar_view.tsx (1)
1372-1405: Consider enhancing the FloodFillSettings componentWhile the implementation is solid, consider these improvements:
- Extract the tooltip text to a constant for reusability
- Add a data-testid attribute for easier testing
- Consider using a memo hook for the toggle callback to prevent unnecessary re-renders
+const FLOOD_FILL_BBOX_TOOLTIP = + "When enabled, the floodfill will be restricted to the bounding box enclosed by the clicked position. If multiple bounding boxes enclose that position, the smallest is used."; function FloodFillSettings() { const dispatch = useDispatch(); const isRestrictedToBoundingBox = useSelector( (state: OxalisState) => state.userConfiguration.isFloodfillRestrictedToBoundingBox, ); - const toggleRestrictFloodfillToBoundingBox = () => { + const toggleRestrictFloodfillToBoundingBox = useCallback(() => { dispatch( updateUserSettingAction("isFloodfillRestrictedToBoundingBox", !isRestrictedToBoundingBox), ); - }; + }, [dispatch, isRestrictedToBoundingBox]); return ( - <div> + <div data-testid="flood-fill-settings"> <FillModeSwitch /> <ButtonComponent style={{ opacity: isRestrictedToBoundingBox ? 1 : 0.5, marginLeft: 12, }} type={isRestrictedToBoundingBox ? "primary" : "default"} onClick={toggleRestrictFloodfillToBoundingBox} - title={ - "When enabled, the floodfill will be restricted to the bounding box enclosed by the clicked position. If multiple bounding boxes enclose that position, the smallest is used." - } + title={FLOOD_FILL_BBOX_TOOLTIP} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
public/images/icon-restrict-floodfill-to-bbox.svgis excluded by!**/*.svg
📒 Files selected for processing (18)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/components/async_clickables.tsx(2 hunks)frontend/javascripts/libs/mjs.ts(1 hunks)frontend/javascripts/oxalis/constants.ts(1 hunks)frontend/javascripts/oxalis/controller/combinations/bounding_box_handlers.ts(1 hunks)frontend/javascripts/oxalis/default_state.ts(1 hunks)frontend/javascripts/oxalis/model/accessors/tracing_accessor.ts(2 hunks)frontend/javascripts/oxalis/model/bucket_data_handling/bucket.ts(2 hunks)frontend/javascripts/oxalis/model/bucket_data_handling/data_cube.ts(4 hunks)frontend/javascripts/oxalis/model/sagas/volume/floodfill_saga.tsx(1 hunks)frontend/javascripts/oxalis/model/sagas/volumetracing_saga.tsx(3 hunks)frontend/javascripts/oxalis/store.ts(1 hunks)frontend/javascripts/oxalis/view/action-bar/toolbar_view.tsx(2 hunks)frontend/javascripts/oxalis/view/action-bar/tracing_actions_view.tsx(2 hunks)frontend/javascripts/oxalis/view/context_menu.tsx(3 hunks)frontend/javascripts/test/sagas/volumetracing/volumetracing_saga_integration.spec.ts(4 hunks)frontend/stylesheets/trace_view/_action_bar.less(1 hunks)tools/test.sh(1 hunks)
🧰 Additional context used
📓 Learnings (1)
frontend/javascripts/oxalis/model/sagas/volumetracing_saga.tsx (1)
Learnt from: dieknolle3333
PR: scalableminds/webknossos#8168
File: frontend/javascripts/oxalis/model/sagas/volumetracing_saga.tsx:433-434
Timestamp: 2024-11-22T17:19:07.947Z
Learning: In the codebase, certain usages of `segmentationLayer.resolutions` are intentionally retained and should not be changed to `segmentationLayer.mags` during refactoring.
🔇 Additional comments (13)
frontend/javascripts/oxalis/model/sagas/volumetracing_saga.tsx (1)
13-13:
Retain usage of segmentationLayer.resolutions where necessary
According to previous learnings, certain usages of segmentationLayer.resolutions should not be replaced with segmentationLayer.mags. Please verify that all such intentional usages remain unchanged.
Run the following script to identify unintended replacements:
frontend/javascripts/test/sagas/volumetracing/volumetracing_saga_integration.spec.ts (1)
Line range hint 277-305: Ensure consistent use of EXPECTED_HALF_EXTENT in tests
In the new test case, EXPECTED_HALF_EXTENT is calculated using Constants.FLOOD_FILL_EXTENTS. Verify that this matches the actual extents used in the flood fill operation to ensure the test accurately reflects the behavior.
frontend/javascripts/components/async_clickables.tsx (1)
50-55: LGTM! Good UX improvement
The addition of ConfigProvider to disable motion effects is a clean solution to prevent jarring animations during icon swaps.
frontend/javascripts/oxalis/model/accessors/tracing_accessor.ts (2)
91-95: LGTM! Robust null handling
The function safely handles the case where tracing might be null by leveraging the existing maybeGetSomeTracing helper.
97-104: Consider memoizing the BoundingBox instances
Creating new BoundingBox instances on every filter iteration could be inefficient for large numbers of boxes. Consider transforming the bounding boxes into BoundingBox instances once, then reusing them.
export const getUserBoundingBoxesThatContainPosition = (
state: OxalisState,
position: Vector3,
): Array<UserBoundingBox> => {
const bboxes = getUserBoundingBoxesFromState(state);
+ const boundingBoxInstances = bboxes.map(
+ (el) => new BoundingBox(el.boundingBox)
+ );
- return bboxes.filter((el) => new BoundingBox(el.boundingBox).containsPoint(position));
+ return bboxes.filter((_, idx) =>
+ boundingBoxInstances[idx].containsPoint(position)
+ );
};Let's verify the BoundingBox implementation:
tools/test.sh (1)
93-93: LGTM! Improved test file filtering
The addition of grep -E -v "\.(md|snap)" correctly excludes markdown and snapshot files from the test execution, making the test runs more focused and efficient.
frontend/javascripts/libs/mjs.ts (1)
253-255: LGTM: Simple and correct implementation of vector product.
The prod method correctly calculates the product of vector components, which is useful for computing areas (V2) or volumes (V3).
frontend/javascripts/oxalis/controller/combinations/bounding_box_handlers.ts (1)
245-247: LGTM: Good type safety improvement.
The explicit third argument to V3.add ensures a Vector3 is returned instead of Float32Array, making the code more type-safe.
frontend/javascripts/oxalis/store.ts (1)
397-397: LGTM: Clean addition of flood fill restriction configuration
The new boolean property isFloodfillRestrictedToBoundingBox is properly typed and well-placed within the UserConfiguration interface.
frontend/javascripts/oxalis/view/action-bar/tracing_actions_view.tsx (1)
Line range hint 499-507: LGTM: Improved button styling consistency
The addition of the 'undo-redo-button' class to both undo and redo buttons helps prevent width flickering. The 'hide-on-small-screen' class on the redo button maintains proper responsive behavior.
Also applies to: 509-517
frontend/javascripts/oxalis/model/bucket_data_handling/bucket.ts (2)
42-45: LGTM: Well-implemented throttled warning
The new warning function follows the established pattern of throttled warnings and helps prevent warning spam while maintaining error visibility.
780-780: LGTM: Improved error handling consistency
Good replacement of direct warning with the throttled version, maintaining functionality while improving error handling consistency.
frontend/javascripts/oxalis/model/bucket_data_handling/data_cube.ts (1)
490-491: LGTM: Good use of BoundingBox class for type safety
The parameter renaming and wrapping in BoundingBox class is a good practice that provides type safety and maintainability.
Also applies to: 510-511
frontend/javascripts/oxalis/model/sagas/volume/floodfill_saga.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/volume_annotation/tools.md (1)
71-72: Enhance the documentation with additional implementation details.The documentation provides a good overview of the new feature, but could be enhanced with the following details from the PR objectives:
- Clarify the toggle behavior:
- When disabled: Flood fill operates normally
- When enabled: Fill is limited to the bounding box
- Add a note about the notification shown when no bounding box exists
- Explain the 3D mode behavior regarding early termination and bounding box creation
Consider expanding the section with this additional content:
{align=left width="60"} **Restrict Fill by Bounding Box**: When enabled, the fill operation will be restricted by the smallest bounding box that encloses the clicked position. This feature can be useful when correcting segmentation in a small bounding box (e.g., when curating training data). + +When the toggle is disabled, flood fill operates normally. When enabled, the fill will be limited to the bounding box encompassing the clicked position. If no bounding box exists, you'll receive a notification. In 3D mode, a new bounding box will only be created on early termination if the restriction is disabled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/images/icon_restricted_floodfill.jpgis excluded by!**/*.jpg
📒 Files selected for processing (1)
docs/volume_annotation/tools.md(1 hunks)
…ple floodfills; also don't show bbox message if floodfill was constraint to bbox
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/javascripts/oxalis/model/sagas/volume/floodfill_saga.tsx (2)
53-54: Consider extracting repeated volume calculation logic into a helper function.Here, you use new BoundingBox(bbox.boundingBox).getVolume() while also using a similar logic for 2D calculations below. Factoring out this logic would reduce duplication and improve readability.
69-72: Provide localized or more concise error messaging if needed.The error message might be displayed to end-users verbatim. If you plan to support multiple languages or wish to keep messages more concise, consider externalizing or simplifying the string.
frontend/javascripts/libs/progress_callback.ts (1)
14-14: Document the final feedback method parameter.The “finalFeedbackMethod” is intuitive, but consider adding a brief line in the code comments about acceptable values (“success”, “error”, “info”, “warning”) for self-documentation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/javascripts/libs/progress_callback.ts(2 hunks)frontend/javascripts/oxalis/model/sagas/volume/floodfill_saga.tsx(1 hunks)
🔇 Additional comments (4)
frontend/javascripts/oxalis/model/sagas/volume/floodfill_saga.tsx (3)
106-110: Clarify concurrency behavior in the while loop.
Using a while loop with yield* take("FLOOD_FILL") may sequentially process requests, but consider explaining or documenting in comments how concurrent flood fill operations should be handled. If concurrency is not intended, ensuring there's only one active flood fill at a time is fine.
153-156: Reminder: Handle potential ongoing operations.
This code checks if the UI is busy, but does not handle an ongoing flood fill. In a previous review comment, concurrency handling was suggested; re-verify if additional checks are needed so that multiple fill operations do not overlap.
190-199: Deferred bounding box creation logic.
When wasBoundingBoxExceeded is true, you handle the special case (line ~257) of skipping bounding box creation in restricted mode. Ensure that you have confirmed this meets the PR’s objective to avoid inadvertently ignoring large bounding boxes in restricted mode.
frontend/javascripts/libs/progress_callback.ts (1)
3-5: Good use of type alias for extensibility.
Defining HideFn as a dedicated type clarifies the code’s intent and will help if you expand these functions later.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/javascripts/test/sagas/saga_integration.mock.ts (1)
18-21: LGTM! Consider clarifying the comment.The changes improve the mock's fidelity by returning hide functions, which better matches the actual
antdmessage API behavior.Consider making the comment more explicit about the API behavior:
- // These return a "hide function" + // These methods return a function that can be called to hide/close the message
frontend/javascripts/oxalis/model/sagas/volume/floodfill_saga.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/model/sagas/volume/floodfill_saga.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/test/sagas/volumetracing/volumetracing_saga_integration.spec.ts
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/model/sagas/volume/floodfill_saga.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/model/sagas/volume/floodfill_saga.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/model/sagas/volume/floodfill_saga.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/model/sagas/volume/floodfill_saga.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/model/sagas/volume/floodfill_saga.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Nice, thanks for this PR 🎉 🪣
I left some suggestions. Besides these suggestions the PR looks totally fine to me and also works great. The fast closing in case of a fast floodfill operation is awesome btw :D
Let me know if you want to skip some suggestions so I can rereview / approve
frontend/javascripts/oxalis/model/sagas/volume/floodfill_saga.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/model/sagas/volume/floodfill_saga.tsx
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,32 @@ | |||
| <?xml version="1.0" encoding="UTF-8" standalone="no"?> | |||
There was a problem hiding this comment.
Nice image. But maybe the color bucket as used in the flood fill tool icon would be more consistent instead of a color / water drop 🤔
There was a problem hiding this comment.
hmm, I think, it's fine. the icon should be easily discernable from the floodfill icon. so, only reusing the drop and not the bucket seems like a good choice imo.
|
thanks for your review, @MichaelBuessemeyer! could you have another look? :) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/javascripts/oxalis/model/sagas/volume/floodfill_saga.tsx (2)
36-37: Constants appear clear and descriptive.Using separate named constants (
NO_FLOODFILL_BBOX_TOAST_KEYandNO_SUCCESS_MSG_WHEN_WITHIN_MS) helps maintain readability. Consider documenting in code comments how these constants were chosen, to guide future refactoring or tuning efforts.
277-356: User notification flow is thorough.The
notifyUserAboutResultgenerator provides detailed feedback on bounding box usage, early termination, and quick fills. Keep in mind that heavier UI logic, such as large JSX blocks, might be extracted into a separate helper or a dedicated UI component for better separation of concerns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/volume_annotation/images/icon_restricted_floodfill.jpgis excluded by!**/*.jpg
📒 Files selected for processing (2)
frontend/javascripts/oxalis/model/actions/volumetracing_actions.ts(1 hunks)frontend/javascripts/oxalis/model/sagas/volume/floodfill_saga.tsx(1 hunks)
🔇 Additional comments (7)
frontend/javascripts/oxalis/model/actions/volumetracing_actions.ts (1)
17-17: Good job makingFloodFillActionpublic.Exporting this type increases maintainability and allows it to be reused more flexibly throughout the codebase. This appears well-aligned with the newly introduced flood fill logic in the sagas.
frontend/javascripts/oxalis/model/sagas/volume/floodfill_saga.tsx (6)
1-13: Imports are well-structured.All imported modules and types seem properly referenced, and no apparent duplicates or missing dependencies are detected.
39-42: Solid saga registration.The
floodFillsaga pattern, waiting for"INITIALIZE_VOLUMETRACING"then taking every"FLOOD_FILL"action, is straightforward and fits well within Redux-Saga best practices.
44-73: Choice of smallest bounding box may warrant a functional comment.Selecting the smallest bounding box that encloses the clicked position can be valid, but if multiple bounding boxes overlap, users might expect other behaviors (e.g., the most recently created bounding box, etc.). If this is the intended design, consider adding clarifying comments or usage docs.
75-102: Unrestricted bounding box calculation is concise.Restricting the z-dimension to a single slice in
_2Dmode matches expectations. No performance or logical pitfalls stand out.
104-116: Bounding box selection logic is straightforward.Switching between restricted and unrestricted modes via
isFloodfillRestrictedToBoundingBoxis clear. No issues detected.
118-275: Comprehensive flood fill handler—consider further partitioning.The
handleFloodFillgenerator function is quite large, handling toast notifications, concurrency checks, bounding box setup, data manipulation, and final result dispatching. Extracting parts (like labeling, concurrency validation, or final updates) into smaller functions can improve readability and testability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.unreleased.md (1)
16-16: Consider enhancing the changelog entry with additional details.The entry could be more comprehensive by including:
- The special behavior in 3D mode regarding bounding box creation
- Any limitations or considerations when working with large bounding boxes
Consider expanding the entry like this:
- The fill tool can now be adapted so that it only acts within a specified bounding box. Use the new "Restrict Floodfill" mode for that in the toolbar. [#8267](https://github.com/scalableminds/webknossos/pull/8267) + The fill tool can now be adapted so that it only acts within a specified bounding box. Use the new "Restrict Floodfill" mode for that in the toolbar. In 3D mode, a new bounding box is created if the flood fill terminates early. Note that performance may be impacted when using very large bounding boxes. [#8267](https://github.com/scalableminds/webknossos/pull/8267)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/oxalis/store.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/oxalis/store.ts
🔇 Additional comments (2)
CHANGELOG.unreleased.md (2)
16-17: LGTM! Clear and well-documented changelog entries.The changelog entries are well-written, providing clear information about the new features and how to use them. The entries correctly reference their respective PRs and follow the Keep a Changelog format.
Line range hint
48-48: Consider adding a Breaking Changes entry.The introduction of
FLOOD_FILL_MULTIPLIER_FOR_BBOX_RESTRICTIONmight affect existing flood fill operations. If this change impacts the behavior significantly, it should be documented in the Breaking Changes section.Could you verify if this constant significantly changes the flood fill behavior? If so, consider adding an entry like:
### Breaking Changes + - The flood fill behavior has been modified with the introduction of `FLOOD_FILL_MULTIPLIER_FOR_BBOX_RESTRICTION`, which may affect the performance and extent of flood fill operations. [#8267](https://github.com/scalableminds/webknossos/pull/8267)🧰 Tools
🪛 LanguageTool
[duplication] ~13-~13: Possible typo: you repeated a word
Context: ...webknossos/compare/24.12.0...HEAD) ### Added - Added the total volume of a dataset to a tool...(ENGLISH_WORD_REPEAT_RULE)
URL of deployed dev instance (used for testing):
Steps to test:
TODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)