Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove empty masks #7295

Merged
merged 47 commits into from
Jan 18, 2024
Merged

Remove empty masks #7295

merged 47 commits into from
Jan 18, 2024

Conversation

klakhov
Copy link
Contributor

@klakhov klakhov commented Dec 26, 2023

Motivation and context

This PR improves CVAT behavior in case of appearing empty masks.

  1. In case if some masks become empty as a result of remove underlying pixels feature, empty masks are now deleted and special notification is shown:
    image
  2. Added support for undo/redo for remove underlying pixels
  3. When user starts creating new mask (or editing existing one which was fully erased) eraser and poly-minus tools are blocked
    image
  4. If user fully erases mask and tries to apply such changes the action is cancelled. (If user wants to delete mask completely he should use Del/Remove actions)
  5. Added empty mask validation checks in cvat-core

Fixed bug with copying masks which lead to changing label.

Resolved #7160

How has this been tested?

Checklist

  • I submit my changes into the develop branch
  • I have created a changelog fragment
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • I have linked related issues (see GitHub docs)
  • I have increased versions of npm packages if it is necessary
    (cvat-canvas,
    cvat-core,
    cvat-data and
    cvat-ui)

License

  • I submit my code changes under the same MIT License that covers the project.
    Feel free to contact the maintainers if that's a concern.

cvat-core/src/annotations-collection.ts Outdated Show resolved Hide resolved
cvat-core/src/annotations-collection.ts Outdated Show resolved Hide resolved
cvat-ui/src/actions/annotation-actions.ts Outdated Show resolved Hide resolved
cvat-core/src/annotations-objects.ts Outdated Show resolved Hide resolved
cvat-core/src/annotations-objects.ts Outdated Show resolved Hide resolved
@klakhov klakhov marked this pull request as ready for review December 27, 2023 13:00
Copy link

codecov bot commented Dec 27, 2023

Codecov Report

Merging #7295 (7ccc1f5) into develop (f688908) will decrease coverage by 0.07%.
The diff coverage is 50.96%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #7295      +/-   ##
===========================================
- Coverage    83.24%   83.18%   -0.07%     
===========================================
  Files          373      373              
  Lines        39615    39693      +78     
  Branches      3701     3713      +12     
===========================================
+ Hits         32977    33018      +41     
- Misses        6638     6675      +37     
Components Coverage Δ
cvat-ui 78.90% <50.96%> (-0.11%) ⬇️
cvat-server 87.12% <ø> (ø)

@bsekachev
Copy link
Member

bsekachev commented Dec 28, 2023

Found inconsistencies:

Still can create empty mask in cvat-canvas module. It can be passed to cvat-core where exception is thrown and message is shown.
In case with other shapes, it is not created, message does not appear

Create a normal mask, modify it to empty => exception is thrown, message is shown
In case of other shapes, it cancels changes, message does not appear

Found problems:
Message: "As a result of removing the underlying pixels, some masks became empty and were subsequently deleted." disappears to fast. I was not even able to read it until the end for the first time.
Message: "As a result of removing the underlying pixels, some masks became empty and were subsequently deleted." appears everytime when I create a mask with enabled "removeUnderlyingPixels"

Let's try to modify the message, splitting it to title and description. Something like this?
image

@klakhov
Copy link
Contributor Author

klakhov commented Dec 29, 2023

I belive everything is consistent.

Found inconsistencies:

Still can create empty mask in cvat-canvas module. It can be passed to cvat-core where exception is thrown and message is shown. In case with other shapes, it is not created, message does not appear
Why there is no exception with other shapes?

We have the same logic for all shapes in checkNumOfPoints function. If I try to create rect with <4 points then an errror occurs

            const state = new cvat.classes.ObjectState({
                shapeType: ShapeType.RECTANGLE,
                objectType: ObjectType.SHAPE,
                points: [0],
                occluded: false,
                attributes: {},
                frame: 1,
                label: jobInstance.labels[0],
            });
            const [clientID] = await jobInstance.annotations.put([state]);

image

Create a normal mask, modify it to empty => exception is thrown, message is shown In case of other shapes, it cancels changes, message does not appear

If I try to set rectangle to <4 points and save it then the same exeption is thrown. Is there even a way to create such case for other shapes than masks?

Found problems: Message: "As a result of removing the underlying pixels, some masks became empty and were subsequently deleted." disappears to fast. I was not even able to read it until the end for the first time. Message: "As a result of removing the underlying pixels, some masks became empty and were subsequently deleted." appears everytime when I create a mask with enabled "removeUnderlyingPixels"

Let's try to modify the message, splitting it to title and description. Something like this? image

Fixed the message.

@bsekachev
Copy link
Member

How can you create/edit a rectangle setting it less then 4 points?

@bsekachev
Copy link
Member

bsekachev commented Dec 29, 2023

We have the same logic for all shapes in checkNumOfPoints function. If I try to create rect with <4 points then an errror occurs

Yes, we have such check in cvat-core

But cvat-canvas, cvat-ui will not even try to save a rectangle with less number of points for a rectangle.
The same for polyshapes. For example when polygon has only three points, the interface doesn't allow to remove any more points:
image
image

The idea is to prevent useless exception when user does something wrong.
With masks we can see these exceptions in regular work now, with other shapes, we can't

@bsekachev
Copy link
Member

I believe locking polygon-minus and eraser tool if mask is empty in cvat-canvas would be a good solution in first.
This way let users avoid misclicking a button. Probably it is one of ways why such masks are created, not intentionally
One more way, is when we finished drawing a mask using eraser/polygon minus, creating a new shape will have this tool as default one. What is also not really user friendly.

Finally if a user created an empty mask in cvat-canvas, it should not raise shape.drawn event

@klakhov
Copy link
Contributor Author

klakhov commented Dec 29, 2023

How can you create/edit a rectangle setting it less then 4 points?

Using cvat-core api via code.

I believe locking polygon-minus and eraser tool if mask is empty in cvat-canvas would be a good solution in first.
Finally if a user created an empty mask in cvat-canvas, it should not raise shape.drawn event

Okay, ill think about that

@bsekachev bsekachev changed the title Remove empty masks [WIP] Remove empty masks Jan 2, 2024
@bsekachev
Copy link
Member

There is an issue with inserting. Class has changed
Peek 2024-01-15 13-07

cvat-core/src/annotations-collection.ts Outdated Show resolved Hide resolved
cvat-core/src/annotations-collection.ts Outdated Show resolved Hide resolved
cvat-core/src/annotations-objects.ts Outdated Show resolved Hide resolved
cvat-core/src/annotations-objects.ts Outdated Show resolved Hide resolved
const redo = (): void => {
stashedPoints = [];
for (const object of Object.values(updatedObjects)) {
const points = mask2Rle(masks[object.clientID]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There will be a problem with huge memory consumption

In closure it refers to variable masks that contains all unpacked masks stored on the frame (imagine that there are 100+ masks per frame).
And undo/redo stores up to 128 actions as far as I remember

It gigabytes of data

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

masks store not all masks on the frame but only affected ones, but still there can be a problem.
Ive added limitations to annotation history, so there only can be 20 undos for removing underlying pixels. (maybe we should change this number to even lower value)

@klakhov
Copy link
Contributor Author

klakhov commented Jan 17, 2024

@bsekachev

The issue with copying mask that was changing class seems to be reproducible on develop, its separate bug but Ive fixed it.

I cant really reproduce the issue with color changing on last verion of PR, so maybe its fixed already.

@bsekachev bsekachev merged commit 209826c into develop Jan 18, 2024
34 checks passed
@klakhov klakhov deleted the kl/remove-empty-masks branch January 19, 2024 10:01
@klakhov klakhov mentioned this pull request Jan 23, 2024
4 tasks
@cvat-bot cvat-bot bot mentioned this pull request Jan 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Creating an empty mask when using "Remove polygon selection"
2 participants