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

Implement click-move-click & snapping behavior for the georeferencer's move GCP point tool #60563

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

nirvn
Copy link
Contributor

@nirvn nirvn commented Feb 12, 2025

Description

This PR upgrades the georeferencer's move GCP point tool to rely on a click-move-click behavior. On top of harmonizing the behavior with other QGIS tools (such as the vertex editor tool, annotation node manipulation tool, etc.), it allows us to implement snapping when moving the GCP points.

Having moved to the click-move-click behavior also made it possible to implement a right-click cancel functionality when users realize they are moving the wrong GCP point.

Finally, GCP points now react to being hovered, with a larger symbol being drawn when that's the case. It makes for a much nicer experience trying to pick the right point to move.

@github-actions github-actions bot added this to the 3.42.0 milestone Feb 12, 2025
Copy link

github-actions bot commented Feb 12, 2025

🪟 Windows builds

Download Windows builds of this PR for testing.
Debug symbols for this build are available here.
(Built from commit d6cf21b)

🪟 Windows Qt6 builds

Download Windows Qt6 builds of this PR for testing.
(Built from commit d6cf21b)

@nirvn nirvn force-pushed the georeferencer_move_snap branch from 4df6f8e to 0f4a19a Compare February 13, 2025 04:37
@nirvn
Copy link
Contributor Author

nirvn commented Feb 13, 2025

@uclaros , was this PR ok to merge otherwise? :)

@uclaros
Copy link
Contributor

uclaros commented Feb 14, 2025

@uclaros , was this PR ok to merge otherwise? :)

I now gave the code a proper look and looks good. There's one UX thing:
The ellipse growing on hover is super nice, however by changing the ellipse size from an odd number to even (from 5 to 6 when not hovering), the ellipse will not appear centered to the cursor crosshair any more, not to a single pixel in the canvas.
Also, when moving a point, it is great that the GCP stays highlighted on the other canvas, however the 12 pixel marker on the active canvas kind of "obstructs" the view, while the previous 5 pixel marker was barely visible behind the mouse crosshair (a single pixel was visible in each quadrant).

I'd suggest to keep the 5 pixel ellipse when non hovering, use an odd number (13?) when hovering, and disable the hovering mode when a point has been picked, either in the current canvas or both canvases if that complicates things.

@nirvn
Copy link
Contributor Author

nirvn commented Feb 15, 2025

@uclaros , we can disabled the hovered state when moving, that'll give us as much of a view of the canvas as possible.

With regards to the change from 5 to 6 point width, wasn't the previous size creating an issue whereas the marker wasn't aligned to the X,Y map canvas coordinate itself? When calling QgsGCPCanvasItem::paint, the QPointF( 0,0 ) there represents the center of the coordination location on the map canvas, by having a 5 x 5 ellipse marker drawn at QPointF( -2, -2 ), we end up with a marker with a misalignment of QPointF( 0.5, 0.5 ) against the coordinate it is representing:

image

So I guess then question becomes, is it better to be aligned to the crosshair cursor (I assume the shape varies across OSes, etc.) and be slightly misaligned with the map canvas itself, or vice versa. I thought map canvas accuracy was better, hence the change.

@nirvn nirvn force-pushed the georeferencer_move_snap branch from 0f4a19a to d6cf21b Compare February 15, 2025 04:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants