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

Add support for georeferencing vector layers in georeferencer #47577

Merged
merged 17 commits into from
Mar 7, 2022

Conversation

nyalldawson
Copy link
Collaborator

@nyalldawson nyalldawson commented Mar 1, 2022

Allows vector layers without spatial referencing to be interactively georeferenced (or layers with referencing to be re-referenced!).

Georeferencing occurs in a task, so it remains nice and responsive even with massive layers.

Sponsored by the Danish QGIS Usergroup

This PR builds on the work started by @roya0045 in #41386 -- full credit to Alex for the groundwork here!

Fixes #41300

@nyalldawson nyalldawson added Feature Needs Documentation When merging a labeled PR, an issue will be created in the Doc repo. Changelog Items that are queued to appear in the visual changelog - remove after harvesting labels Mar 1, 2022
@github-actions
Copy link

github-actions bot commented Mar 1, 2022

@nyalldawson
This pull request has been tagged as requiring documentation.

A documentation ticket will be opened at https://github.com/qgis/QGIS-Documentation when this PR is merged.

Please update the description (not the comments) with helpful description and screenshot to help the work from documentors.
Also, any commit having [needs-doc] or [Needs Documentation] in will see its message pushed to the issue, so please be as verbose as you can.

Thank you!

@github-actions github-actions bot added this to the 3.26.0 milestone Mar 1, 2022
@nyalldawson
Copy link
Collaborator Author

@roya0045 would love some feedback if you have a chance to build and test this branch!

@nyalldawson
Copy link
Collaborator Author

@3nids all done!

@roya0045
Copy link
Contributor

roya0045 commented Mar 1, 2022

@roya0045 would love some feedback if you have a chance to build and test this branch!

I don't think I can provide timely feedback, the sooner I might be able to do is this weekend and that might be unrealistic. I don't doubt the quality of the changes, the only element that I had issues with my own version was the exporter. It converted any outputs to a geopackage and I couldn't figure out why this format was imposed despite calling the exporter 'properly'.

Did you manage to find out what was causing this issue with the QgsVectorFileWriter?

Allows vector layers without spatial referencing to be interactively
georeferenced (or layers with referencing to be re-referenced!)

Sponsored by the Danish QGIS Usergroup
Since the georeferencer now supports vector layers too, putting this
action in the Raster menu is no longer appropriate

o# Changes to be committed:
that sublayers from databases can be georeferenced (also layers from
non-ogr sources!)
@nyalldawson
Copy link
Collaborator Author

@roya0045

I don't think I can provide timely feedback, the sooner I might be able to do is this weekend and that might be unrealistic. I don't doubt the quality of the changes, the only element that I had issues with my own version was the exporter. It converted any outputs to a geopackage and I couldn't figure out why this format was imposed despite calling the exporter 'properly'.

Did you manage to find out what was causing this issue with the QgsVectorFileWriter?

Ah, good catch! Fixed in b03611c.

@roya0045
Copy link
Contributor

roya0045 commented Mar 2, 2022

If that's fixed I assume the remainder is fine. 👍

Copy link
Contributor

@nirvn nirvn left a comment

Choose a reason for hiding this comment

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

@nyalldawson , I feel like an editor who's trying to justify his salary by pointing out frivolous issues here ;) small formatting comments added, couldn't find anything fundamentally wrong.

src/analysis/georeferencing/qgsgcppoint.cpp Outdated Show resolved Hide resolved
src/app/georeferencer/qgsgeorefmainwindow.cpp Show resolved Hide resolved
src/app/georeferencer/qgsgeorefmainwindow.cpp Show resolved Hide resolved
src/app/georeferencer/qgsimagewarper.h Outdated Show resolved Hide resolved
src/analysis/georeferencing/qgsvectorwarper.h Outdated Show resolved Hide resolved
@nyalldawson
Copy link
Collaborator Author

Thanks @nirvn, all addressed!

@nyalldawson nyalldawson merged commit 667886b into qgis:master Mar 7, 2022
@nyalldawson nyalldawson deleted the vector_georef branch March 7, 2022 09:13
@github-actions
Copy link

github-actions bot commented Mar 7, 2022

@nyalldawson
A documentation ticket has been opened at qgis/QGIS-Documentation#7384
It is your responsibility to visit this ticket and add as much detail as possible for the documentation team to correctly document this change.
Thank you!

@zacharlie zacharlie added ChangelogHarvested This PR description has been harvested in the Changelog already. and removed Changelog Items that are queued to appear in the visual changelog - remove after harvesting labels May 28, 2022
@agiudiceandrea
Copy link
Contributor

@nyalldawson, may this PR be the cause of #51197 and #51299?

@roya0045
Copy link
Contributor

roya0045 commented Jan 2, 2023

@nyalldawson, may this PR be the cause of #51197 and #51299?

Would be more likely to have been caused by something during the rework ( the previous PR affecting the georeferencer I recall). Regardless, of when, there's something to fix.

@agiudiceandrea
Copy link
Contributor

agiudiceandrea commented Jan 3, 2023

Would be more likely to have been caused by something during the rework

@roya0045, if you are referring to #47141 ("Fix lots of georeferencer issues") and #47279 ("Georeferencer fixes, pt 2"), they have been both allegedly backported to 3.22 (#47276), so the issues should have occurred also in version 3.22, while it seems not the case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ChangelogHarvested This PR description has been harvested in the Changelog already. Feature Needs Documentation When merging a labeled PR, an issue will be created in the Doc repo.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Georeferencer] Georeferencing vector data
6 participants