-
Notifications
You must be signed in to change notification settings - Fork 25.5k
Fix mvt polygon orientation #86555
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 mvt polygon orientation #86555
Conversation
Pinging @elastic/es-analytics-geo (Team:Analytics) |
Hi @iverase, I've created a changelog YAML for you. |
Hi @iverase, I've updated the changelog YAML for you. |
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.
I think it looks good to me, but I also feel like I do not know enough of the inner workings of the MVT encoding to completely follow everything.
// Check CCW Winding (must be positive area in original coordinate system, MVT is positive-y-down, | ||
// so inequality is flipped) | ||
// See: https://docs.mapbox.com/vector-tiles/specification/#winding-order | ||
if (exteriorArea > 0d) { |
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.
I presume this is the change
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.
yes, that is the orientation for the outer ring
// Check CW Winding (must be negative area in original coordinate system, MVT is positive-y-down, | ||
// so inequality is flipped) | ||
// See: https://docs.mapbox.com/vector-tiles/specification/#winding-order | ||
if (interiorArea < 0d) { |
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.
And another one
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.
yes, that is the orientation for inner rings (holes)
assertThat(feature.getGeometryCount(), equalTo(22)); | ||
{ | ||
// outer ring | ||
double[] xs = new double[5]; |
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.
Understanding these tests requires inner knowledge of the MVT tile encoding, which I do not (yet) have.
…ch into fixMVTPolOrientation
This commit forks the method we use in JTSAdapter to generate the vector tile feature containing the right logic for polygon orientation.
💔 Backport failed
You can use sqren/backport to manually backport by running |
This commit forks the method we use in JTSAdapter to generate the vector tile feature containing the right logic for polygon orientation. # Conflicts: # x-pack/plugin/vector-tile/src/test/java/org/elasticsearch/xpack/vectortile/feature/FeatureFactoryTests.java
* Fix mvt polygon orientation (#86555) This commit forks the method we use in JTSAdapter to generate the vector tile feature containing the right logic for polygon orientation. # Conflicts: # x-pack/plugin/vector-tile/src/test/java/org/elasticsearch/xpack/vectortile/feature/FeatureFactoryTests.java * compile error
The current library we use to encode mvt geometries into vector tiles features handle incorrectly the orientation of the polygons. The main branch of the project contains a fix to the issue but it has not been released.
This PR forks the method we use in JTSAdapter to generate the vector tile feature containing the fix for polygon orientation.
fixes #86560