Save drawn shape on submit in single shape mode
#8807
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes in this pull request enhance the drawing functionality within the Changes
Assessment against linked issues
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
🧹 Outside diff range and nitpick comments (1)
cvat-ui/src/utils/drawing.ts (1)
19-29: Consider adding JSDoc comments for better documentation.While the function implementation is clean and handles different canvas types well, adding JSDoc comments would improve maintainability by documenting the expected behavior for different control states.
+/** + * Finishes the current drawing operation on the canvas + * @param canvas - The canvas instance (2D or 3D) + * @param activeControl - The current active control + */ export function finishDraw(canvas: Canvas | Canvas3d, activeControl: ActiveControl): void {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
cvat-ui/src/components/annotation-page/single-shape-workspace/single-shape-sidebar/single-shape-sidebar.tsx(2 hunks)cvat-ui/src/components/annotation-page/top-bar/left-group.tsx(2 hunks)cvat-ui/src/containers/annotation-page/top-bar/top-bar.tsx(2 hunks)cvat-ui/src/utils/drawing.ts(1 hunks)
🔇 Additional comments (6)
cvat-ui/src/utils/drawing.ts (1)
9-17: LGTM! Well-structured utility function.
The function provides a clear and maintainable way to check drawing control states. The comprehensive list of drawing controls makes it easy to extend in the future.
cvat-ui/src/components/annotation-page/top-bar/left-group.tsx (1)
82-82: LGTM! Good use of the utility function.
The change improves code maintainability by centralizing the drawing control logic in a dedicated utility.
cvat-ui/src/containers/annotation-page/top-bar/top-bar.tsx (2)
550-550: LGTM! Good use of the utility function.
The change improves code maintainability by moving the canvas interaction logic to a dedicated utility.
Line range hint 19-29: Verify implementation of shape saving on submit.
While the utility functions are well-implemented, I don't see direct implementation of saving shapes on submit in single shape mode. This might be implemented in other files not included in the review.
cvat-ui/src/components/annotation-page/single-shape-workspace/single-shape-sidebar/single-shape-sidebar.tsx (2)
40-40: LGTM: Clean import of drawing utilities
The new utility functions are imported cleanly and follow the existing import style.
299-304: LGTM: Proper implementation of shape saving on submit
The implementation correctly handles the shape saving process by:
- Checking if drawing can be finished using
finishDrawAvailable - Only attempting to finish drawing when available
- Using the canvas instance from the store
- Maintaining the existing control flow
This change successfully addresses the issue of shapes not being saved when clicking submit.
Let's verify the integration with the drawing utilities:
✅ Verification successful
Implementation correctly integrates with drawing utilities and maintains expected behavior
The verification confirms that:
finishDrawAvailablecorrectly checks for supported drawing controls (POLYGON, POLYLINE, POINTS, AI_TOOLS, OPENCV_TOOLS)finishDrawproperly handles both standard drawing and AI/OpenCV tools- The functions are imported and used consistently across the codebase
- Canvas draw calls are properly managed without conflicts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of finishDraw and finishDrawAvailable
# Expected: These functions should be defined in utils/drawing and handle all shape types
# Check the implementation of the drawing utilities
ast-grep --pattern 'export function finishDrawAvailable($_) {
$$$
}'
# Check for any direct canvas.draw calls that might conflict
rg -A 2 'canvas\.draw\('
Length of output: 1198
Script:
#!/bin/bash
# Let's check the implementation of both functions in the drawing utilities
rg -A 10 "export function finishDrawAvailable|export function finishDraw" cvat-ui/src/utils/drawing.ts
# Also check where these functions are imported and used
rg "import.*finishDraw.*from.*drawing"
Length of output: 1173
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
cvat-ui/src/utils/drawing.ts (1)
19-29: Consider adding JSDoc comments.The function handles different canvas types and controls well, but could benefit from documentation explaining the behavior difference between AI/OpenCV tools and other drawing modes.
+/** + * Finishes the current drawing operation based on the active control type. + * For AI/OpenCV tools, disables interaction mode. + * For other drawing controls, disables drawing mode. + * @param canvas - The canvas instance (2D or 3D) + * @param activeControl - The currently active control + */ export function finishDraw(canvas: Canvas | Canvas3d, activeControl: ActiveControl): void {cvat-ui/src/components/annotation-page/single-shape-workspace/single-shape-sidebar/single-shape-sidebar.tsx (2)
301-302: Add null check for canvas instanceConsider adding a null check for the canvas instance to prevent potential runtime errors.
- const canvas = store.getState().annotation.canvas.instance as Canvas; - finishDraw(canvas, activeControl); + const canvas = store.getState().annotation.canvas.instance; + if (canvas) { + finishDraw(canvas as Canvas, activeControl); + }
299-304: Well-structured implementation of the save-on-submit featureThe changes follow good architectural principles:
- Minimal and focused modification
- Clear separation of concerns (drawing logic in utilities)
- Maintains existing component behavior
- Type-safe implementation
This approach makes the code maintainable and testable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
cvat-ui/src/components/annotation-page/single-shape-workspace/single-shape-sidebar/single-shape-sidebar.tsx(2 hunks)cvat-ui/src/components/annotation-page/top-bar/left-group.tsx(2 hunks)cvat-ui/src/containers/annotation-page/top-bar/top-bar.tsx(2 hunks)cvat-ui/src/utils/drawing.ts(1 hunks)
🔇 Additional comments (5)
cvat-ui/src/utils/drawing.ts (1)
9-17: LGTM! Well-structured utility function.
The function provides a clear and maintainable way to check if drawing can be finished for a given control.
cvat-ui/src/components/annotation-page/top-bar/left-group.tsx (1)
21-21: LGTM! Good refactoring.
Moving the control check logic to a utility function improves code maintainability and reusability.
Also applies to: 82-82
cvat-ui/src/containers/annotation-page/top-bar/top-bar.tsx (1)
44-44: LGTM! Good simplification.
Utilizing the utility function reduces code duplication and improves maintainability by centralizing the drawing completion logic.
Also applies to: 550-550
cvat-ui/src/components/annotation-page/single-shape-workspace/single-shape-sidebar/single-shape-sidebar.tsx (2)
40-40: LGTM: Clean import of new utility functions
The new utility functions are properly imported and organized with the existing imports.
299-304: LGTM: Drawing completion logic properly integrated
The new code block correctly implements the feature to save drawn shapes on submit by checking if drawing can be finished and completing it before saving.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8807 +/- ##
========================================
Coverage 73.88% 73.88%
========================================
Files 408 409 +1
Lines 44104 44115 +11
Branches 3986 3991 +5
========================================
+ Hits 32586 32595 +9
- Misses 11518 11520 +2
|
|
The patch does not seem to work. First |
|
|
|
I’ve fixed the issue mentioned above. The problem was with the @zhiltsov-max Could you check if fix works for you? |
|
|
Looks ok, but it probably works incorrectly if there is predefined number of points used. If set to > 1, the shape is saved automatically even with less number of points. I think maybe it will be better to disable skip/submit if there are not enough points placed. Actually, it seems to be a bug in the implementation of predefined number of points - the shape can be finished by pressing N before all the required points are placed. |
Motivation and context
Resolved #8713
It would make sense to save the currently drawn shape (e.g., points, polygon, polyline) when clicking
Submiton the last frame. On non-last frames, we clickDoneto finish drawing, but on the last frame, we need to clickDoneand thenSubmit. IfDoneis not clicked, the shape is lost, which is inconvenient for users.How has this been tested?
Checklist
developbranch[ ] I have updated the documentation accordingly[ ] I have added tests to cover my changes(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
New Features
Bug Fixes
Chores