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 elevation profile on closed polyline (fix #58450) #58949

Merged
merged 1 commit into from
Oct 23, 2024

Conversation

dvdkon
Copy link
Contributor

@dvdkon dvdkon commented Oct 2, 2024

Description

Currently, the elevation profile code for vector layers (i.e. polylines) goes through each part of the target curve and uses lineLocatePoint(QgsPoint) to see how far a point is on the curve. This, however, can't work for closed curves (or in general curves which use a single point multiple times), since there are multiple answers for "how far along the curve is this point?".

This PR fixes the problem by splitting the profile curve into multiple segments which don't self-intersect, and processing each independently. This should fix the issue for all affected polylines while having no effect for previously-working inputs.

Fixes #58450

@github-actions github-actions bot added this to the 3.40.0 milestone Oct 2, 2024
Copy link

github-actions bot commented Oct 2, 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 035aae4)

Copy link

github-actions bot commented Oct 10, 2024

🪟 Windows builds

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

🪟 Windows Qt6 builds

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

@dvdkon dvdkon marked this pull request as ready for review October 10, 2024 15:56
@dvdkon dvdkon force-pushed the fix-58450 branch 6 times, most recently from 12b229f to 939ea06 Compare October 14, 2024 09:52
src/core/geometry/qgslinestring.cpp Outdated Show resolved Hide resolved
src/core/vector/qgsvectorlayerprofilegenerator.cpp Outdated Show resolved Hide resolved

for ( QgsLineString &part : profileLine->splitToDisjointXYParts() )
{
mProfileCurve.reset( &part );
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is rather ugly -- we are assigning a unique_ptr a value which it does not have ownership on. I would suggest instead doing something like:

   QVector< QgsLineString * > parts = profileLine->splitToDisjointXYParts();
   while ( !parts.isEmpty() )
   {
      mProfileCurve.reset( parts.takeFirst() );

(This will require modifying splitToDisjointXYParts to return pointers in the list, which is arguably more flexible anyway)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is ugly. I didn't want to have splitToDisjointXYParts() return a list of pointers, to avoid allocations.

Returning a list of unique_ptr doesn't work with QVector (it copies elements on mutation), so I switched to std::vector. Sadly now SIP complains that "splitToDisjointXYParts has an unsupported return type", so I've excluded it from the Python bindings for now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is ugly. I didn't want to have splitToDisjointXYParts() return a list of pointers, to avoid allocations.

Trust me -- In this case, you'll actually end up with an API which AVOIDS more allocations by returning pointers here. This is because of the mix of geometry classes in QGIS, and the different ways these are used. There's many classes which require QgsGeometry objects (or methods ONLY available from QgsGeometry), so we end up having to create QgsGeometry objects taking ownership of QgsAbstractGeometry in order to call these. A prime example would be something like QgsFeature -- in order to set the geometry for a feature, we need a QgsGeometry value, and that can ONLY be created with a heap allocated QgsAbstractGeometry.

So by returning values from a method like splitToDisjointXYParts we'll end up having to clone those QgsAbstractGeometry values in order to transfer them to a QgsGeometry... which means we're heap allocating anyway PLUS incurring the cost of the clone.

I agree it's not ideal, but we're dealing with a bunch of constraints here which we just have to live with, eg

  • Qt API isn't designed around modern c++ memory handling, and FORCES us to use a lot of raw pointers and manual memory management to fit in with their parent/child object model
  • The SIP Python binding generator has NO concept of modern c++, and FORCES us to use raw pointers and manual memory management if we want to expose methods to PyQGIS
  • We have a stable API constraint forcing us to live with the API decisions from back in the 3.0 era, so we're even constrained by QGIS API itself 🙀

I would LOVE to be able to use unique_ptr throughout our API in order to modernise things, but the sad reality of QGIS development is that we just CAN'T do this right now.

Returning a list of unique_ptr doesn't work with QVector (it copies elements on mutation), so I switched to std::vector. Sadly now SIP complains that "splitToDisjointXYParts has an unsupported return type", so I've excluded it from the Python bindings for now.

As per my comments above, this is something you just need to learn to compromise on, and hold back the vomit 😉 and expose as an unsafe QList< pointer *> return value (along with the SIP_FACTORY annotation to indicate that the caller takes ownership of the objects).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your rationale on the allocations, that makes sense.

I really don't like returning raw pointers, and lists of them are even worse. But if you think it's the only way for now, I'll change the code. Hopefully one day SIP will support smart pointers and we can refactor at least some of the ugliness away.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the flexibility!

Hopefully one day SIP will support smart pointers and we can refactor at least some of the ugliness away.

It's possible we could add support for std::vector< std::unique_ptr< T > > return types via some new conversion code in https://github.com/qgis/QGIS/blob/master/python/core/conversions.sip, but AFAIK no-one has attempted that yet...

src/core/vector/qgsvectorlayerprofilegenerator.cpp Outdated Show resolved Hide resolved
tests/src/core/geometry/testqgsgeometry.cpp Show resolved Hide resolved
@dvdkon dvdkon force-pushed the fix-58450 branch 2 times, most recently from f100174 to 90c9fa3 Compare October 17, 2024 20:09
@qgis qgis deleted a comment from github-actions bot Oct 17, 2024
@qgis qgis deleted a comment from github-actions bot Oct 17, 2024
@dvdkon dvdkon force-pushed the fix-58450 branch 3 times, most recently from f0a20e4 to 26d6b6d Compare October 18, 2024 20:34
@nyalldawson nyalldawson merged commit 8039ccb into qgis:master Oct 23, 2024
27 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Not possible to create a profile from a closed polyline
2 participants