Hide mask during editing #8554
Hide mask during editing #8554
Conversation
WalkthroughThe changes introduced in this pull request enhance the configuration and visibility management of edited objects in the canvas model. A new optional property Changes
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: 9
🧹 Outside diff range and nitpick comments (7)
cvat-canvas/src/typescript/canvasModel.ts (1)
986-989: Implementation ofhideEditedObjectconfigurationThe
configuremethod has been updated to handle the newhideEditedObjectoption. This allows users to dynamically change the visibility of edited objects during runtime.However, consider adding a comment explaining the purpose and impact of this new configuration option for better maintainability.
if (typeof configuration.hideEditedObject === 'boolean') { + // Controls whether the object being edited should be hidden during the editing process this.data.configuration.hideEditedObject = configuration.hideEditedObject; }cvat-ui/src/reducers/annotation-reducer.ts (1)
644-657: LGTM: UPDATE_EDITED_STATE case implementation with a minor suggestionThe UPDATE_EDITED_STATE case correctly updates the
shapeTypeandeditedStateInstanceproperties of theeditedStateobject. The use of the nullish coalescing operator (??) ensures that null values are set when the payload properties are undefined.For consistency with the RESET_EDITED_STATE case, consider explicitly preserving the
editedStateHiddenproperty:editedState: { ...state.annotations.editedState, shapeType: (shapeType ?? null), editedStateInstance: (editedStateInstance ?? null), + editedStateHidden: state.annotations.editedState.editedStateHidden, },This change isn't strictly necessary due to the spread operator, but it makes the code more explicit and consistent with the other cases.
cvat-ui/src/components/annotation-page/canvas/views/canvas2d/brush-tools.tsx (2)
373-383: SimplifyclassNameconstruction for improved readabilityThe
classNameproperty is currently constructed using array concatenation andjoin, which can be simplified for better readability. Consider using template literals or theclassnamesutility for cleaner code.For example, you can rewrite it as:
className={`cvat-brush-tools-hide ${editedState.editedStateHidden ? 'cvat-brush-tools-active-tool' : ''}`}
373-383: Add unit tests for the new hide/show edited mask functionalityTo ensure the new feature works correctly and to prevent future regressions, please consider adding unit tests that cover the hide/show functionality of the edited mask.
Would you like assistance in creating these tests or opening a GitHub issue to track this task?
cvat-canvas/src/typescript/svg.patch.ts (1)
Line range hint
99-100: Simplify array destructuring forthis.p.xandthis.p.yCurrently,
this.p.xandthis.p.yare assigned separately fromarray[i]. You can simplify the code by destructuring both values in a single statement.Apply this diff:
- [this.p.x] = array[i]; - [, this.p.y] = array[i]; + [this.p.x, this.p.y] = array[i];cvat-canvas/src/typescript/drawHandler.ts (2)
1309-1317: Avoid using 'any'; define proper typings for 'paintHandler'.The use of
as anywhen castingpaintHandlercan obscure type checking and potentially introduce errors. Consider defining an interface or type forpaintHandlerto enhance type safety and maintainability.Suggested improvement:
Define a type for
paintHandlerif possible, for example:interface PaintHandler { set: { members: SVG.Element[]; }; }Then, you can use:
const paintHandler = this.drawInstance.remember('_paintHandler') as PaintHandler;
Line range hint
1319-1343: Fix typo in variable name 'isFilalblePolygon'.The variable
isFilalblePolygonappears to be misspelled. It should beisFillablePolygonto maintain consistency and readability.Apply this diff to correct the typo:
- const isFilalblePolygon = this.drawData && this.drawData.shapeType === 'polygon'; + const isFillablePolygon = this.drawData && this.drawData.shapeType === 'polygon';Also, update all usages of
isFilalblePolygontoisFillablePolygonwithin the method:- if (this.drawInstance && (isFilalblePolygon)) { + if (this.drawInstance && (isFillablePolygon)) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- cvat-canvas/src/typescript/canvasModel.ts (3 hunks)
- cvat-canvas/src/typescript/drawHandler.ts (6 hunks)
- cvat-canvas/src/typescript/masksHandler.ts (8 hunks)
- cvat-canvas/src/typescript/svg.patch.ts (1 hunks)
- cvat-ui/src/actions/annotation-actions.ts (2 hunks)
- cvat-ui/src/components/annotation-page/canvas/views/canvas2d/brush-tools.tsx (5 hunks)
- cvat-ui/src/components/annotation-page/canvas/views/canvas2d/canvas-wrapper.tsx (9 hunks)
- cvat-ui/src/containers/annotation-page/standard-workspace/objects-side-bar/object-buttons.tsx (5 hunks)
- cvat-ui/src/containers/annotation-page/standard-workspace/objects-side-bar/objects-list.tsx (8 hunks)
- cvat-ui/src/reducers/annotation-reducer.ts (2 hunks)
- cvat-ui/src/reducers/index.ts (3 hunks)
🧰 Additional context used
🔇 Additional comments (48)
cvat-ui/src/reducers/index.ts (4)
12-12: New import added for ObjectStateThe
ObjectStateimport has been added from 'cvat-core-wrapper'. This is likely to support the newEditedStateinterface.
696-700: New EditedState interface looks goodThe new
EditedStateinterface is well-structured and provides a clear representation of an edited state in the application. It includes:
shapeType: Allows for aShapeTypeornull, providing flexibility.editedStateInstance: Can be anObjectStateornull, which is good for handling various scenarios.editedStateHidden: A boolean flag for visibility control.This interface will help in managing and tracking edited states in the application.
780-780: AnnotationState interface updated with editedState propertyThe
AnnotationStateinterface has been updated to include aneditedStateproperty of typeEditedState. This addition is consistent with the newly createdEditedStateinterface and will allow for tracking the currently edited state within the annotation process.
12-12: Summary of changes: Enhanced state management for edited statesThe changes introduced in this file improve the state management capabilities for edited states in the annotation process. Here's a summary of the modifications:
- Added import for
ObjectStatefrom 'cvat-core-wrapper'.- Introduced a new
EditedStateinterface to represent the state of edited objects.- Updated the
AnnotationStateinterface to include aneditedStateproperty of typeEditedState.These changes provide a more structured way to handle and track edited states during the annotation process, which should improve the overall functionality and maintainability of the application.
Also applies to: 696-700, 780-780
cvat-canvas/src/typescript/canvasModel.ts (3)
99-99: New configuration option added:hideEditedObjectA new optional boolean property
hideEditedObjecthas been added to the Configuration interface. This addition allows for more flexible control over the visibility of edited objects in the canvas.
420-420: Default value set for new configuration optionThe
hideEditedObjectproperty is initialized with a default value offalsein the CanvasModelImpl constructor. This ensures backward compatibility with existing implementations.
99-99: Summary of changes: New configuration option for hiding edited objectsThe changes introduce a new configuration option
hideEditedObjectto the CanvasModel. This boolean option allows for controlling the visibility of objects being edited. The implementation includes:
- Adding the option to the Configuration interface
- Setting a default value in the CanvasModelImpl constructor
- Handling the option in the configure method
These changes provide more flexibility in how edited objects are displayed, which could be useful for various use cases such as reducing visual clutter or focusing on specific parts of an annotation.
To fully utilize this new feature, make sure to update any relevant UI components or documentation to reflect this new configuration option.
Also applies to: 420-420, 986-989
cvat-ui/src/reducers/annotation-reducer.ts (3)
101-105: NeweditedStateproperty added toannotationsThe
editedStateproperty has been added to theannotationsobject in thedefaultState. This new property includesshapeType,editedStateInstance, andeditedStateHidden, which are all initialized with null or false values. This addition allows for tracking the current editing state of annotations.
630-643: LGTM: RESET_EDITED_STATE case implementationThe RESET_EDITED_STATE case correctly resets all properties of the
editedStateobject to their initial values. This implementation ensures that the editing state can be cleared when necessary.
658-670: LGTM: HIDE_EDITED_STATE case implementationThe HIDE_EDITED_STATE case correctly updates the
editedStateHiddenproperty of theeditedStateobject. The implementation preserves the other properties ofeditedStatewhile updating the visibility state.cvat-ui/src/containers/annotation-page/standard-workspace/objects-side-bar/object-buttons.tsx (6)
12-13: Import Statements Updated CorrectlyThe addition of
changeHideEditedStateAsyncandEditedStateto the import statements is appropriate and follows the project's import conventions.
32-32: AddededitedStatetoStateToPropsInterfaceIncluding
editedState: EditedStatein theStateToPropsinterface ensures that the component has access to the edited state from the Redux store.
38-38: AddedchangeHideEditedStatetoDispatchToPropsInterfaceDefining
changeHideEditedState(value: boolean): voidin theDispatchToPropsinterface allows the component to dispatch actions to change the hide state of the edited object.
44-44: DestructurededitedStatefrom Redux StateIn
mapStateToProps, extractingeditedStatefromannotation.annotationsensures that the component receives the current edited state.
66-66: PassededitedStateto Component PropsIncluding
editedStatein the returned object ofmapStateToPropsmakes it available to the component, enabling conditional logic based on the edited state.
82-84: ImplementedchangeHideEditedStateinmapDispatchToPropsThe
changeHideEditedStatefunction correctly dispatches thechangeHideEditedStateAsyncaction, aligning with Redux Thunk practices.cvat-ui/src/components/annotation-page/canvas/views/canvas2d/brush-tools.tsx (1)
107-109: Well-structured use ofuseCallbackforhideMaskfunctionThe
hideMaskfunction is properly memoized usinguseCallback, which helps prevent unnecessary re-renders by maintaining a stable function reference.cvat-ui/src/containers/annotation-page/standard-workspace/objects-side-bar/objects-list.tsx (7)
22-22: ****
29-29: ****
60-60: ****
72-72: ****
186-186: ****
240-240: ****
271-273: ****cvat-canvas/src/typescript/masksHandler.ts (9)
9-9: ImportPositiontype for consistencyAdding
Positionto the import statement ensures type consistency across the codebase.
64-64: UsePositiontype forlatestMousePosChanging the type of
latestMousePostoPositionenhances type safety and clarity.
68-68: AddisHiddenproperty to manage visibility stateIntroducing the
isHiddenproperty allows the handler to manage the visibility of edited objects effectively.
221-224: Correct parameters ingetImageDatacallThe parameters in the
getImageDatamethod correctly define the area from which to extract image data.
371-371: InitializeisHiddentofalsein constructorSetting
isHiddentofalseensures the canvas is visible by default upon initialization.
474-474: Prevent drawing when canvas is hiddenAdding
!this.isHiddento the condition correctly prevents drawing operations when the canvas is hidden.
589-592: Consistent parameter usage inimageDataToDataURLMatching parameter adjustments in
imageDataToDataURLcalls ensures consistency in image processing functions.
683-686: Update parameters forimageDataToDataURLin edit flowAdjusting the parameters when editing masks maintains consistency and correctness in the mask editing process.
552-555: SynchronizeisHiddenstate with configurationUpdating the
isHiddenstate based onconfiguration.hideEditedObjectensures that the visibility setting is applied correctly.Ensure that the
hideEditedObjectproperty exists in theConfigurationinterface and is properly typed.Run the following script to verify its presence:
If the property is not found, please update the
Configurationinterface accordingly.✅ Verification successful
hideEditedObjectproperty is correctly defined in the Configuration interfaceThe
hideEditedObjectproperty exists in theConfigurationinterface and is properly typed as an optional boolean.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that 'hideEditedObject' is defined in the 'Configuration' interface. # Test: Search for the 'hideEditedObject' property definition in 'canvasModel.ts'. # Expect: At least one match indicating the property is defined. ast-grep --lang typescript --pattern 'interface Configuration { $$$; hideEditedObject?: boolean; $$$ }' ./src/typescript/canvasModel.tsLength of output: 216
Script:
#!/bin/bash # Description: Locate the 'Configuration' interface definition in the codebase. # Search for the 'Configuration' interface definition across all TypeScript files. # Expect: At least one match indicating where the interface is defined. ast-grep --lang typescript --pattern 'interface Configuration { $$$ }' .Length of output: 1714
cvat-ui/src/components/annotation-page/canvas/views/canvas2d/canvas-wrapper.tsx (8)
48-49: Importing new action creatorsThe action creators
updateEditedStateAsyncandresetEditedStateAsyncare correctly imported, enabling dispatching these actions within the component.
151-152: Adding methods to DispatchToProps interfaceThe methods
onUpdateEditedObjectandonResetEditedObjectare appropriately added to theDispatchToPropsinterface, ensuring they are available as props.
354-359: Defining dispatch methods for edited object actionsThe dispatch methods
onUpdateEditedObjectandonResetEditedObjectare properly defined inmapDispatchToProps, allowing the component to dispatch edited object state updates.
683-683: Resetting edited object state after shape is drawnCalling
onResetEditedObject()after a shape is drawn ensures that the edited object state is cleared, preventing unintended behavior.
911-913: Resetting edited object state upon canvas cancelIncluding
onResetEditedObject()in theonCanvasCancelmethod ensures the edited object state is reset when canvas interactions are canceled, maintaining the correct application state.
844-847: Updating edited object state on canvas draw startThe
onCanvasDrawStartmethod correctly updates the edited object state by callingonUpdateEditedObjectwith the shape type, which is appropriate.
849-852: Updating active control and edited object state on edit startIn the
onCanvasEditStartmethod, updating the active control toActiveControl.EDITand callingonUpdateEditedObjectwith the shape type and state ensures proper state management during editing.
Line range hint
856-871: Resetting edited object state after editing is doneCalling
onResetEditedObject()at the end ofonCanvasEditDoneensures that the edited object state is properly reset after editing is completed.cvat-canvas/src/typescript/svg.patch.ts (1)
Line range hint
97-99: Verify loop boundary condition indrawCirclesThe loop uses
i < array.length - 1, which may skip processing the last element ofarray. Please verify if this is intentional. If all elements should be processed, consider changing the condition toi < array.length.cvat-ui/src/actions/annotation-actions.ts (3)
129-131: Action types added to AnnotationActionTypesThe new action types
UPDATE_EDITED_STATE,HIDE_EDITED_STATE, andRESET_EDITED_STATEare appropriately added to theAnnotationActionTypesenum.
1615-1644: Implementation of updateEditedStateAsync is appropriateThe function
updateEditedStateAsynccorrectly updates the canvas configuration and dispatches the necessary actions to manage the edited state.
1670-1693: Function changeHideEditedStateAsync correctly updates hide stateThe function
changeHideEditedStateAsyncproperly updates the canvas configuration, dispatches theHIDE_EDITED_STATEaction, and updates thehiddenproperty ofeditedStateInstance.cvat-canvas/src/typescript/drawHandler.ts (3)
8-8: Import of 'CIRCLE_STROKE' looks correct.The import statement for 'CIRCLE_STROKE' from './svg.patch' is appropriate and ensures the constant is available for use in the class.
107-107: Initialization of 'isHidden' property is appropriate.The declaration and initialization of the
isHiddenproperty correctly manage the visibility state of the drawn shapes.Also applies to: 1283-1283
1345-1350: 'updateHidden' method logic is sound.The
updateHiddenmethod correctly updates the hidden state and adjusts the draw instance's opacity accordingly.
cvat-ui/src/containers/annotation-page/standard-workspace/objects-side-bar/object-buttons.tsx
Show resolved
Hide resolved
cvat-ui/src/containers/annotation-page/standard-workspace/objects-side-bar/object-buttons.tsx
Show resolved
Hide resolved
cvat-ui/src/containers/annotation-page/standard-workspace/objects-side-bar/objects-list.tsx
Show resolved
Hide resolved
cvat-ui/src/containers/annotation-page/standard-workspace/objects-side-bar/objects-list.tsx
Show resolved
Hide resolved
cvat-ui/src/components/annotation-page/canvas/views/canvas2d/canvas-wrapper.tsx
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8554 +/- ##
===========================================
+ Coverage 74.24% 74.25% +0.01%
===========================================
Files 403 403
Lines 43287 43350 +63
Branches 3914 3935 +21
===========================================
+ Hits 32137 32191 +54
- Misses 11150 11159 +9
|
|
There was a problem hiding this comment.
I think we should remove shapeType and editedStateHidden from editing in global store, because the implementation only was requested for masks and it works only for masks.
You already have shapeType in drawing state (when we speak about initial creating of the mask), do not need to duplicate it. You may use existing value.
Also, small bug was found:
- In brush tools switch to polygon drawing
- Draw part of polygon
- Press hide, press show
- Drawn part of polygon lost
| @@ -1045,6 +1046,7 @@ export default class Collection { | |||
| z_order: 0, | |||
| outside: element.outside || false, | |||
| occluded: element.occluded || false, | |||
| hidden: state.hidden || false, | |||
| @@ -1026,6 +1026,7 @@ export default class Collection { | |||
| label_id: state.label.id, | |||
| outside: state.outside || false, | |||
| occluded: state.occluded || false, | |||
| hidden: state.hidden || false, | |||
There was a problem hiding this comment.
I am not sure that creating a new object already hidden makes sense as it confuses the user
They press N and expect a new object to be appear, I think.
If the object is hidden, they may think that something is going wrong and start drawing it again (especially when there are a lot of objects in the sidebar, newly added is not visible in the list)
| dispatch({ | ||
| type: AnnotationActionTypes.RESET_EDITED_STATE, | ||
| payload: {}, | ||
| }); |
There was a problem hiding this comment.
dispatch(resetEditedState())?
There was a problem hiding this comment.
Not really relevant considering my other comment with proposal to combine actions
| const hideMask = useCallback((hide: boolean) => { | ||
| dispatch(changeHideEditedStateAsync(hide)); | ||
| }, [dispatch]); |
There was a problem hiding this comment.
dispatch is a const, so, I am not sure we need this wrapper
| @@ -75,6 +75,7 @@ function BrushTools(): React.ReactPortal | null { | |||
| const config = useSelector((state: CombinedState) => state.annotation.canvas.brushTools); | |||
| const canvasInstance = useSelector((state: CombinedState) => state.annotation.canvas.instance); | |||
| const labels = useSelector((state: CombinedState) => state.annotation.job.labels); | |||
| const editedState = useSelector((state: CombinedState) => state.annotation.editing); | |||
There was a problem hiding this comment.
Please, combine useSelector's together with shallowEqual
| if (editedStateInstance) { | ||
| const state = getState(); | ||
| const { instance: canvas } = state.annotation.canvas; | ||
| if (canvas) { | ||
| (canvas as Canvas).configure({ | ||
| hideEditedObject: editedStateInstance.hidden, | ||
| }); | ||
| } | ||
|
|
||
| dispatch({ | ||
| type: AnnotationActionTypes.HIDE_EDITED_STATE, | ||
| payload: { | ||
| hide: editedStateInstance.hidden, | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
I think we have three actions changeHideEditedStateAsync, updateEditedStateAsync, resetEditedStateAsync doing the same thing (updating the same key in store).
Probably we may combine them to reduce the code duplication. Let it accept shapeType, objectState and isHidden. And use these arguments to update store and objectState properly
There was a problem hiding this comment.
Considering my general review comment it should only accept objectState: ObjectState | null
| @@ -649,6 +664,7 @@ class CanvasWrapperComponent extends React.PureComponent<Props> { | |||
| state.rotation = state.rotation || 0; | |||
| state.occluded = state.occluded || false; | |||
| state.outside = state.outside || false; | |||
| state.hidden = state.hidden || (editedStateHidden && workspace !== Workspace.SINGLE_SHAPE); | |||
| @@ -185,6 +188,12 @@ const componentShortcuts = { | |||
| sequences: ['del', 'shift+del'], | |||
| scope: ShortcutScope.SINGLE_SHAPE_ANNOTATION_WORKSPACE, | |||
| }, | |||
| HIDE_MASK_SINGLE_SHAPE: { | |||
| name: 'Hide mask', | |||
| description: 'Hide edited mask', | |||
There was a problem hiding this comment.
| description: 'Hide edited mask', | |
| description: 'Hide currently edited mask', |
| @@ -44,6 +46,7 @@ function mapStateToProps(state: CombinedState, own: OwnProps): StateToProps { | |||
| player: { | |||
| frame: { number: frameNumber }, | |||
| }, | |||
| editing: editedState, | |||
There was a problem hiding this comment.
It is better to pass exact boolean property of the object you only use in this component. Potentially it will cause less renderings
| @@ -29,11 +29,13 @@ interface StateToProps { | |||
| outsideDisabled: boolean; | |||
| hiddenDisabled: boolean; | |||
| keyframeDisabled: boolean; | |||
| editedState: EditingState, | |||
There was a problem hiding this comment.
As it comes from global store and we anyway use mapStateToProps here, I do not see the reason to pass as own props
Motivation and context
Added ability to hide a mask during editing
How has this been tested?
Checklist
developbranch[ ] I have updated the documentation accordingly[ ] I have added tests to cover my changes[ ] I have linked related issues (see GitHub docs)(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores