-
-
Notifications
You must be signed in to change notification settings - Fork 805
Update linestopolygons algorithm description #9784
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
base: master
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | |||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -3849,12 +3849,232 @@ Python code | ||||||||||||||||||||||||||||||||||||||||||||
|
|||||||||||||||||||||||||||||||||||||||||||||
Lines to polygons | |||||||||||||||||||||||||||||||||||||||||||||
----------------- | |||||||||||||||||||||||||||||||||||||||||||||
Generates a polygon layer using as polygon rings the lines from an | |||||||||||||||||||||||||||||||||||||||||||||
input line layer. | |||||||||||||||||||||||||||||||||||||||||||||
Generates a polygon layer from line layer, considering LineString geometry with three | |||||||||||||||||||||||||||||||||||||||||||||
or more vertices as polygon rings. Input LineString geometry doesn't need to close a loop, | |||||||||||||||||||||||||||||||||||||||||||||
algorithm will automatically connect last to first point when forming a polygon. | |||||||||||||||||||||||||||||||||||||||||||||
Result is always promoted to MultiPolygon. | |||||||||||||||||||||||||||||||||||||||||||||
LineString geometry that have less than three vertices will produce | |||||||||||||||||||||||||||||||||||||||||||||
|
|||||||||||||||||||||||||||||||||||||||||||||
new polygon features with **EMPTY** MultiPolygon geometry, attributes are kept. | |||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Code show us a MultiPolygon, but there is no need to repeat that information twice in the same description? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi Selma, this two sentences describe different things. Let me try to explain: Row 3855 explains that you will never get Polygon geometry as a result - it will always be MultiPolygon even when not needed - this is a decision made by developer and should be documented. Many results could be simple Polygons, but result is always promoted to MultiPolygon because then we would need two layers on output - Polygon layer and MultiPolygon layer for specific cases (see table below for difference between Polygon and MultiPolygon for the same geometry shape - they are pretty different in WKB (in database or on hard disk, while on screen they are identical). Although to casual QGIS user doesn't mean much - it is still different geometry type, just like Point and Polygon are different just on a different level. This sentence is very important because if user wants Polygons in final output he must additionally run Row 3857 explains that result is EMPTY geometry - but technically there is no such thing as "EMPTY geometry", you can only have data-types (literally
So, because of technical reasons I would like to keep name P.S. I've been personally through these problems before - trying to parsing binary geometry in PHP (I didn't have any good library to parse this for me) - so - in this type of cases it is very useful to know whether should you expect |
|||||||||||||||||||||||||||||||||||||||||||||
|
|||||||||||||||||||||||||||||||||||||||||||||
The attribute table of the output layer is the same as the one of | |||||||||||||||||||||||||||||||||||||||||||||
the input layer. | |||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
3859
to
3860
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
And then I wonder if we need to repeat it for every example below There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will add this new sentence to new PR, but I would like to keep it repeated for every example. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don’t have a strong opinion on this, but I’m concerned that if we adopt this kind of repetition for every example in every algorithm (we will probably add more examples in the future) it could lead to a lot of unnecessary text in the documentation?
|
|||||||||||||||||||||||||||||||||||||||||||||
|
|||||||||||||||||||||||||||||||||||||||||||||
Examples | |||||||||||||||||||||||||||||||||||||||||||||
........ | |||||||||||||||||||||||||||||||||||||||||||||
|
|||||||||||||||||||||||||||||||||||||||||||||
Lines to polygons on linestrings | |||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a note: I think the titles “Lines to polygons on lines” vs “...on linestrings” could be a bit confusing, especially for users who aren’t deeply familiar with GIS geometry types (as you mentioned someone with a CAD background). Since both examples use LineString inputs, maybe we could rephrase the titles to reflect the real difference, like “with two points” vs “with three or more points” or “simple vs complex LineStrings”? |
|||||||||||||||||||||||||||||||||||||||||||||
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | |||||||||||||||||||||||||||||||||||||||||||||
|
|||||||||||||||||||||||||||||||||||||||||||||
Features with 3 or more vertices are expected on input: **start point - vertex/vertices - end point** | |||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+3866
to
+3869
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The first example contradicts this statement. And as I suggest later, I'd just merge concept of single and multiline segments together
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I am aware that first example is opposite of description, but as noted in previous comment it's for people who will immediately jump to examples. My logic is to prove the opposite - just like in math - this is an example how algorithm works - and it is only example that show that in this case algorithm will have an output visible in attribute table only, with EMPTY geometry so it does fit in this table since output does exist it is not rejected. I didn't merge examples from single segment lines and multi segment lines because the first table explains how algorithm works and it's much more important, and second table is like an appendix for users that are coming from CAD background and finding name of algorithm - "lines to polygons" confusing. The second table could be dismissed at all, but I would like to keep it separate because I find it important for understanding how algorithm works in these cases. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ADDITIONAL NOTES:
So both of these examples are intended for better user experience and not directly for describing algorithm so I moved them to another table/section. |
|||||||||||||||||||||||||||||||||||||||||||||
|
|||||||||||||||||||||||||||||||||||||||||||||
.. list-table:: | |||||||||||||||||||||||||||||||||||||||||||||
:header-rows: 1 | |||||||||||||||||||||||||||||||||||||||||||||
:widths: 30 30 40 | |||||||||||||||||||||||||||||||||||||||||||||
phidrho marked this conversation as resolved.
Show resolved
Hide resolved
|
|||||||||||||||||||||||||||||||||||||||||||||
|
|||||||||||||||||||||||||||||||||||||||||||||
* - INPUT | |||||||||||||||||||||||||||||||||||||||||||||
- OUTPUT | |||||||||||||||||||||||||||||||||||||||||||||
- NOTE | |||||||||||||||||||||||||||||||||||||||||||||
|
|||||||||||||||||||||||||||||||||||||||||||||
* - **Feature a**: Single line LineString | |||||||||||||||||||||||||||||||||||||||||||||
phidrho marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
|||||||||||||||||||||||||||||||||||||||||||||
|
|||||||||||||||||||||||||||||||||||||||||||||
.. figure:: img/lines_to_polygons_from_linestr_input_1.png | |||||||||||||||||||||||||||||||||||||||||||||
:width: 25 em | |||||||||||||||||||||||||||||||||||||||||||||
:align: center | |||||||||||||||||||||||||||||||||||||||||||||
|
|||||||||||||||||||||||||||||||||||||||||||||
LineString with two vertices | |||||||||||||||||||||||||||||||||||||||||||||
|
|||||||||||||||||||||||||||||||||||||||||||||
- **Feature a**: MultiPolygon EMPTY | |||||||||||||||||||||||||||||||||||||||||||||
|
|||||||||||||||||||||||||||||||||||||||||||||
.. figure:: img/lines_to_polygons_from_linestr_output_1.png | |||||||||||||||||||||||||||||||||||||||||||||
:width: 25 em | |||||||||||||||||||||||||||||||||||||||||||||
:align: center | |||||||||||||||||||||||||||||||||||||||||||||
|
|||||||||||||||||||||||||||||||||||||||||||||
Output doesn't contain geometry | |||||||||||||||||||||||||||||||||||||||||||||
|
|||||||||||||||||||||||||||||||||||||||||||||
- | Algorithm creates one new feature with empty geometry, but with all attributes from source feature. | |||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what does the | character stand for? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
|||||||||||||||||||||||||||||||||||||||||||||
| | |||||||||||||||||||||||||||||||||||||||||||||
| **Output has a valid (empty) geometry.** | |||||||||||||||||||||||||||||||||||||||||||||
|
|||||||||||||||||||||||||||||||||||||||||||||
* - **Feature b**: LineString with three vertices | |||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does the number of vertice/segments matters? It is more the situation of a not closed linestring with more than one segment, no? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Linestring doesn't need to be closed. Algorithm always assume vertices are closing a loop - that's why I included example as open linestring with three vertices. I have also tried some other examples when all three vertices lie on same line but these are special cases, and I think that I decided to discard these examples. |
|||||||||||||||||||||||||||||||||||||||||||||
|
|||||||||||||||||||||||||||||||||||||||||||||
.. figure:: img/lines_to_polygons_from_linestr_input_2.png | |||||||||||||||||||||||||||||||||||||||||||||
:width: 25 em | |||||||||||||||||||||||||||||||||||||||||||||
:align: center | |||||||||||||||||||||||||||||||||||||||||||||
|
|||||||||||||||||||||||||||||||||||||||||||||
Open LineString with three vertices | |||||||||||||||||||||||||||||||||||||||||||||
|
|||||||||||||||||||||||||||||||||||||||||||||
- **Feature b**: MultiPolygon | |||||||||||||||||||||||||||||||||||||||||||||
|
|||||||||||||||||||||||||||||||||||||||||||||
.. figure:: img/lines_to_polygons_from_linestr_output_2.png | |||||||||||||||||||||||||||||||||||||||||||||
:width: 25 em | |||||||||||||||||||||||||||||||||||||||||||||
:align: center | |||||||||||||||||||||||||||||||||||||||||||||
|
|||||||||||||||||||||||||||||||||||||||||||||
Output is a triangle shaped MultiPolygon | |||||||||||||||||||||||||||||||||||||||||||||
|
|||||||||||||||||||||||||||||||||||||||||||||
- | Algorithm creates one new feature as MultiPolygon with one part, with all attributes from source feature. | |||||||||||||||||||||||||||||||||||||||||||||
| | |||||||||||||||||||||||||||||||||||||||||||||
| **Output has a valid geometry.** | |||||||||||||||||||||||||||||||||||||||||||||
|
|||||||||||||||||||||||||||||||||||||||||||||
* - **Feature c**: LineString with four vertices | |||||||||||||||||||||||||||||||||||||||||||||
|
|||||||||||||||||||||||||||||||||||||||||||||
.. figure:: img/lines_to_polygons_from_linestr_input_3.png | |||||||||||||||||||||||||||||||||||||||||||||
:width: 25 em | |||||||||||||||||||||||||||||||||||||||||||||
:align: center | |||||||||||||||||||||||||||||||||||||||||||||
|
|||||||||||||||||||||||||||||||||||||||||||||
Closed loop LineString with four vertices | |||||||||||||||||||||||||||||||||||||||||||||
|
Closed loop LineString with four vertices | |
Closed loop LineString |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, thanks, I have set your version for new PR.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it would not necessarily be quadrilateral then
Output is a quadrilateral shaped MultiPolygon | |
Output is a shaped MultiPolygon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I have set your version for new PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.. warning:: **Output geometry is invalid!** | |
MultiPolygon parts should not intersect. | |
.. warning:: Output geometry is invalid, as MultiPolygon parts should not intersect. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
phidrho marked this conversation as resolved.
Show resolved
Hide resolved
phidrho marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(here and below) Are LineString and MultiPolygon (written with that casing) pluralizable? How about
Two distinct LineStrings, one geometrically contained inside another | |
Two distinct line features, one geometrically contained inside another |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this render in HTML?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO this subsection is unnecessary. It repeats the example a. We just need to clearly state that the alg processes line by line, one feature at a time.
Or we could reuse the first example (next to a) to show that connected single segment features behave same as one segment feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see my comment on first example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These lines are part of the alg description and should be kept out of the example section. To move above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see my comment on first example.
Uh oh!
There was an error while loading. Please reload this page.