In georeferencer, allow to move and delete GCPs without perfect aiming #37256

uclaros · 2020-06-16T21:58:30Z

Description

In the georeferencer, one has to aim frustratingly good in order to use the Delete GCP and Move GCP point tool.
This PR fixes this by allowing a manhattan distance of 10 between the mouse cursor and the actual GCP, similarly to the way the vertex tool works.

QgsGeorefDataPoint::contains is no longer used; should I remove it?

nirvn · 2020-06-17T02:02:37Z

Nice, that's a nice PITA taken care of.

nyalldawson · 2020-06-17T22:32:39Z

src/app/georeferencer/qgsgeorefmainwindow.cpp

+  // This is the manhattan length from mouse cursor point to GCP
+  // We will use this distance to pick the closest GCP to the cursor
+  // GCPs farther than the initial minDistance are too far and are ignored
+  qreal minDistance = 10;


instead of the hardcoded value -- could we use QgsMapTool::searchRadiusMU/QgsMapTool::searchRadiusMM() instead?

I guess we could, but is there a real benefit from it?
AFAICT, searchRadiusMM is pretty much tied to the identify tool, so the options and code comment descriptions will have to be adapted too.
I can imagine someone raising this value to identify multiple features at once, or minimizing it for better identify precision, and then struggle with the georeferencer and his strange searchRadiusMM value choice!

Hmm - I think we need an equivalent central method for map editing tools then. My concern with a hardcoded pixel value is that it's going to be completely inappropriate for certain setups (e.g. hidpi screens). We need something which accounts for dpi also, which is why a mm based size would be preferable.

There is also the Search radius for vertex edits setting (/qgis/digitizing/search_radius_vertex_edit) that looks more appropriate, but it can also be set for map units which makes no sense for the georeferencer case...

@nyalldawson I think the easiest solution is to just set the hardcoded value to a larger number if 10 is too small for hidpi displays (I cannot test).
A too small value will make the tool function like it does now, needing better aiming from the user.
A slightly large value will cause no real issue, since the GCP closest to the click point will be moved, the user will still expect this GCP to be moved.
A huge value could be a little strange as the closest GCP might be very far from the click position and the user might not make the connection.

I gave qreal minDistance = mCanvas->physicalDpiX() / 10; a shot and it works nice though slightly different on each of my two displays. Could you please test it on a hidpi setup?

Gives around 7mm radius here

I also get around 7mm on a 24" 1920x1080 display and around 3mm on a 13" 1366x768 display.
Both values are very usable, way better than the current 0.1mm imho! =p
What do you think?

I think having involving dpi is a very good thing. And the number 10 should be configurable.

Well, if it's configurable then there is no need for it to be dpi dependent, is it?

stale · 2020-07-03T18:44:36Z

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

that all unit tests are passing
that all comments by reviewers have been addressed
that there is enough information for reviewers, in particular
- link to any issues which this pull request fixes
- add a description of workflows which this pull request fixes
- add screenshots if applicable
that you have written unit tests where possible
In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
If there is no further activity on this pull request, it will be closed in a week.

stale · 2020-08-29T08:50:07Z

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

that all unit tests are passing
that all comments by reviewers have been addressed
that there is enough information for reviewers, in particular
- link to any issues which this pull request fixes
- add a description of workflows which this pull request fixes
- add screenshots if applicable
that you have written unit tests where possible
In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
If there is no further activity on this pull request, it will be closed in a week.

stale · 2020-10-04T02:35:55Z

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

that all unit tests are passing
that all comments by reviewers have been addressed
that there is enough information for reviewers, in particular
- link to any issues which this pull request fixes
- add a description of workflows which this pull request fixes
- add screenshots if applicable
that you have written unit tests where possible
In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
If there is no further activity on this pull request, it will be closed in a week.

uclaros · 2020-10-09T10:22:32Z

Do you think this is on the right track? Should I un-stale it or should I let it go?

m-kuhn · 2020-10-10T11:07:07Z

I think this is on a good track. But we need the factor (10) to be configurable (visual impairment or other specific environment). Still for a good default value or multi screen setups the dpi needs to be taken into account. I.e. the config should reflect distance in mm, not pixels. Ideally this is shared with another setting as discussed earlier.

stale · 2020-11-07T21:57:43Z

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

that all unit tests are passing
that all comments by reviewers have been addressed
that there is enough information for reviewers, in particular
- link to any issues which this pull request fixes
- add a description of workflows which this pull request fixes
- add screenshots if applicable
that you have written unit tests where possible
In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
If there is no further activity on this pull request, it will be closed in a week.

uclaros · 2020-11-13T10:27:48Z

Giving this a last look before abandoning and noticed there could be a much simpler approach:
Right now, the gcp-clicked logic uses QgsGeorefDataPoint::contains() which in turn calls mGCPSourceItem->shape().contains(); . Contrary to what I thought at first, mGCPSourceItem->shape() is not the QPainterPath used to draw the point on the canvas, but a temporary QPainterPath with the same ellipse size, used only to perform this contains() query.
That means we could enlarge the ellipse returned by mGCPSourceItem->shape() by doubling or quadrupling its radii, without affecting the displayed point size while doubling or quadrupling the click distance uniformly for low dpi and hidpi layouts.
What do you think?

nirvn · 2020-11-29T13:49:05Z

@uclaros , I'd love to see this pushed through the finish line. Are you able to take @m-kuhn 's comments into account and adjust the PR?

uclaros · 2020-12-01T18:48:20Z

@nirvn

Regarding configurability, I can add a georeferencer specific setting.
Regarding mm instead of pixels, I am not sure how I can achieve that if not with mCanvas.physicalDpiX() or Y. Any input is welcome.
Regarding reusing other setting, there are two candidate settings but none of them is appropriate as discussed.

m-kuhn · 2020-12-01T20:34:18Z

Can you remind me what's wrong with physicalDpi?

uclaros · 2020-12-03T14:15:11Z

One problem is it does not work as I expected, I might be misunderstanding something:
Right now, I am using qreal minDistance = mCanvas->physicalDpiX() / 10; expecting a distance of 0.1inch or 2.5mm but, as I mentioned in the discussion, I get 7mm on a 24" 1920x1080 display and around 3mm on a 13" 1366x768 display.

Furthermore, I came to realize that we do not want this distance to be in mm. We want this distance to be measured in the smallest unit of the feedback of the user's interaction, ie the smallest distance one can move the mouse and see it moving on the screen.
For normal displays that unit is the pixel. Not sure what that is for hidpi, it could be pixel * mCanvas->devicePixelRatioF()?

Use proper distance instead of manhattan length Scale pixel distance by mCanvas->devicePixelRatioF()

uclaros · 2020-12-07T21:44:20Z

Update:

Added a georeferencer specific setting for the minimum point selection distance, with a default value of 10, minimum 4.
Multiplied the distance by mCanvas->devicePixelRatioF(), to increase the distance proportionally for hidpi
Compare with the actual distance instead of the manhattan length so we have an theoretical circle instead of a rhombus.
Added ctrl+e shortcut to move GCP. Add is ctrl+a, delete is ctrl+d. Ctrl+m was not used because it is not a one hand shortcut.
Rebased

uclaros · 2020-12-23T15:43:35Z

@nirvn or @m-kuhn could you please evaluate the latest changes on a hidpi setup?

m-kuhn · 2020-12-23T16:28:39Z

devicePixelRatio is 1.0 here despite the HiDpi screen. This might be a local configuration issue though.
Can you give me your numbers for:

iface.mapCanvas().devicePixelRatioF()
1.0
iface.mapCanvas().physicalDpiX()
210

P.S.

Furthermore, I came to realize that we do not want this distance to be in mm. We want this distance to be measured in the smallest unit of the feedback of the user's interaction, ie the smallest distance one can move the mouse and see it moving on the screen.

I dare to challenge this statement. When a user is aiming for a point, it's his eye and hand which are the limitation. These sensors don't work in pixels, they work in real world distances. So we want the setting and default value to be in mm and the computer calculate the equivalent in pixels for us.

uclaros · 2020-12-25T00:36:57Z

iface.mapCanvas().devicePixelRatioF() is 1.0 for all my screens.
iface.mapCanvas().physicalDpiX() is 118 for my laptop's 1366x768 13" screen and 72 for my desktop's 1920x1080 24" screen.

I dare to challenge this statement. When a user is aiming for a point, it's his eye and hand which are the limitation. These sensors don't work in pixels, they work in real world distances. So we want the setting and default value to be in mm and the computer calculate the equivalent in pixels for us.

I do not agree. The hand and mouse sensor works in real world distances indeed, but it's all pixels once the pointer is displayed on the screen! You can see the mouse move less than a mm but you cannot see the mouse pointer move less than one pixel.
Here's a sample case with distance in mm to highlight the issue:
Let's say we wanted the default distance to be 10mm. That translates to 28 pixels on my 24" screen, not too big, not too small, nice ux.
On a 3.5" 1080p portable monitor 10mm would be like 130 pixels, allowing the mouse to move 5 times the distance on the mousepad than with the bigger screen.
Then on a 75" tv with the same resolution that would be almost 10 pixels, 3 times harder to point with the mouse than on the normal screen.

m-kuhn · 2020-12-25T07:10:50Z

On a 3.5" 1080p portable monitor 10mm would be like 130 pixels, allowing the mouse to move 5 times the distance on the mousepad than with the bigger screen.

Are you sure that's how things work? I.e. the higher the physical dpi the slower your mouse moves on the screen?

uclaros · 2020-12-25T10:55:06Z

AFAIK, same distance traveled by the mouse on the mouse pad results in same pixels traveled by the pointer on the screen, regardless of screen size or resolution (granted that no mouse acceleration setting is enabled of course).
You can test this by lowering your screen resolution, then checking that you can reach the screen bounds with smaller mouse movements.
Merry X-mas!

m-kuhn · 2020-12-25T11:42:20Z

Even if this is true, the main reason for me stays visual recognition. What I perceive as "close enogh to snap" is not related to pixels.

uclaros · 2020-12-25T12:31:17Z

What I perceive as "close enogh to snap" is not related to pixels.

It definitely is. If 10mm was your close enough to snap, would you be able to operate on a concert video wall where pixels are 10mm apart?

m-kuhn · 2020-12-25T13:04:32Z

You are right,it's not perfect. But still better than px for the average everyday gis scenario I guess? And I cannot see a perfect solution unfortunately.

uclaros · 2020-12-25T21:20:32Z

I am sorry, but I completely disagree. If it was in any way better than px then snapping tolerance would have been implemented in screen mm instead of px. Same for qt buttons and menus and widgets in general.

nyalldawson · 2020-12-25T21:45:32Z

@uclaros that's not quite right -- a lot of this stuff was in place from way before hidpi was even a concept, so we can't take past decisions as correct here.

For reference, all the qt docs describe best practice as basing screen sizes on font metrics (ie real world units like mm) instead of pixels.

uclaros · 2020-12-25T22:36:52Z

font metrics (ie real world units like mm)

I don't think that is so. Font metrics actually mean pixels, after any scaling done by the os or the display driver. Hence, QFontMetrics and QFontMetricsF methods return values are in pixels

nyalldawson · 2020-12-25T23:32:58Z

Font metrics return pixels, but they are converted from fixed values in terms of character sizes (in real world units). That's why they are used as a conversion route from "how big should things actually appear on the screen" to "what size does this correspond to in pixels"

uclaros · 2020-12-26T00:48:33Z

Wouldn't then a QFont font("times", 24); need to be the same size in mm on my phone, pc and videowall regardless of screen resolution?

nyalldawson · 2020-12-26T00:57:34Z

@uclaros you omit the font size -- ie.

QFontMetrics fm( QFont() );

that uses the default application font sizing then -- so basically ui elements and measurements are effectively specified in "multiple of menu/standard widget height" units. The thinking there is that for a videowall the user will have already set the default font size to a suitable size, using whatever scaling factors/dpi/font size settings their particular operating system offers.

uclaros · 2020-12-26T01:34:09Z

Maybe I'm tired, but I still do not see where real world units are. Isn't the default application font defined by the application itself, regardless of display?

github-actions · 2021-01-09T03:11:47Z

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

that all unit tests are passing
that all comments by reviewers have been addressed
that there is enough information for reviewers, in particular
- link to any issues which this pull request fixes
- add a description of workflows which this pull request fixes
- add screenshots if applicable
that you have written unit tests where possible
In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
If there is no further activity on this pull request, it will be closed in a week.

github-actions · 2021-01-16T03:33:29Z

While we hate to see this happen, this PR has been automatically closed because it has not had any activity in the last 21 days. If this pull request should be reconsidered, please follow the guidelines in the previous comment and reopen this pull request. Or, if you have any further questions, just ask! We love to help, and if there's anything the QGIS project can do to help push this PR forward please let us know how we can assist.

github-actions bot added this to the 3.14.0 milestone Jun 16, 2020

uclaros closed this Jun 17, 2020

uclaros reopened this Jun 17, 2020

nyalldawson reviewed Jun 17, 2020

View reviewed changes

nyalldawson modified the milestones: 3.14.0, 3.16.0 Jun 19, 2020

stale bot added stale Uh oh! Seems this work is abandoned, and the PR is about to close. and removed stale Uh oh! Seems this work is abandoned, and the PR is about to close. labels Jul 3, 2020

stale bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Aug 29, 2020

uclaros force-pushed the georef-move-near branch from 976e624 to 1fb0b3e Compare September 4, 2020 17:30

stale bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Sep 4, 2020

uclaros closed this Sep 7, 2020

uclaros reopened this Sep 7, 2020

stale bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Oct 4, 2020

stale bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Oct 9, 2020

nyalldawson modified the milestones: 3.16.0, 3.18 (Feature) Oct 23, 2020

stale bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Nov 7, 2020

stale bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Nov 13, 2020

uclaros added 4 commits December 7, 2020 18:52

Allow to move and delete GCPs without perfect aiming

471c847

Make minDistance dpi dependable

af65432

Add setting for point selection distance

fd64742

Use proper distance instead of manhattan length Scale pixel distance by mCanvas->devicePixelRatioF()

Added move GCP keyboard shortcut

a1affc8

uclaros force-pushed the georef-move-near branch from 1fb0b3e to a1affc8 Compare December 7, 2020 21:23

github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jan 9, 2021

github-actions bot closed this Jan 16, 2021

uclaros mentioned this pull request Oct 12, 2021

Georeferencer: Move GCP point, hitbox size #45491

Closed

In georeferencer, allow to move and delete GCPs without perfect aiming #37256

In georeferencer, allow to move and delete GCPs without perfect aiming #37256

Conversation

uclaros commented Jun 16, 2020 • edited

Description

nirvn commented Jun 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Jul 3, 2020

stale bot commented Aug 29, 2020

stale bot commented Oct 4, 2020

uclaros commented Oct 9, 2020

m-kuhn commented Oct 10, 2020

stale bot commented Nov 7, 2020

uclaros commented Nov 13, 2020

nirvn commented Nov 29, 2020

uclaros commented Dec 1, 2020

m-kuhn commented Dec 1, 2020

uclaros commented Dec 3, 2020

uclaros commented Dec 7, 2020 • edited

uclaros commented Dec 23, 2020

m-kuhn commented Dec 23, 2020 • edited

uclaros commented Dec 25, 2020

m-kuhn commented Dec 25, 2020

uclaros commented Dec 25, 2020

m-kuhn commented Dec 25, 2020

uclaros commented Dec 25, 2020

m-kuhn commented Dec 25, 2020 • edited

uclaros commented Dec 25, 2020

nyalldawson commented Dec 25, 2020

uclaros commented Dec 25, 2020

nyalldawson commented Dec 25, 2020

uclaros commented Dec 26, 2020

nyalldawson commented Dec 26, 2020

uclaros commented Dec 26, 2020

github-actions bot commented Jan 9, 2021

github-actions bot commented Jan 16, 2021

uclaros commented Jun 16, 2020 •

edited

uclaros commented Dec 7, 2020 •

edited

m-kuhn commented Dec 23, 2020 •

edited

m-kuhn commented Dec 25, 2020 •

edited