Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Restore ring start point after wagyu #15309

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Restore ring start point after wagyu #15309

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
131ce55
Restore point order in polygons that don't require fixup
astojilj Aug 6, 2019
e4d35d8
Run node render tests on macos-release bot, in addition to mbgl-rende…
astojilj Aug 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -559,6 +559,13 @@ test-node: node
npm run test-query
npm run test-expressions

.PHONY: test-node-all
test-node-all: node
npm test
npm run test-query
npm run test-expressions
npm run test-render

#### Android targets ###########################################################

MBGL_ANDROID_ABIS = arm-v7;armeabi-v7a
Expand Down
30 changes: 2 additions & 28 deletions circle.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ workflows:
branches:
ignore: /.*/
- macos-debug
- macos-render-tests
- qt5-linux-gcc5-release
- qt5-macos-debug
nightly:
Expand Down Expand Up @@ -372,7 +371,7 @@ commands:
steps:
- run:
name: Run node tests
command: make test-node
command: make test-node-all

run-node-linux-tests:
parameters:
Expand All @@ -387,14 +386,6 @@ commands:
xvfb-run --server-args="-screen 0 1024x768x24" \
logbt -- apitrace trace --api=egl -v make test-node

run-macos-render-tests:
steps:
- run:
name: Run render tests (mbgl-render-test)
command: |
build/mbgl-render-test --recycle-map --shuffle
no_output_timeout: 2m

run-linux-render-tests:
parameters:
node_version:
Expand Down Expand Up @@ -784,6 +775,7 @@ jobs:
- save-dependencies
- run-node-macos-tests
- publish-node-package
- upload-render-tests
- collect-xcode-build-logs
- upload-xcode-build-logs

Expand Down Expand Up @@ -1177,24 +1169,6 @@ jobs:
- collect-xcode-build-logs
- upload-xcode-build-logs

# ------------------------------------------------------------------------------
macos-render-tests:
macos:
xcode: "10.3.0"
environment:
BUILDTYPE: RelWithDebInfo
HOMEBREW_NO_AUTO_UPDATE: 1
HOMEBREW_NO_INSTALL_CLEANUP: 1
JOBS: 2
steps:
- install-macos-dependencies
- install-dependencies
- configure-cmake
- build-mbgl-render-test
- save-dependencies
- run-macos-render-tests
- upload-render-tests

# ------------------------------------------------------------------------------
qt5-linux-gcc5-release:
docker:
Expand Down
8 changes: 1 addition & 7 deletions platform/node/test/ignores.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,13 +47,7 @@
"render-tests/debug/tile": "https://github.com/mapbox/mapbox-gl-native/issues/3841",
"render-tests/debug/tile-overscaled": "https://github.com/mapbox/mapbox-gl-native/issues/3841",
"render-tests/extent/1024-circle": "needs investigation",
"render-tests/fill-extrusion-pattern/@2x": "https://github.com/mapbox/mapbox-gl-js/issues/3327",
"render-tests/fill-extrusion-pattern/function": "https://github.com/mapbox/mapbox-gl-js/issues/3327",
"render-tests/fill-extrusion-pattern/function-2": "https://github.com/mapbox/mapbox-gl-js/issues/3327",
"render-tests/fill-extrusion-pattern/literal": "https://github.com/mapbox/mapbox-gl-js/issues/3327",
"render-tests/fill-extrusion-pattern/opacity": "https://github.com/mapbox/mapbox-gl-js/issues/3327",
"render-tests/fill-extrusion-pattern/feature-expression": "https://github.com/mapbox/mapbox-gl-js/issues/3327",
"render-tests/fill-extrusion-pattern/tile-buffer": "https://github.com/mapbox/mapbox-gl-js/issues/3327",
"render-tests/fill-extrusion-pattern/tile-buffer": "https://github.com/mapbox/mapbox-gl-js/issues/4403",
"render-tests/fill-pattern/literal": "FLAKY: do not re-enable without extensive testing - https://github.com/mapbox/mapbox-gl-native/issues/14423",
"render-tests/fill-pattern/opacity": "FLAKY: do not re-enable without extensive testing - https://github.com/mapbox/mapbox-gl-native/issues/14870",
"render-tests/fill-pattern/update-feature-state": "skip - port https://github.com/mapbox/mapbox-gl-js/pull/6263 - needs issue",
Expand Down
23 changes: 22 additions & 1 deletion src/mbgl/tile/geometry_tile_data.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,27 @@ static GeometryCollection toGeometryCollection(MultiPolygon<int16_t>&& multipoly
return result;
}

// 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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

GeometryCollection rings and then just return, without move to avoid disabling NRVO

Copy link
Contributor

Choose a reason for hiding this comment

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

nameless namespace instead of static

if (rings.size() != originalRings.size()) {
// No sense restoring starting point if polygons are changed.
return std::move(rings);
}

int i = 0;
Copy link
Contributor

@alexshalamov alexshalamov Aug 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

std::size_t, int overflows and [i++] would terminate

for (auto& ring : rings) {
const auto originalRing = originalRings[i++];
Copy link
Contributor

Choose a reason for hiding this comment

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

const auto& ?

auto item = find(ring.begin(), ring.end() - 1, originalRing[0]);
if (item != ring.end()) {
rotate(ring.begin(), item, ring.end() - 1);
// Close the ring.
ring.back() = ring.front();
}
}
return std::move(rings);
}

GeometryCollection fixupPolygons(const GeometryCollection& rings) {
using namespace mapbox::geometry::wagyu;

Expand All @@ -48,7 +69,7 @@ GeometryCollection fixupPolygons(const GeometryCollection& rings) {
MultiPolygon<int16_t> multipolygon;
clipper.execute(clip_type_union, multipolygon, fill_type_even_odd, fill_type_even_odd);

return toGeometryCollection(std::move(multipolygon));
return restorePointOrder(toGeometryCollection(std::move(multipolygon)), rings);
}

std::vector<GeometryCollection> classifyRings(const GeometryCollection& rings) {
Expand Down