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.

Sign up for GitHub

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

Jump to bottom

[elevation profile] add tolerance for lines and polyons #54964

Merged
merged 26 commits into from
Mar 13, 2024
Merged

[elevation profile] add tolerance for lines and polyons #54964

merged 26 commits into from
Mar 13, 2024

Conversation

benoitdm-oslandia
Copy link
Contributor

@benoitdm-oslandia benoitdm-oslandia commented Oct 17, 2023 •
edited
Loading

Improve elevation profile tool by adding tolerance support for lines and polygons (2D/3D). This allows to create, according to the elevation curve, a 2D view version of a 3D scene slice.
Without tolerance support, a line or a polygon is fully displayed in the elevation profile when it intersects the profile curve. By adding support for Tolerance, only the parts of the lines and polygon which intersect the gray area are taken into account to compute the elevation profile.

Here is a screencast of a sample project with ours fixes. The more the tolerance increases the more we capture new objects or parts of objects (as lines or poylgons). We can see the captured polygons growing:
elevation_profile_improved_trimmed

Here is another screencast with drillhole representations (from OpenLog plugin):

screencast_forage_openlog

Nota bene: extruding closed polygons is not trivial because it is equivalent to representing a 3D object (like a 3D closed polygon reprojected into a 2D closed polygon in the elevation profile tool + the extrusion dimension). We are still working to find a good solution but right now the polygon extrusion is not showing when tolerance is enabled.

Funded by CEA/DAM @renardf

Thanks to @jmkerloch @ptitjano @lbartoletti @troopa81

OLD PR description:

This PR adds the support to tolerance for lines and polygons by using a buffer.

  • some lambda functions have been moved to member functions to increase the readability
  • add some tests with tolerance
  • some minor fixes and refactor

Here is a screencast of a sample project from the master:
elevation_profile_master_trimmed

and the same version with ours fixes (more zoomed in the elevation profile tool to show the captured lines):
elevation_profile_improved_trimmed

Extruding closed polygons is not trivial because it is equivalent to representing a 3D object (like a 3D closed polygon reprojected into a 2D closed polygon in the elevation profile tool + the extrusion dimension). We are still working to find a good solution but right now the polygon extrusion is not showing when tolerance is enabled.

cc @ptitjano @lbartoletti @jmkerloch @troopa81

Funded by CEA/DAM @renardf

@benoitdm-oslandia benoitdm-oslandia self-assigned this Oct 17, 2023
@github-actions github-actions bot added this to the 3.34.0 milestone Oct 17, 2023
@benoitdm-oslandia benoitdm-oslandia changed the title Gh/feature/vector profile line tolerance [elevation profile] add tolerance for lines and polyons Oct 18, 2023
@benoitdm-oslandia benoitdm-oslandia force-pushed the gh/feature/vector_profile_line_tolerance branch from 49b1c3c to 3704fb4 Compare October 18, 2023 08:15
@fiddlersfan
Copy link
Contributor

fiddlersfan commented Oct 18, 2023 •
edited
Loading

I have tested it locally, this unfortunately generates an incorrect display for raster data with NaN values. Its similar to #54422 but not exactly the same. The gap that should be created by the NaN values will not be filled in the "Fill" modes nor in the "Line" mode.

Maybe this screenshot helps in compare with #54642 it is approximately the same profile
image

@benoitdm-oslandia
Copy link
Contributor Author

I have tested it locally, this unfortunately generates an incorrect display for raster data with NaN values. Its similar to #54422 but not exactly the same. The gap that should be created by the NaN values will not be filled in the "Fill" modes nor in the "Line" mode.

Thanks! I will add a test and fix this regression.

@benoitdm-oslandia benoitdm-oslandia force-pushed the gh/feature/vector_profile_line_tolerance branch from 3704fb4 to eac6490 Compare October 20, 2023 13:07
@benoitdm-oslandia
Copy link
Contributor Author

@fiddlersfan I add a test and do some fixes

@nyalldawson can you have a look? Thanks

@benoitdm-oslandia benoitdm-oslandia force-pushed the gh/feature/vector_profile_line_tolerance branch from eac6490 to 7bce217 Compare October 20, 2023 13:17
@nyalldawson nyalldawson added the Frozen Feature freeze - Do not merge! label Oct 22, 2023
@fiddlersfan
Copy link
Contributor

@fiddlersfan I add a test and do some fixes

Thanks for the test and fix! I just tested it again and the regression is fixed.

@benoitdm-oslandia benoitdm-oslandia force-pushed the gh/feature/vector_profile_line_tolerance branch 2 times, most recently from 852c881 to b6791d9 Compare November 2, 2023 17:04
@benoitdm-oslandia benoitdm-oslandia removed the Frozen Feature freeze - Do not merge! label Nov 7, 2023
@benoitdm-oslandia benoitdm-oslandia force-pushed the gh/feature/vector_profile_line_tolerance branch 2 times, most recently from ef2f2ac to 31cdbd2 Compare November 8, 2023 07:15
@benoitdm-oslandia
Copy link
Contributor Author

@nyalldawson at last, all checks are green! Can you review this PR please?

Regards.

@nyalldawson
Copy link
Collaborator

This one will take some time for me to review -- there's a lot here!

@benoitdm-oslandia
Copy link
Contributor Author

@nyalldawson indeed!

@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 2, 2023

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 github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Dec 2, 2023
@benoitdm-oslandia benoitdm-oslandia force-pushed the gh/feature/vector_profile_line_tolerance branch from 31cdbd2 to 4b0334c Compare December 5, 2023 10:54
@github-actions github-actions bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Dec 5, 2023
@benoitdm-oslandia
Copy link
Contributor Author

rebase from upstream/master

@benoitdm-oslandia benoitdm-oslandia force-pushed the gh/feature/vector_profile_line_tolerance branch from 4b0334c to c8ade4a Compare December 5, 2023 14:36
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 5, 2023 •
edited
Loading

🪟 Windows builds ready!

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

(Built from commit 83a7a50)

@github-actions GitHub Actions
Copy link

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.

@benoitdm-oslandia benoitdm-oslandia force-pushed the gh/feature/vector_profile_line_tolerance branch from b3e2758 to a94bbdc Compare March 12, 2024 14:07
@benoitdm-oslandia
Copy link
Contributor Author

rebased ! @nyalldawson can you have a look please ?

@nyalldawson
Copy link
Collaborator

@benoitdm-oslandia LGTM! Just the test failure to fix now -- looks like it just needs a mask update for qt 6 (see #54964 (comment))

@benoitdm-oslandia benoitdm-oslandia force-pushed the gh/feature/vector_profile_line_tolerance branch from a94bbdc to f0bd1ac Compare March 13, 2024 07:46
@benoitdm-oslandia
fix(elevation_profile): add fix when intersecting polygons vs wall.
5b123f8 
In case the geometry is invalid, we shift all coordinates by an epsilon to ensure the 2D equivalent will be topologically correct.

This is just a fix, a proper 3D geometry library is needed (like SFCGAL)!
@benoitdm-oslandia
fix(elevation_profile): adjust tolerance tests for geos 3.12 and add …
83a7a50 
…mask for vector_layer_map_units_tolerance
@benoitdm-oslandia benoitdm-oslandia force-pushed the gh/feature/vector_profile_line_tolerance branch from f0bd1ac to 83a7a50 Compare March 13, 2024 07:56
@benoitdm-oslandia
Copy link
Contributor Author

fix test by adding a mask and add comments

@benoitdm-oslandia
Copy link
Contributor Author

@nyalldawson all green! Can you merge?

@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 and removed Freeze Exempt Feature Freeze exemption granted labels Mar 13, 2024
@nyalldawson nyalldawson merged commit 81573d1 into qgis:master Mar 13, 2024
36 checks passed
@qgis-bot
Copy link
Collaborator

@benoitdm-oslandia

This pull request has been tagged for the changelog.

  • The description will be harvested so please provide a "nearly-ready" text for the final changelog
  • If possible, add a nice illustration of the feature. Only the first one in the description will be harvested (GIF accepted as well)
  • If you can, it's better to give credits to your sponsor, see below for different formats.

You can edit the description.

Format available for credits
  • Funded by NAME
  • Funded by URL
  • Funded by NAME URL
  • Sponsored by NAME
  • Sponsored by URL
  • Sponsored by NAME URL

Thank you!

@qgis-bot
Copy link
Collaborator

@benoitdm-oslandia
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!

@qgis-bot qgis-bot mentioned this pull request Mar 13, 2024
Closed
@benoitdm-oslandia
Copy link
Contributor Author

@nyalldawson thanks for your time!

@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 Jun 18, 2024
@benoitdm-oslandia benoitdm-oslandia deleted the gh/feature/vector_profile_line_tolerance branch July 2, 2024 14:08
@nicogodet nicogodet mentioned this pull request Aug 19, 2024
Open
2 tasks
@lbartoletti lbartoletti mentioned this pull request Oct 17, 2024
Closed
@nicogodet nicogodet mentioned this pull request Dec 11, 2024
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nyalldawson nyalldawson nyalldawson approved these changes

@lbartoletti lbartoletti Awaiting requested review from lbartoletti

Assignees

@benoitdm-oslandia benoitdm-oslandia

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. Profile tool
Projects
None yet
Milestone
3.34.0
Development

Successfully merging this pull request may close these issues.
8 participants
@benoitdm-oslandia @fiddlersfan @nyalldawson @Asnaia @qgis-bot @lbartoletti @zacharlie @jmkerloch