Qgspolyhedralsurface fix is valid

[ptitjano]

Description

QgsPolyhedralSurface::isValid checks that all its polygon are valid
with GEOS. However, GEOS is not able to properly handle 3D polygons
because it drops the Z component.

For example, "Polygon Z((0 0 0,0 0 7,0 5 7,0 5 0,0 0 0))" is
wrongly considered as an invalid polygon because it contains duplicate
nodes when the Z component is dropped.

This issue is fixed by calling QGIS internal geometry validator which
is able to handle 3D polygons.

Fixes: 721b045

[ptitjano]

GEOS isValid is not able to properly handle 3D polygons.. With @lbartoletti, we found this solution to use QGIS geometry validator instead. However, It seems wrong to create a QgsGeometry in QgsPolyhedralSurface. Any thoughts?

[github-actions]

🪟 Windows builds

Download Windows builds of this PR for testing.
Debug symbols for this build are available here.
(Built from commit e3d25a7)

🪟 Windows Qt6 builds

Download Windows Qt6 builds of this PR for testing.
(Built from commit e3d25a7)

[nyalldawson]

Can we take a step back and look at the use case here? What are you using isValid for? Because the most common use of this function is testing whether a geometry is safe to use BY GEOS, eg for an intersection test.

So I'm concerned about hacking this and reporting true in cases where this will break that common use case.

Maybe we need extra flags for the function to specify whether we are working in a 2.5d model or 3d model....

[ptitjano]

Can we take a step back and look at the use case here? What are you using isValid for? Because the most common use of this function is testing whether a geometry is safe to use BY GEOS, eg for an intersection test.

So I'm concerned about hacking this and reporting true in cases where this will break that common use case.

Maybe we need extra flags for the function to specify whether we are working in a 2.5d model or 3d model....

You're right. I should have explained it in the first place.
The use case is using QgsPolyhedralSurface for processing such as in this plugin: https://gitlab.com/Oslandia/qgis/qsfcgal

At the moment, some processing algorithms from this plugin fail because QgsPolyhedralSurface::isValid wrongly returns false on valid geometries. Of course, you can tweak the flags from the processing widget but it's a bad user experience and some people won't know how to make it or conclude that the processing algorithm is broken.

At this point, I think it would be easier to just always return true with a warning message saying that isValid is not implemented. And later on to introduce it with SFCGAL support as mentioned previously

[nyalldawson]

@ptitjano ok, thanks for the info!

Here's how this should be handled:

1. Create an enum in Qgis for GeometryValidityModel, with values for 25d and 3d.
2. Add this enum as an argument to the QgsGeometry/QgsAbstractGeometry isValid methods. Default to 25d model to maintain API.
3. Add this as a getter/setter to QgsFeatureRequest, alongside the existing setInvalidGeometryCheck. Default to 25d mode. Make sure the iterator methods are updated accordingly to use the model specified from the feature request.
4. Add validity model argument for QgsProcessingFeatureSource::getFeatures. Default to 25d model. Make sure this class passes the model on when getting iterators from the underlying source.
5. Update the processing algorithms in your plugin to specify the 3d model when calling getFeatures on the sources.

Some other things to note:

• it'd be possible to adapt QgsGeometryValidator to work on const QgsAbstractGeometry objects by extracting the internal logic which works with these to public methods.
• that said, QgsGeometryValidator is sloooow (orders of magnitude or worse) because it doesn't use any spatial indices (unlike GEOS' validity checks)

[github-actions]

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]

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]

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.

[nyalldawson]

@ptitjano what's the status here? This looks stale to me. Can we close?

[ptitjano]

@ptitjano what's the status here? This looks stale to me. Can we close?

I'm still working on it.

[github-actions]

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]

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.