Georeferencer fixes, pt 2

[nyalldawson]

Follow up #47141

Supersedes #46941

[roya0045]

I'm not sure how the changes of 46941 are integrated in this, as far as I understand the number of columns are the same, the main difference is that changing the value of the point in the table now changes the crs of the stored point and the values stored in the point.

The approach proposed in 46941 was to exposed the hidden values of the points, that way the user could maintain the stored coordinates in the intended crs and still change the initial or transformed values as needed. This would be needed if the stored values of the point are in a distinct crs than the output and would need a tweak( such as using known geographic coords on a map). Now the used would need to change the target crs for the whole widget to the ones of the points that need a change to 'expose' the values in the proper way and then give them the correct values before reverting back. I agree those cases may be rare, but I don't think the proposed changes make this convivial.

Wether that behavior is worth crowding up the table with two extra columns, I assumed we had enough room for it.

[nyalldawson]

@roya0045

Wether that behavior is worth crowding up the table with two extra columns, I assumed we had enough room for it.

That's my big concern. Adding the extra columns in the table makes it very complex and information heavy, and I suspect that even experienced qgis users would have difficulties interpreting all the columns and information exposed.

I think we have two possibilities here:

1. the approach used in this pr, where we only ever show the destination coordinates in the actual output crs. Users can still create new points in a different crs, but these points are always shown as transformed to the output crs. I think this is easiest for users to understand, with a cost of a little less flexibility (users can't see a number of points with different destination crs in their untransformed coordinates)
2. add a read only "dest crs" column, and only ever show destination coordinates in the crs which they were originally created in. If the points are moved, then we transform them back to the original crs for display/storage. Benefits are the added flexibility of supporting mixed destination crs, but downsides are the extra complexity for users (and potential frustration if a user creates a point in an incorrect destination crs and can only delete and recreate the point to fix)

My preference obviously goes to 1, but I'd value wider feedback (ping @gioman @nirvn @3nids @m-kuhn )!
Even with approach 1, I would like to make the output crs more prominent in the dialog, eg by showing it explicitly somewhere below/above the table...

[gioman]

but I'd value wider feedback (ping @gioman

agree.

[roya0045]

I'm fine with either, it's a tradeoff, my only worry was with multiple change in crs, was if the points could 'drift' . Hence why I wanted to keep the original coords and transform them, though this fear might be unfounded.

As for the output crs I think we could fit it at the bottom of the window.

[nyalldawson]

my only worry was with multiple change in crs, was if the points could 'drift

That's indeed a valid concern! I think it's only an extreme corner case though -- it would only have an impact on results when:

1. A user is georeferencing something with high accuracy requirements
2. They enter some points in one crs (with high accuracy!)
3. They engage in multiple cycles of changing the target crs, and then modifying the destination coordinate (either manually in the table, or via the move point tool)

Step 3 is the big one here. If they don't do multiple cycles of change crs+move point, then there's only a single transform involved at the actual "run georeference" time.

I can't see this affecting real world use cases as a result, so that pushes me to the simpler interface/UX option.

[qgis-bot]

The backport to release-3_22 failed:

The process '/usr/bin/git' failed with exit code 1

stderr

error: could not apply ea2bf1b766... [georeferencer] Cleanup target CRS handling, and ensure destination
hint: After resolving the conflicts, mark them with
hint: "git add/rm <pathspec>", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

stdout

Auto-merging src/app/georeferencer/qgsgeorefmainwindow.cpp
CONFLICT (content): Merge conflict in src/app/georeferencer/qgsgeorefmainwindow.cpp
Auto-merging src/app/georeferencer/qgsgeorefmainwindow.h
Auto-merging src/app/georeferencer/qgstransformsettingsdialog.cpp
CONFLICT (content): Merge conflict in src/app/georeferencer/qgstransformsettingsdialog.cpp
Auto-merging src/app/georeferencer/qgstransformsettingsdialog.h
CONFLICT (content): Merge conflict in src/app/georeferencer/qgstransformsettingsdialog.h

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-release-3_22 release-3_22
# Navigate to the new working tree
cd .worktrees/backport-release-3_22
# Create a new branch
git switch --create backport-47279-to-release-3_22
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick ea2bf1b7664265dbad0f7634968edb019bafed09,ee7ffe9f779a7c1b17af25257ffebb84e50e5a43,d1b9a3ff32d96cbd220a9a499a07774077afba28,40a8b4386ebb52bacaf8dea049917fa6d05b942f,d5fe47d307a37d00f88ea95acdb91ad6a812d159,d9542c3dcf382dbc61066ed1e7c93e21d536d5a0,c613e1791efc400a6643bcbc5d2ff35a3681b7e2,287605d0964a7f39db8ebc9e64292c88e8365c71,903776b8964b19bd5813dbd6a2becc636f2ec356,5d248287bf72659f97fe9f0f4c97551f26b1d6de,39e1581a2b11417126b897955805f6d0456f0115,ff345d363d68074ecca4386e4a10c5cab2cf9650
# Push it to GitHub
git push --set-upstream origin backport-47279-to-release-3_22
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-release-3_22

Then, create a pull request where the base branch is release-3_22 and the compare/head branch is backport-47279-to-release-3_22.