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

Improved interface of interactors on UI #2054

Merged
merged 26 commits into from
Aug 31, 2020
Merged

Conversation

bsekachev
Copy link
Member

@bsekachev bsekachev commented Aug 19, 2020

Motivation and context

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) 2020 Intel Corporation
#
# SPDX-License-Identifier: MIT

Checklist:

  • UI notifications & lock when the process is active (UI feedback)
  • Freeze history when updated points
  • Add description as a tooltip
  • Change icon for AI tools
  • Remove old DEXTR code
  • CTRL modifier key to maintain requests
  • Update server interactors API (need some additional parameters and negative points)

@bsekachev bsekachev added the enhancement New feature or request label Aug 19, 2020
@bsekachev bsekachev added this to the 1.1.0-release milestone Aug 19, 2020
@bsekachev bsekachev changed the title [WIP] Improved interface of UI Interactors [WIP] Improved interface of interactors on UI Aug 19, 2020
@coveralls
Copy link

coveralls commented Aug 19, 2020

Pull Request Test Coverage Report for Build 7214

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 140 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-0.07%) to 69.681%

Files with Coverage Reduction New Missed Lines %
cvat/apps/engine/media_extractors.py 5 76.11%
src/annotations.js 5 48.63%
src/annotations-history.js 14 30.77%
src/session.js 116 53.97%
Totals Coverage Status
Change from base Build 7202: -0.07%
Covered Lines: 11753
Relevant Lines: 16412

💛 - Coveralls

cvat-canvas/README.md Outdated Show resolved Hide resolved
cvat-ui/src/utils/typescript.ts Outdated Show resolved Hide resolved
cvat-ui/src/utils/interactors/openvino.dextr.ts Outdated Show resolved Hide resolved
cvat-ui/src/utils/dextr-utils.ts Outdated Show resolved Hide resolved
cvat-ui/src/reducers/models-reducer.ts Outdated Show resolved Hide resolved
@bsekachev bsekachev changed the title [WIP] Improved interface of interactors on UI Improved interface of interactors on UI Aug 20, 2020
@bsekachev
Copy link
Member Author

@nmanovic, @ActiveChooN

The PR is ready to review

Copy link
Contributor

@ActiveChooN ActiveChooN left a comment

Choose a reason for hiding this comment

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

Have a strange issue with canvasModel.ts

изображение

Have an issue with a label selection.

ait_failed_label_selection

Have an issue with interaction.

ait_failed_interaction

cvat-canvas/src/typescript/canvasModel.ts Show resolved Hide resolved
cvat-ui/src/assets/ai-tools.svg Outdated Show resolved Hide resolved
cvat-ui/src/icons.tsx Outdated Show resolved Hide resolved
cvat-canvas/src/typescript/canvasModel.ts Outdated Show resolved Hide resolved
cvat-canvas/src/typescript/crosshair.ts Show resolved Hide resolved
@bsekachev
Copy link
Member Author

Have a strange issue with canvasModel.ts

It had the same behaviour with export Configuration.
Typescript/Babel/Wepback issue.
Nothing critical: babel/babel#8639 . Can try workaround to fix it.

Have an issue with a label selection.

Fixed

Have an issue with interaction.

Hmm. Cannot reproduce. I will try more, but try to restart server, may be it will help.

@bsekachev
Copy link
Member Author

bsekachev commented Aug 21, 2020

Have an issue with interaction.

I suppose it was related with label selector (wrong onChange function as I later found). Should be fixed for now.

Comment on lines 159 to 163
const result = await core.lambda.call(jobInstance.task, interactor, {
task: jobInstance.task,
frame,
points: convertShapesForInteractor((e as CustomEvent).detail.shapes),
});
Copy link
Contributor

Choose a reason for hiding this comment

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

As i understand, negative points in this patch works like positive, isn't it? Maybe it's better add a filter for its or something else, what do you think?

Copy link
Member Author

@bsekachev bsekachev Aug 24, 2020

Choose a reason for hiding this comment

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

Right now, yes.
Need to update server API for interactors (an interactor on the server should itself decide what to do with negative points)
We can filter these points for DEXTR only on UI (till the next PR), but what problem we are trying to solve?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a little bit confusing that negative points works like positive. Maybe it's better to temporary disable ability to create negative points in UI, since we have no interactors to work with and in the next PR with extended interactors functionality enable it.

Copy link
Member Author

Choose a reason for hiding this comment

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

And in the next PR when we enable them, they will be confusing again.. Or maybe user will input some negative points in the future and they are ignored, what also can be confusing for him, or a user inputs four points (two are accidentaly negative), they are filtered and dextr gets two points instead of 4. Here are a lot of open questions. If it makes you worried, we can discuss it tomorrow on the meeting

ActiveChooN
ActiveChooN previously approved these changes Aug 26, 2020
@nmanovic
Copy link
Contributor

nmanovic commented Aug 26, 2020

@bsekachev , it is a great feature! It works like a charm. Please give me a day to experiment with it. Thanks!

@nmanovic
Copy link
Contributor

@bsekachev ,

When I press ESC, the drawn polygon isn't removed. Is it expected?

@nmanovic
Copy link
Contributor

@bsekachev , should we introduce semi-automatic hint in addition to MANUAL and AUTOMATIC? What is your opinion?

@nmanovic
Copy link
Contributor

@bsekachev , should we allow to move to another frame during interaction? For now CVAT doesn't and it is good from my point of view. The reason of the question is because for another drawing methods we allow. Should it be unified?

@bsekachev
Copy link
Member Author

When I press ESC, the drawn polygon isn't removed. Is it expected?

No, It is expected that drawing will canceled.
I will fix it

@bsekachev
Copy link
Member Author

@bsekachev , should we introduce semi-automatic hint in addition to MANUAL and AUTOMATIC? What is your opinion?

If we get a request for that, we can. But right now I would suggest left it as is.

@bsekachev
Copy link
Member Author

should we allow to move to another frame during interaction? For now CVAT doesn't and it is good from my point of view. The reason of the question is because for another drawing methods we allow. Should it be unified?

Historically we didn't strict moving between frames during drawing
Because of obvious reasons moving is locked during redrawing an object or interaction
Restricting it in regular drawing might bring inconvenience for users, but I am not sure. We can try and reject it in case of negative feedback

@bsekachev bsekachev mentioned this pull request Aug 31, 2020
11 tasks
nmanovic
nmanovic previously approved these changes Aug 31, 2020
Copy link
Contributor

@nmanovic nmanovic left a comment

Choose a reason for hiding this comment

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

LGTM

nmanovic
nmanovic previously approved these changes Aug 31, 2020
@nmanovic nmanovic merged commit 908e056 into develop Aug 31, 2020
@nmanovic nmanovic deleted the bs/canvas_interaction branch August 31, 2020 19:20
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.

4 participants