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

Fix issue with vertical lines not visible in the elevation profile tool #58959

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ptitjano
Copy link
Contributor

@ptitjano ptitjano commented Oct 3, 2024

Description

This PR reverts commit ce96dee.

This commit was introduced to gain some performance when the
tolerance is set by using request.setDistanceWithin instead of
request.setFilterRect. However, in that case, the providers either
use GEOS::distance or GEOS::distanceWithin which is not able to
handle the Z component of a Geometry.
For example, a purely vertical line, i.e.: LineString(X Y Z1, X Y Z2) will always be to an infinite distance of any geometry according
to GEOS.

In practice, this means that a purely vertical line will never be
visible in the elevation profile when the tolerance is set. Indeed,
the provider will always discard such geometry because of the usage
of setDistanceWithin. This issue is fixed by always using
setFilterRect. This might introduce a performance penalty but this
ensures that the result is always correct.

…equest.setDistanceWithin` when tolerance>0 as some providers as improved performances with setDistanceWithin"

This reverts commit ce96dee.

This commit was introduced to gain some performance when the
`tolerance` is set by using `request.setDistanceWithin` instead of
`request.setFilterRect`. However, in that case, the providers either
use `GEOS::distance` or `GEOS::distanceWithin` which is not able to
handle the Z component of a Geometry.
For example, a purely vertical line, i.e.: `LineString(X Y Z1, X Y
Z2)` will always be to an infinite distance of any geometry according
to GEOS.

In practice, this means that a purely vertical line will never be
visible in the elevation profile when the tolerance is set. Indeed,
the provider will always discard such geometry because of the usage
of `setDistanceWithin`. This issue is fixed by always using
`setFilterRect`. This might introduce a performance penalty but this
ensures that the result is always correct.
@github-actions github-actions bot added this to the 3.40.0 milestone Oct 3, 2024
@ptitjano ptitjano self-assigned this Oct 3, 2024
@ptitjano ptitjano added Bug Either a bug report, or a bug fix. Let's hope for the latter! Profile tool labels Oct 3, 2024
Copy link

github-actions bot commented Oct 3, 2024

🪟 Windows builds ready!

Windows builds of this PR are available for testing here. Debug symbols for this build are available here.

(Built from commit 2e5df52)

@ptitjano ptitjano force-pushed the elevation-profile-fix-vertical branch from 42d217c to 2e5df52 Compare October 3, 2024 20:32
@nyalldawson
Copy link
Collaborator

However, in that case, the providers either
use GEOS::distance or GEOS::distanceWithin which is not able to
handle the Z component of a Geometry.
For example, a purely vertical line, i.e.: LineString(X Y Z1, X Y Z2) will always be to an infinite distance of any geometry according
to GEOS.

I think this special case should be handled in the iterator (or geometry) classes then, not specifically the elevation profile code

@ptitjano
Copy link
Contributor Author

ptitjano commented Oct 4, 2024

However, in that case, the providers either
use GEOS::distance or GEOS::distanceWithin which is not able to
handle the Z component of a Geometry.
For example, a purely vertical line, i.e.: LineString(X Y Z1, X Y Z2) will always be to an infinite distance of any geometry according
to GEOS.

I think this special case should be handled in the iterator (or geometry) classes then, not specifically the elevation profile code

Thanks for you message. However, I don't understand it. Do you mean that the iterator code of the providers should be changed? Or something else?

@nyalldawson
Copy link
Collaborator

. Do you mean that the iterator code of the providers should be changed?

I mean if the setDistanceWithin method for QgsFeatureRequest can't handle this situation, then it needs to be fixed in the iterator code. Otherwise other users of setDistanceWithin will remain broken.

So maybe the fix needs to apply in QgsGeos::distance / QgsGeos::distanceWithin instead, eg by detecting a vertical line and using a GEOS point geometry there instead...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Either a bug report, or a bug fix. Let's hope for the latter! Profile tool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants