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

Tolerance for lines and polygons - elevation profile #9385

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

selmaVH1
Copy link
Collaborator

fixes #8971

@selmaVH1
Copy link
Collaborator Author

Hi @benoitdm-oslandia
Thank you for the provided descriptions. I placed the Limitations part as a note instead of creating a new section, I believe it fits more logically in that format - is that ok?

@selmaVH1 selmaVH1 added the backport release_3.34 On merge create a backported pull request to 3.34 label Nov 15, 2024
@DelazJ DelazJ added backport release_3.40 On merge create a backported pull request to 3.40 Profile Tool and removed backport release_3.34 On merge create a backported pull request to 3.34 labels Nov 16, 2024
Copy link
Collaborator

@DelazJ DelazJ left a comment

Choose a reason for hiding this comment

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

I have questions. My feedback is fully based on my understanding of the text, I didn't check the actual behavior in QGIS...

@@ -113,7 +113,8 @@ At the top of the :guilabel:`Elevation Profile` panel, a toolbar provides you wi
- Allows to render distances in the profile chart with units other than the map canvas units.
* - :menuselection:`--> Tolerance`
-
- Sets how far from the actual profile line a point can reside within to be included in the results.
- Sets how far from the actual profile line a feature (point, line, polygon, etc.)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does "etc." refer to? Here and later

Copy link
Contributor

Choose a reason for hiding this comment

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

any geometrical object in the future. For now: point, line, polygon, pointcloud. I am not really sure for meshes, but support will be added for polyhedralsurfaces.


.. note:: **Limitations - Line and polygon extrusion**
Copy link
Collaborator

Choose a reason for hiding this comment

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

@benoitdm-oslandia thanks for the notes. There are a few unclear points to me:

  • "This works fine for points or lines in any case." : what is "this"? The extrusion or its combination with tolerance? Does "in any case" not include 3D lines?
  • "when tolerance is enabled and for 3D lines or 3D polygons, extrusion is not trivial": does 3D refer to layers with z component or is it because of the extrusion output?
  • Then if both 3D(or extruded?) line and polygon geometries are hard to render, why only "Thus, right now, polygon extrusion is disabled when tolerance is enabled."?
  • and trying to get the workflow: a user enables extrusion in elevation properties on a (3D?) polygon layer. Is this option only for the elevation profile purpose? Not for 3D map view? Then he opens a profile view, sets a tolerance, and his extrusion checkbox in the layer properties gets disabled, unknowingly? Not really user friendly IMHO. We should rather somehow ignore the extrusion setting, not disable (meaning to me, uncheck) the user selection. What about his potential 3D map view?

Back to the note, I think we could hide the internal development explanations and simply mention incompatibilities between/disabling/ignoring of polygon extrusion when tolerance is on. And should it be here or in the layer elevations properties, next to the extrusion option?

Copy link
Contributor

Choose a reason for hiding this comment

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

@benoitdm-oslandia thanks for the notes. There are a few unclear points to me:

* "_This works fine for points or lines in any case._" : what is "this"? The extrusion or its combination with tolerance? Does "in any case" not include 3D lines?

extrusion works fine with points or lines with/without tolerance.

* "_when tolerance is enabled and for 3D lines or 3D polygons, extrusion is not trivial_": does 3D refer to layers with z component or is it because of the extrusion output?

it is about the extrusion output: it becomes an 3D volume we have to reproject in the 2D plane of the profile elevation canvas. This introduces new challenges in object representation as we want to have a fast rendering. Also the current code is not easily understandable nor easily optimizable, this kind of new feature may kill this tool.

* Then if both 3D(or extruded?) line and polygon geometries are hard to render, why only "_Thus, right now, polygon extrusion is disabled when tolerance is enabled._"?

This choice allows to display the intersected objects without having rendering issues.

* and trying to get the workflow: a user enables extrusion in elevation properties on a (3D?) polygon layer. Is this option only for the elevation profile purpose? Not for 3D map view? Then he opens a profile view, sets a tolerance, and his extrusion checkbox in the layer properties gets disabled, unknowingly? Not really user friendly IMHO. We should rather somehow ignore the extrusion setting, not disable (meaning to me, uncheck) the user selection. What about his potential 3D map view?

The checkbox is just ignored (not disabled) when this condition is met: 3D polygon+extrusion+tolerance. The extrusion feature is temporally disabled for this layer.

Back to the note, I think we could hide the internal development explanations and simply mention incompatibilities between/disabling/ignoring of polygon extrusion when tolerance is on. And should it be here or in the layer elevations properties, next to the extrusion option?

I think issue explanation is a good thing for the user as he can understand he asks for something not trivial and it is not a new issue to report.
Also having the explanation in layer elevations properties UI, is a good idea but hard to maintain: we will forget to change it when the code will be fixed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks @benoitdm-oslandia for the feedback

  • Then if both 3D(or extruded?) line and polygon geometries are hard to render, why only "Thus, right now, polygon extrusion is disabled when tolerance is enabled."?

This choice allows to display the intersected objects without having rendering issues.

My point was more about "extruded" line. if both line and polygon geometries are hard to render but we finally disable only polygon, I guess it is because we solve hardness of lines and don't need to mention it in docs anymore. As you replied:

extrusion works fine with points or lines with/without tolerance.

And the title of the note should be adjusted.
Well, trying a rewording to clarify what I meant by "hiding explanations", and taking into account your reply. Hopefully I understand clearly and catch all the key points:

Geometry extrusion can be set in the |elevationscale| :guilabel:Elevation properties of a layer,
and rendered in the profile view. <-- I don't know if rendered is the more accurate word here
When tolerance is enabled, it is however not trivial to render extruded polygons,
thus, right now, polygon extrusion is ignored when tolerance is enabled.
Only the overlap with the 2D geometry of the polygon feature is taken into account.

I added a last line as I understand that it is the extrusion that is ignored, not the polygon which I think is somehow taken into account.

Also having the explanation in layer elevations properties UI, is a good idea but hard to maintain: we will forget to change it when the code will be fixed.

OK. Given that the extrusion is ignored (and not disabled aka unchecked in the elevation properties), this is a tolerance "feature", so we should mention it only in the profile docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport release_3.40 On merge create a backported pull request to 3.40 Profile Tool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[elevation profile] add tolerance for lines and polyons (Request in QGIS)
3 participants