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

Fix move and rotation map tools preview when layer crs != canvas crs #51445

Merged
merged 2 commits into from
Feb 21, 2023

Conversation

YoannQDQ
Copy link
Contributor

Description

Fixes the Rotate Feature and Move Feature maptools previews, when the layer crs is not the same as the map canvas destination crs.

Current behavior

broken_rubber

Fixed behavior

fix_rubber

@github-actions github-actions bot added this to the 3.30.0 milestone Jan 12, 2023
@roya0045
Copy link
Contributor

Isn't it expensive to do a feature request and iteration on each update? If it's negligible or there's no way around, fine. Otherwise I would assume that creating a single entity and just moving it around would be more efficient, instead of refetching the data and transforming it?

@YoannQDQ
Copy link
Contributor Author

You're probably right. I had no visible performance issue with my data, but they were extrememely simple polygons. I'll update the PR when I have the time.

@YoannQDQ YoannQDQ closed this Jan 12, 2023
@YoannQDQ YoannQDQ reopened this Jan 12, 2023
@YoannQDQ YoannQDQ closed this Jan 13, 2023
@YoannQDQ YoannQDQ reopened this Jan 13, 2023
@YoannQDQ YoannQDQ closed this Jan 13, 2023
@YoannQDQ YoannQDQ reopened this Jan 13, 2023
Comment on lines -56 to -57
mRubberBand->updatePosition();
mRubberBand->update();
Copy link
Member

Choose a reason for hiding this comment

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

why are these 2 lines dropped?

Copy link
Contributor Author

@YoannQDQ YoannQDQ Jan 30, 2023

Choose a reason for hiding this comment

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

If I'm not mistaken, setTranslationOffset already updates the rubberband, making the call to updatePosition and update redundant.

@YoannQDQ YoannQDQ requested a review from 3nids February 8, 2023 12:48
@YoannQDQ
Copy link
Contributor Author

@3nids Should it be the other way around though? When drawing circles for instance (#51931), what matters is the shape be a circle in the project CRS, not in the layer CRS. So we would have to keep the rubberband as it was, and reproject the rubber geometry to the layer crs on release.

@3nids 3nids merged commit 6d8aa13 into qgis:master Feb 21, 2023
@YoannQDQ YoannQDQ deleted the fix-move-rotate-tool-preview branch February 21, 2023 13:50
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.

None yet

3 participants