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

[OGC] Polygon: fix rings order #55306

Merged
merged 10 commits into from
Nov 25, 2023
Merged

Conversation

lbartoletti
Copy link
Member

@lbartoletti lbartoletti commented Nov 16, 2023

Description

I'm reaching out regarding an important initiative aimed at rectifying an anomaly observed in the digitization process within QGIS. Specifically, the adherence to OGC standards, upon which QGIS relies, dictates a distinct orientation requirement for polygon rings.

As per the OGC norm, the exterior boundary LinearRing must define the "top" of the surface, following a counterclockwise traversal along the boundary. Conversely, interior LinearRings should exhibit an opposite orientation, appearing clockwise when viewed from the "top."

Presently, there's a lack of verification within QGIS to ensure adherence to this prescribed behavior. To address this issue, I'll be initiating a series of PR; focusing only on the digitization process here. The primary goal is to align the behavior more closely with the established standards.

Nevertheless, I intend to maintain flexibility in the codebase, allowing for the preservation of the previous behavior programmatically. This approach aims to avoid directly enforcing orientation within the *Polygon classes. So, classes still accept "unordered" rings.

Before:

before-converted.mp4

After:

after-converted.mp4

PS: The motivation behind this is that certain tools perform stricter checks than the ones we currently conduct or those performed with GEOS.
PS2: I'm aware that ESRI Shapefile is CW unlike OGC, but should be a provider role to check this part?

TODO:

  • fix tests
  • add test

@lbartoletti lbartoletti added Requires Tests! Waiting on the submitter to add unit tests before eligible for merging Digitizing Related to feature digitizing map tools or functionality Needs Documentation When merging a labeled PR, an issue will be created in the Doc repo. labels Nov 16, 2023
@lbartoletti lbartoletti requested review from strk and rouault November 16, 2023 13:27
@lbartoletti lbartoletti self-assigned this Nov 16, 2023
@qgis-bot
Copy link
Collaborator

@lbartoletti
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!

@github-actions github-actions bot added this to the 3.36.0 milestone Nov 16, 2023
@rouault
Copy link
Contributor

rouault commented Nov 16, 2023

Slight side tracking: determining which side is the top and which one is the bottom depends on the convention of the editing tool. Your change assumes that the editor looks at polygons from the top, which is a reasonable one. But for a 3D shape and 3D editor, determining the top from the inside cannot be just done by looking at the coordinates of the vertex of the ring (for vertical polygons,, the usual method would return a "random" value). You need to take into account the whole shape to determine the top from the bottom.

The addition of the constraint "exterior boundary = counter-clock-wise" is something "new" in SFA 1.2.1 - 06-103r4 (https://www.ogc.org/standard/sfa/). It is not present in SFA 1.1.0 - 05-126, which might perhaps explain why GEOS doesn't take that into account.

@lbartoletti
Copy link
Member Author

Thanks @rouault for your insight.

Slight side tracking: determining which side is the top and which one is the bottom depends on the convention of the editing tool. Your change assumes that the editor looks at polygons from the top, which is a reasonable one.

Yes, what I should have written was that QGIS now allows you to mix the order of the interior rings; this is the most problematic aspect for me. Whether CW or CCW, all interior rings must have the same direction, which is not the case today.

But for a 3D shape and 3D editor, determining the top from the inside cannot be just done by looking at the coordinates of the vertex of the ring (for vertical polygons,, the usual method would return a "random" value). You need to take into account the whole shape to determine the top from the bottom.

Do you think, it's applicable for these tools? I suggest enforcing the digitalization of new geometry, but we can still apply CW or CCW with some algorithms/expressions in QGIS.

The addition of the constraint "exterior boundary = counter-clock-wise" is something "new" in SFA 1.2.1 - 06-103r4 (https://www.ogc.org/standard/sfa/). It is not present in SFA 1.1.0 - 05-126, which might perhaps explain why GEOS doesn't take that into account.

Yes, you're right. I was a baby geomatician when the new standard has landed. :) To be clear, I don't blame GEOS.
However, I don't see what's wrong with forcing digitization in a certain direction - that of the current standard - while being lax about what we receive/read. Does that sound right to you, or am I missing something?

@rouault
Copy link
Contributor

rouault commented Nov 17, 2023

Does that sound right to you, or am I missing something?

What you propose is probably OK and I don't strongly object, but I'd say the tools you're trying to be compatible with should only enforce the OGC rules when displaying/working with 3D volumes, not 2D surfaces where the concept of top/outside or bottom/inside doesn't really make sense. But if we are trying to be fully compatible with those tools (can be they named :-) ?), that's a lot of effort because they're plenty of places in QGIS, GDAL or GEOS where changes would have to be made.

@lbartoletti
Copy link
Member Author

Does that sound right to you, or am I missing something?

What you propose is probably OK and I don't strongly object, but I'd say the tools you're trying to be compatible with should only enforce the OGC rules when displaying/working with 3D volumes, not 2D surfaces where the concept of top/outside or bottom/inside doesn't really make sense. But if we are trying to be fully compatible with those tools (can be they named :-) ?), that's a lot of effort because they're plenty of places in QGIS, GDAL or GEOS where changes would have to be made.

At the very least, the case that poses a real issue for me is the potential disorder when digitizing the rings and parts.
One of the tools, SFCGAL, is very strict about this, and this also applies to other proprietary tools whose names I might not necessarily know (mining activities in mind). There's an expectation, which seems logical to me, of having the exterior rings in one direction and the interior rings in the opposite direction.
Even if the new standard mentions CCW, I could ignore it. However, I would like it so that if the exterior ring is in one direction, all interior rings are in the opposite direction, and the parts follow the direction of the exterior ring.

@rouault
Copy link
Contributor

rouault commented Nov 20, 2023

However, I would like it so that if the exterior ring is in one direction, all interior rings are in the opposite direction, and the parts follow the direction of the exterior ring.

that indeed makes sense

@lbartoletti
Copy link
Member Author

However, I would like it so that if the exterior ring is in one direction, all interior rings are in the opposite direction, and the parts follow the direction of the exterior ring.

that indeed makes sense

I've made the changes so that the parts are in the same direction as the first exterior ring and the reverse for the interior rings, and I'm not forcing the direction of the main exterior ring.

src/core/geometry/qgsgeometry.cpp Outdated Show resolved Hide resolved
src/core/geometry/qgsgeometry.cpp Outdated Show resolved Hide resolved
src/core/geometry/qgsgeometry.cpp Show resolved Hide resolved
src/core/geometry/qgsgeometry.h Show resolved Hide resolved
@lbartoletti lbartoletti removed the Requires Tests! Waiting on the submitter to add unit tests before eligible for merging label Nov 25, 2023
@lbartoletti lbartoletti merged commit 5ac90ce into qgis:master Nov 25, 2023
30 checks passed
@lbartoletti lbartoletti deleted the fix_polygon_addring branch November 25, 2023 04:45
Copy link

@lbartoletti
A documentation ticket has been opened at qgis/QGIS-Documentation#8658
It is your responsibility to visit this ticket and add as much detail as possible for the documentation team to correctly document this change.
Thank you!

@agiudiceandrea
Copy link
Contributor

Hi @lbartoletti, please see #58333.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Digitizing Related to feature digitizing map tools or functionality Needs Documentation When merging a labeled PR, an issue will be created in the Doc repo.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants