Fix lots of georeferencer issues #47141

nyalldawson · 2022-02-02T04:03:17Z

Before trying to fix the mess that has gotten into, we need a good test suite...!

roya0045 · 2022-02-02T12:42:38Z

Unsure if this can be helpful but there were some exchanges on #46415

The renaming of things is welcome.

nyalldawson · 2022-02-03T06:42:11Z

@roya0045 @gioman I'd love testing of this branch and confirmation that you don't see any remaining issues (aside from those described in #46941)

The size of these changes makes me nervous, but at the same time the georeferencer is quite broken in master/3.22 so we don't have any choice but to merge and backport and hope that we can test sufficiently prior to release...!

gioman · 2022-02-03T09:05:50Z

@nyalldawson I tested the windows artifact. The georef operation seems to get stuck (progress bar moves, but like 1% every 5 minutes) and GCP points are a no show in georef map window:

I'm using the data and configs from here #46901 (comment)

A few days back I tested the artifact for a now closed patch #46524 (comment) and worked ok (even if I understand that was not the right fix).

gioman · 2022-02-03T09:11:44Z

@nyalldawson GCP did show up when I double clicked somwhere in the GCP table, but that made the image to georef disappear

nyalldawson · 2022-02-03T09:16:13Z

@gioman I suspect the points file there is invalid, and has map coordinates for the raster instead of pixels. Creation of the gcp file is fixed here, but I'm not sure we can "auto repair" invalid ones...

gioman · 2022-02-03T09:17:58Z

@nyalldawson ok, I'll try with another dataset.

nyalldawson · 2022-02-03T09:28:08Z

@gioman

It's always possible that I've made the wrong choice here, and that we should be storing the source coordinates instead of pixels in the gcp file! I'm not sure what the original intention was, but we always interpret these values as pixels and the heading states "pixelX" so that's why I went that way.

But maybe the heading is what's wrong... 😳

Thinking about a future time when we support vector file georeferencing, it no longer would make sense to use pixels here.

So in short... I'm 50/50, and would value any insights anyone has!

gioman · 2022-02-03T09:49:29Z

@nyalldawson tested again from scratch. The operation seems to work ok. Anyway I have a few observations I will leave here. As a start;

I georeferenced a jpg using a project that had CRS 3763 and picking the GCP on the canvas. In georef configs I choose 3763 as target CRS. The output is added to QGIS with "unknown" CRS (but in the right place).

gioman · 2022-02-03T12:13:10Z

@nyalldawson tested again from scratch. The operation seems to work ok. Anyway I have a few observations I will leave here. As a start;

I georeferenced a jpg using a project that had CRS 3763 and picking the GCP on the canvas. In georef configs I choose 3763 as target CRS. The output is added to QGIS with "unknown" CRS (but in the right place)

@nyalldawson after all this seems to be the only outstanding issue. I tried also to georef to WGS84 and the resulting layer shows with "invalid" projection.

roya0045 · 2022-02-03T12:55:44Z

@gioman

It's always possible that I've made the wrong choice here, and that we should be storing the source coordinates instead of pixels in the gcp file! I'm not sure what the original intention was, but we always interpret these values as pixels and the heading states "pixelX" so that's why I went that way.

But maybe the heading is what's wrong... 😳

Thinking about a future time when we support vector file georeferencing, it no longer would make sense to use pixels here.

So in short... I'm 50/50, and would value any insights anyone has!

IMO the coordinates should be the good things to store, as ungeoreferenced images also have their own 'coordinates' and the pixel values are quite specific to a file, but the intent is to move one point to another and from one projection to another. The pixels are just where the point is for a given raster. I could see uses where there is a single gcp files to georeference multiple images, even if the points are out of the image the transformation should still be usable. IE if you have a lot of overlapping images, do a few generic points and then use the same gcps to georeference everything (if they are already georeferenced, from a non-referenced state this wouldn't work).

But as long as it works fine I don't mind it. But I agree that the requirement of this requires juggling with a lot of 'domains' which can be confusing (raster pixels, pts crs, origin, destinations, destination crs, etc.)

roya0045 · 2022-02-03T12:56:58Z

@gioman Thanks for testing! I wouldn't have much time at work or out of it to test it in depth as you did, even with a few days of delay.

nyalldawson · 2022-02-03T13:03:11Z

@roya0045

IMO the coordinates should be the good things to store

I think I'm leaning that way too (especially given the future vector consideration).

If we go that we we'll definitely need to fix the headers of the .points files though, as they are misleading right now. I'll see what's involved in making this switch tomorrow (and will also re-test with the raster/points from that ticket as a result)

roya0045 · 2022-02-03T13:05:21Z

@roya0045

IMO the coordinates should be the good things to store

I think I'm leaning that way too (especially given the future vector consideration).

If we go that we we'll definitely need to fix the headers of the .points files though, as they are misleading right now. I'll see what's involved in making this switch tomorrow (and will also re-test with the raster/points from that ticket as a result)

👍 Sure, let me know if I can be of assistance, though it might not be too timely.

src/app/georeferencer/qgsgeorefdatapoint.cpp

nyalldawson · 2022-02-08T02:07:17Z

@gioman @roya0045

I've re-worked this so that we ALWAYS use source coordinates when saving/loading gcp points files. This fixes the issue described in #46901 (that file works without issue now).

I'd love some more testing of this branch before we merge, if you've time!

(Also added a bunch of extra paper cut fixes to this branch, tests, cleaner code, ...)

roya0045 · 2022-02-08T12:37:43Z

I'll try to do some tests later today, thanks for doing this!

gioman · 2022-02-08T15:45:32Z

@nyalldawson I see weird things.

First case, canvas is in 3857, georef configured to 3763. The destination Y parameter shows with no decimals, double clicking on its cell a lot more show up (but really, on a projected CRS how many are used?), then when moving the focus the value change to one expressed in scientific notation

Recording.6.mp4

When canvas is in 4326, georef configured to 3763, the dest x/y values are in a projected crs(?), then if clicking in a cell of the gcp table values change to geographic, but only for dest x.

Recording.7.mp4

roya0045 · 2022-02-08T16:13:35Z

First case, canvas is in 3857, georef configured to 3763. The destination Y parameter shows with no decimals, double clicking on its cell a lot more show up (but really, on a projected CRS how many are used?), then when moving the focus the value change to one expressed in scientific notation

I think this is out of scope for the fix, the table should still use the default Qt number formatting. What matters is if the numbers are good.

Here is what I was expecting to test when I had the time:

Mixing projected and geographic coords in the input pts
Moving points on the canvas (with the same or a different origin crs)
georefercing an already georeferenced image
Change the destination crs to validate reprojection
(Maybe ) load a gcp file

gioman · 2022-02-08T16:14:45Z

I think this is out of scope for the fix

maybe, but is totally confusing,

raster file name for these and not the output raster file name The GCPs relate specifically to the ORIGINAL image, not the warped output exported after georeferencing. If we use the output file name for the .points file then when THIS georeferenced output is loaded into the georeferencer we get misleading location of source GCP points, as the pixel coordinates are from the ORIGINAL image, not the warped one.

…images

from class which stores graphical item representing point

This is too fragile, there's too many different situations which should lead us to invalidate the cached point which are not being caught.

are disabled

points instead of always using source pixel row/columns The "always use pixels" approach does not work well when a source image already has some georeferencing information attached. Also add tests for this, and fix loading of "enabled" status for points when loading points from a file

enabled And partially fix this functionality when using an already georeferenced image (not fully)

roya0045 · 2022-02-09T00:41:11Z

@nyalldawson Did my tests, everything seemed ok, even with a projected dataset.

points don't move on canvas when edited in list

nyalldawson · 2022-02-09T02:57:31Z

Thanks @roya0045 !

@gioman

I see weird things.

These aren't directly impacted by the changes in this PR, but should be fixed by #46941
I'd suggest we get this part merged first and then #46941 can be reworked on top of master to fix those issues, with appropriate tests added to suit...

roya0045 · 2022-02-09T12:45:12Z

I'm curious to see how much rework will be needed in 46941 & #41386 🤣

Nyall I know you had interest in 41386, let me do the cleanup and you can take over after if you want, no use in having you do some work I could be doing (albeit slower) during the freeze. Other than reworking the dwg importer I didn't find any easy bugs I could fix quickly and willingly at the moment, so I wouldn't mind updating my things in my free-time.

nyalldawson · 2022-02-09T19:26:55Z

Merged for wider testing

nyalldawson · 2022-02-10T01:56:58Z

@gioman

I see weird things.

These should all be fixed by #47279

gioman · 2022-02-10T23:41:23Z

These should all be fixed by #47279

@nyalldawson confirmed!

github-actions bot added this to the 3.24.0 milestone Feb 2, 2022

nyalldawson mentioned this pull request Feb 2, 2022

[georeferencer] Fix reprojecting projected raster #46524

Closed

3nids approved these changes Feb 2, 2022

View reviewed changes

nyalldawson force-pushed the georef_test branch from a2c3cbd to 0f44c97 Compare February 3, 2022 06:38

nyalldawson changed the title ~~Start on test suite for georeferencer~~ Fix lots of georeferencer issues Feb 3, 2022

nyalldawson added backport release-3_22 Regression Something which used to work, but doesn't anymore labels Feb 3, 2022

roya0045 reviewed Feb 3, 2022

View reviewed changes

src/app/georeferencer/qgsgeorefdatapoint.cpp Outdated Show resolved Hide resolved

nyalldawson force-pushed the georef_test branch from 0f44c97 to 6dd22b3 Compare February 8, 2022 02:05

nyalldawson added 3 commits February 9, 2022 08:22

Minor cleanup to rename ambiguous method

311c6f3

Start on test suite for georeferencer

da9bfd4

Spelling

81067b6

nyalldawson added 14 commits February 9, 2022 08:22

Fix incorrect display of residual lines for previously georeferenced …

a43f219

…images

Remove unused method/member

fd7ec96

Fix misleading override of QList::size

9cfe7c4

Cleanup georeferencer GCP data class by separating data point

a3577cb

from class which stores graphical item representing point

Fix memory leaks and make gcp container class safer to use

5cced15

Don't cache transformed destination points

3091d4d

This is too fragile, there's too many different situations which should lead us to invalidate the cached point which are not being caught.

Add test for createGCPVectors

3c3f472

[georeferencer] Fix recenter on points fails when some points in list

7ea0adf

are disabled

Fix logic determining when link georef to qgis canvas actions are

db9599d

enabled And partially fix this functionality when using an already georeferenced image (not fully)

Fix shortcut

247e232

Fix build on older Qt

111221a

wip

74d7e13

nyalldawson added 4 commits February 9, 2022 11:25

Fix reading destination CRS from .points files

23402c8

Never silently overwrite gcp point files without prompting user

d76fe79

Move code for calculating residuals to QgsGCPList out of model

977585c

Use a proper model for gcp list, fix use of delegates, fix gcp

ca2dbef

points don't move on canvas when edited in list

nyalldawson force-pushed the georef_test branch from 91a058f to ca2dbef Compare February 9, 2022 02:56

nyalldawson merged commit c5ca66d into qgis:master Feb 9, 2022

nyalldawson deleted the georef_test branch February 9, 2022 19:26

qgis-bot mentioned this pull request Feb 9, 2022

[Backport release-3_22] Fix lots of georeferencer issues #47276

Merged

nyalldawson mentioned this pull request Feb 10, 2022

Georeferencer fixes, pt 2 #47279

Merged

agiudiceandrea mentioned this pull request Jan 3, 2023

Add support for georeferencing vector layers in georeferencer #47577

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix lots of georeferencer issues #47141

Fix lots of georeferencer issues #47141

nyalldawson commented Feb 2, 2022

roya0045 commented Feb 2, 2022

nyalldawson commented Feb 3, 2022

gioman commented Feb 3, 2022

gioman commented Feb 3, 2022

nyalldawson commented Feb 3, 2022

gioman commented Feb 3, 2022

nyalldawson commented Feb 3, 2022

gioman commented Feb 3, 2022

gioman commented Feb 3, 2022

roya0045 commented Feb 3, 2022

roya0045 commented Feb 3, 2022

nyalldawson commented Feb 3, 2022

roya0045 commented Feb 3, 2022

nyalldawson commented Feb 8, 2022

roya0045 commented Feb 8, 2022 •

edited

Loading

gioman commented Feb 8, 2022

roya0045 commented Feb 8, 2022

gioman commented Feb 8, 2022

roya0045 commented Feb 9, 2022

nyalldawson commented Feb 9, 2022

roya0045 commented Feb 9, 2022

nyalldawson commented Feb 9, 2022

nyalldawson commented Feb 10, 2022

gioman commented Feb 10, 2022

Fix lots of georeferencer issues #47141

Fix lots of georeferencer issues #47141

Conversation

nyalldawson commented Feb 2, 2022

roya0045 commented Feb 2, 2022

nyalldawson commented Feb 3, 2022

gioman commented Feb 3, 2022

gioman commented Feb 3, 2022

nyalldawson commented Feb 3, 2022

gioman commented Feb 3, 2022

nyalldawson commented Feb 3, 2022

gioman commented Feb 3, 2022

gioman commented Feb 3, 2022

roya0045 commented Feb 3, 2022

roya0045 commented Feb 3, 2022

nyalldawson commented Feb 3, 2022

roya0045 commented Feb 3, 2022

nyalldawson commented Feb 8, 2022

roya0045 commented Feb 8, 2022 • edited Loading

gioman commented Feb 8, 2022

roya0045 commented Feb 8, 2022

gioman commented Feb 8, 2022

roya0045 commented Feb 9, 2022

nyalldawson commented Feb 9, 2022

roya0045 commented Feb 9, 2022

nyalldawson commented Feb 9, 2022

nyalldawson commented Feb 10, 2022

gioman commented Feb 10, 2022

roya0045 commented Feb 8, 2022 •

edited

Loading