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

Semi-automatic tools enhancements (Client-side points minimizer) #3450

Merged
merged 18 commits into from
Jul 28, 2021

Conversation

bsekachev
Copy link
Member

@bsekachev bsekachev commented Jul 22, 2021

Motivation and context

Related #2936
Related #2515

New setting:
Screenshot from 2021-07-26 12-18-49

On the fly working:
Peek 2021-07-26 12-20

How has this been tested?

Manual testing

Checklist

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.
  • I have updated the license header for each file (see an example below)
# Copyright (C) 2021 Intel Corporation
#
# SPDX-License-Identifier: MIT

@bsekachev bsekachev added the enhancement New feature or request label Jul 22, 2021
@bsekachev bsekachev changed the title [WIP] Semi-automatic tools enhancements (Client-side points minimizer) Semi-automatic tools enhancements (Client-side points minimizer) Jul 22, 2021
@bsekachev bsekachev requested a review from klakhov July 22, 2021 09:26
@bsekachev
Copy link
Member Author

@TOsmanov @aschernov

This is a feature you requested, please provide your feedback.
Also, need to update documentation when the PR is merged.

@dvkruchinin
Could you prepare a test for the issue? At least we added a new setting (default accuracy) and need to check this setting can be saved and restored. It also should work for OpenCV intelligent scissors (I am not sure if we have a written test for the feature)

@bsekachev bsekachev changed the title Semi-automatic tools enhancements (Client-side points minimizer) [WIP] Semi-automatic tools enhancements (Client-side points minimizer) Jul 22, 2021
@aschernov
Copy link
Contributor

@bsekachev thank you, we'll take a look.

@dvkruchinin
Copy link
Contributor

@dvkruchinin
Could you prepare a test for the issue? At least we added a new setting (default accuracy) and need to check this setting can be saved and restored. It also should work for OpenCV intelligent scissors (I am not sure if we have a written test for the feature)

Sure. I'll prepare a test. Unfortunately, it will not be possible to do this for OpenCV yet, since the problem of losing the Content-Length header is not solved if OpenCV is loaded from the browser under the control of Cypress.

@bsekachev bsekachev changed the title [WIP] Semi-automatic tools enhancements (Client-side points minimizer) Semi-automatic tools enhancements (Client-side points minimizer) Jul 22, 2021
@bsekachev
Copy link
Member Author

@nmanovic Here you can see errors in tests we discussed last meeting

ActiveChooN
ActiveChooN previously approved these changes Jul 23, 2021
@nmanovic
Copy link
Contributor

nmanovic commented Jul 23, 2021

@dvkruchinin , could you please look at problems with tests in the PR?

@nmanovic
Copy link
Contributor

@bsekachev ,

  1. For an image I got less than 3 points. Probably need to restrict that.

image

  1. From my point of view the "approximation accuracy" should be visible only after we have a polygon. Now it is visible as soon as we activate an interactor.

  2. Personally I will rename "approximation accuracy" into "Number of points" with 2 visible values: low and high.

  3. Also for small epsilon I would prefer to have more values: 0.5, 1, 1.25 1.5, 2, 3, 4. In some cases the difference between number of points is too big.

@nmanovic
Copy link
Contributor

@bsekachev , it will be great to apply the approxPolyDP function for existing polygons somehow in the future. For example, how to reduce the number of points for Mask RCNN?

@nmanovic
Copy link
Contributor

@bsekachev , looks like a natural idea to have "done" button directly on the slider. It is more visible. But I'm not sure.

@dvkruchinin
Copy link
Contributor

@dvkruchinin , could you please look at problems with tests in the PR?

I can't reproduce the problem locally or in a fork. This could probably be related to internal problems of the github actions virtual environment.
I will soon prepare the necessary test and look at the passing of tests in the current repository.

@bsekachev
Copy link
Member Author

For an image I got less than 3 points. Probably need to restrict that.

Looks like OpenCV can return it after approximation (event despite of "closed" flag equals to true).

I am not sure we should strict it because in this case we can only return original points, but it is not clear for a user (users shift slider to less points, but unexpectedly they get all the points).
But I would suggest to mark the shape as RED on the canvas. It's a kind of a signal that something is wrong with the shape. Even if a user submits such the shape, they get an exception from cvat-core (not enough points).

Personally I will rename "approximation accuracy" into "Number of points" with 2 visible values: low and high.
Also for small epsilon I would prefer to have more values: 0.5, 1, 1.25 1.5, 2, 3, 4. In some cases the difference between number of points is too big.

Done

@bsekachev
Copy link
Member Author

@bsekachev , looks like a natural idea to have "done" button directly on the slider. It is more visible. But I'm not sure.
@bsekachev , it will be great to apply the approxPolyDP function for existing polygons somehow in the future. For example, how to reduce the number of points for Mask RCNN?

These ideas are bound each other, because in current implementation "Done" button exists when drawing all polyshapes, but the slider does not.

Right now I would suggest leaving as is beacuse I don't have ideas how it should be implemented in UI for all the cases (exactly when it should appear and disappear, how it should work with drawing (when we need Done button, but a user is not assumed to create extra points to be minimized), or with editing (when we could need minimize points after detectors, but done button is not supposed to be used), etc.

@bsekachev
Copy link
Member Author

From my point of view the "approximation accuracy" should be visible only after we have a polygon. Now it is visible as soon as we activate an interactor.

@nmanovic Done

@nmanovic
Copy link
Contributor

nmanovic commented Jul 27, 2021

Every interaction with DEXTR leads to the problem below. Probably need to check the threshold value.

Uncaught (in promise) RangeError: offset is out of bounds
    at Float32Array.set (<anonymous>)
    at Object.eval [as matFromArray] (eval at initialize (cvat-ui.b532816919397967c04d.min.js:435), <anonymous>:32:9059162)
    at Object.approxPoly (cvat-ui.b532816919397967c04d.min.js:435)
    at sH.approximateResponsePoints (cvat-ui.b532816919397967c04d.min.js:435)
    at sH.componentDidUpdate (cvat-ui.b532816919397967c04d.min.js:435)
    at os (cvat-ui.b532816919397967c04d.min.js:352)
    at pl (cvat-ui.b532816919397967c04d.min.js:352)
    at t.unstable_runWithPriority (cvat-ui.b532816919397967c04d.min.js:360)
    at Ba (cvat-ui.b532816919397967c04d.min.js:352)
    at fl (cvat-ui.b532816919397967c04d.min.js:352)

if (approxPolyMaxDistance > 0) {
if (approxPolyMaxDistance <= 8) {
// −2.75x+7y+1=0 linear made from two points (1; 0.25) and (8; 3)
threshold = (2.75 * approxPolyMaxDistance - 1) / 7;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the threshold be negative?

Copy link
Contributor

Choose a reason for hiding this comment

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

If approxPolyAccuracy == 13, it will be -1/7

Copy link
Member Author

Choose a reason for hiding this comment

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

No, approxPolyMaxDistance is an integer >= 1 and <= 8 in this piece of code.

Copy link
Member Author

Choose a reason for hiding this comment

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

if approxPolyAccuracy == 13, then approxPolyMaxDistance == 0 and the condition in line 35 is false

@bsekachev
Copy link
Member Author

@nmanovic

Every interaction with DEXTR leads to the problem below. Probably need to check the threshold value.

Could you please provide more details (image, points, threshold), because I can't reproduce it.

@nmanovic
Copy link
Contributor

Use the video file: https://github.com/opencv/opencv/blob/master/samples/data/vtest.avi?raw=true, try to annotate people using DEXTR.

@bsekachev
Copy link
Member Author

@nmanovic

Works as expected
ezgif-2-278bb7241d5d

@bsekachev
Copy link
Member Author

@nmanovic

Maybe just give me points that DEXTR returns to client and be sure you pull the latest changes (I committed all the changes)

@nmanovic
Copy link
Contributor

@bsekachev , I will merge. But I cannot pull dots from extreme points (less and more).

@nmanovic nmanovic merged commit 0ea4897 into develop Jul 28, 2021
@nmanovic nmanovic deleted the bs/semi_enhancements branch July 28, 2021 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants