This repository was archived by the owner on Aug 8, 2023. It is now read-only.
Restore ring start point after wagyu #15309
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
astojilj
force-pushed
the
astojilj-15286
branch
from
2d93e32 to
3ea378d
Compare
astojilj
added
Core
wagyu algorithm setup and checking if polygon fixup is required is expensive as much as fixing it - so we continue running wagyu for all geojson and, if attempt to restore original point order after wagyu. Even with no polygon fixup, starting point in rings is changed. This causes a disparity between GL-JS and GL-native, visible in implementation of fill-extrusion pattern. Fixes: #15268
astojilj
force-pushed
the
astojilj-15286
branch
4 times, most recently
from
b884a94 to
0690b1b
Compare
…r-tests run on linux
astojilj
force-pushed
the
astojilj-15286
branch
from
0690b1b to
e4d35d8
Compare
alexshalamov
reviewed
Aug 6, 2019
| return std::move(rings); | ||
| } | ||
|
|
||
| int i = 0; |
Contributor
There was a problem hiding this comment.
std::size_t, int overflows and [i++] would terminate
alexshalamov
reviewed
Aug 6, 2019
|
|
||
| // Attempts to restore original point order in rings: | ||
| // see https://github.com/mapbox/mapbox-gl-native/issues/15268. | ||
| static GeometryCollection restorePointOrder(GeometryCollection&& rings, const GeometryCollection& originalRings) { |
Contributor
There was a problem hiding this comment.
GeometryCollection rings and then just return, without move to avoid disabling NRVO
pozdnyakov
reviewed
Aug 6, 2019
|
|
||
| // Attempts to restore original point order in rings: | ||
| // see https://github.com/mapbox/mapbox-gl-native/issues/15268. | ||
| static GeometryCollection restorePointOrder(GeometryCollection&& rings, const GeometryCollection& originalRings) { |
Contributor
There was a problem hiding this comment.
nameless namespace instead of static
pozdnyakov
reviewed
Aug 6, 2019
|
|
||
| int i = 0; | ||
| for (auto& ring : rings) { | ||
| const auto originalRing = originalRings[i++]; |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Restore point order in polygons after polygon fixup.
Algorithm setup and checking if polygon fixup is required would be as expensive as fixing it. So, we don't introduce another step checking if fixup is needed but continue running wagyu for all geojson and, make an attempt to restore original point order in rings, after wagyu. Having a change in starting point of rings caused a disparity between GL-JS and GL-native, visible in implementation of fill-extrusion pattern.
Fixes: #15268