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

Fixed copy/paste for rotated shapes #4061

Merged
merged 9 commits into from
Dec 24, 2021
Merged

Conversation

bsekachev
Copy link
Member

@bsekachev bsekachev commented Dec 22, 2021

Motivation and context

How has this been tested?

Resolve #4038

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 bug Something isn't working label Dec 22, 2021
@bsekachev bsekachev changed the title [WIP] Fixed copy/paste for rotated shapes Fixed copy/paste for rotated shapes Dec 22, 2021
@bsekachev
Copy link
Member Author

@dvkruchinin

Could you please add a test with copy/paste a rotated shape?

@dvkruchinin
Copy link
Contributor

@dvkruchinin

Could you please add a test with copy/paste a rotated shape?

Sure. I`ll update the corresponding test.

Marishka17
Marishka17 previously approved these changes Dec 23, 2021
Copy link
Contributor

@Marishka17 Marishka17 left a comment

Choose a reason for hiding this comment

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

LGTM, but there is one inconsistency.
inconsistency

@bsekachev
Copy link
Member Author

@Marishka17 That is not inconsistency. Just text cannot be placed right of the first shape because in this case it is out of canvas.

Comment on lines -70 to -73
function testActivatingShape(x, y, expectedShape) {
cy.get('.cvat-canvas-container').trigger('mousemove', x, y);
cy.get(expectedShape).should('have.class', 'cvat_canvas_shape_activated');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you remove this check?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because it is not necessary anymore. Previous test implementation activated shapes to get coordinates of circles.
These coordinates needed to check final result. The test worked this way, but it was a kind of lucky. Circles for different shapes are located in different coordinate spaces and comparing them is incorrect way. Two points in different places on canvas can have the same coordinates (0,0 for example). I've rewritten the test and now it gets correct position from shape attributes.

@bsekachev bsekachev merged commit 1cd2ea0 into develop Dec 24, 2021
@bsekachev bsekachev deleted the bs/fixed_copy_rotated branch December 24, 2021 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bounding box is copied without rotation
3 participants