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

[processing] Auto geometry fixer #35183

Open
roya0045 opened this issue Mar 18, 2020 · 6 comments
Open

[processing] Auto geometry fixer #35183

roya0045 opened this issue Mar 18, 2020 · 6 comments
Labels
Feature Request Processing Relating to QGIS Processing framework or individual Processing algorithms

Comments

@roya0045
Copy link
Contributor

roya0045 commented Mar 18, 2020

Feature description.
Sometimes when you try to process some data you get the invalid geometry, you never know which layer and you end up running fix geometry on the two inputs just to be safe.

Though this is a hassle.

We have an option to skip invalid geometry but you end up missing element.

The best option would be something that intercept the invalid geometry and make it valid by fixing it.

Though with the current codebase this is hard to do as the validation steps is far remote from the part that output the feature. Here is what I can think that might work:

  • rework the internal to now only provide true or false on check but to handle a geometry (would require a lot of refactoring).
  • output the feature with a flag or trigger to intercept it on the way out and fix it there
  • use a signal to intercept the feature and fix it.
  • when encountering an error, fix the faulty input and restart automatically with the temporary fixed input.
  • there could be a data checkup before processing and fix the whole dataset before starting but this would add some runtime.
@uclaros
Copy link
Contributor

uclaros commented Mar 19, 2020

Keep in mind that fix geometries does not always provide the fix the user wants to apply to the data so it should not be applied automatically and silently. For example fixing a self intersection might generate a new feature, while the user would prefer to delete a couple of vertices, and that may cause unexpected results in the processing algorithm.

@roya0045
Copy link
Contributor Author

@uclaros I don't think there can be a magic solution to geometry errors, I simply feel that since fix geometry would be the function that is applied anyway to a geometry when the user receive this error, then doing it within the process is not that bad.
And yes a message/popup would be beneficial. Its simply that to implement this efficiently its not that easy with the current flow.

@uclaros
Copy link
Contributor

uclaros commented Mar 19, 2020

@roya0045 For me, Check validity is the next step when I receive this error on data that I want to have control over, not an automatic fix, so more info on the geometry error would me welcome!

The current setting Invalid features filtering has the following options:

  • Do not filter (better performance)
  • Skip (ignore) features with invalid geometries
  • Stop algorithm execution when a geometry is invalid

Maybe there could be a fourth option to do as you propose like fix invalid geometries and retry.

@roya0045
Copy link
Contributor Author

@uclaros yes this is what I'm proposing.

@nyalldawson
Copy link
Collaborator

@roya0045 some rough notes on how you'd do this:

  • the guts all happens in qgsfeaturerequest.cpp and qgsvectorlayerfeatureiterator.cpp
  • you need to add a new value to the QgsFeatureRequest::InvalidGeometryCheck enum and then in QgsVectorLayerFeatureIterator::checkGeometryValidity you'd handle the new "AutomaticallyRepairGeometry" value by repairing the geometry (via QgsGeometry::makeValid() ) and assigning the repaired geometry to the feature, then return true.

You'd then need to handle the new enum value in various places in processing. You'll get build warnings everywhere you need to handle that.

New tests would need to be added to TestQgsProcessing::features() covering the functionality.

One warning: makeValid is a lossy operation. You'll silently drop z and m values from the geometry by calling it, so you want to make sure all the new documentation states this warning in strong terms!

@roya0045
Copy link
Contributor Author

@nyalldawson Thanks Nyall, I had already noted this and done a part of what you suggested in my prototype. But I rapidly found out that there was no way to replace a feature from the iterator as they are not an output of any function. If features were provided on request there could be a way to replace it with a fixed one.

If I do anything I want to make sure that the original dataset stays intact. Hence why I was opting to replace a provided feature ( would need to make a feature handler that fetches the current feature from the iterator and fixes it) or fix the whole input and use the temporary result as the new input.

Thanks for telling me where the test is!

@alexbruy alexbruy added the Processing Relating to QGIS Processing framework or individual Processing algorithms label Apr 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request Processing Relating to QGIS Processing framework or individual Processing algorithms
Projects
None yet
Development

No branches or pull requests

4 participants