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

[core] Enable circle-sort-key property #15875

Merged
merged 5 commits into from
Apr 14, 2020
Merged

[core] Enable circle-sort-key property #15875

merged 5 commits into from
Apr 14, 2020

Conversation

ahk
Copy link
Contributor

@ahk ahk commented Oct 31, 2019 •
edited by pozdnyakov
Loading

Adds support for circle-sort-key property, consistent with symbol-sort-key.

Sorts drawing order by sort key both within-tile and cross-tile.

Fixes: #15008

@chloekraw chloekraw added the needs changelog Indicates PR needs a changelog entry prior to merging. label Nov 21, 2019
@ahk ahk requested a review from pozdnyakov December 5, 2019 00:49
@ahk ahk force-pushed the port-fill-line-sort-key branch 2 times, most recently from 01e7367 to 190dafc Compare December 5, 2019 06:03
@alexshalamov alexshalamov force-pushed the port-fill-line-sort-key branch 2 times, most recently from c640bab to 9cf1846 Compare December 10, 2019 16:39
@chloekraw chloekraw mentioned this pull request Dec 13, 2019
Closed
@alexshalamov alexshalamov force-pushed the port-fill-line-sort-key branch 6 times, most recently from ae7ea2b to 1f266b4 Compare December 17, 2019 11:37
@chloekraw chloekraw added this to the release-unicorn milestone Dec 18, 2019
@chloekraw
Copy link
Contributor

@ahk #15839 has landed -- should we rebase this pr against master and then get it reviewed?

@pozdnyakov
Copy link
Contributor

@ahk the approach looks nice! 👍 I'll be able to give more thorough review after the rebase and once CI is passing (pls tell if I could help with that).

@ahk ahk changed the base branch from port-fill-line-sort-key to master January 3, 2020 22:43
@ahk
Copy link
Contributor Author

ahk commented Jan 9, 2020 •
edited
Loading

once CI is passing (pls tell if I could help with that).

Hey @pozdnyakov, rebase was no problem but I've spent a bit of time (unsuccessfully) trying to figure out why my PR doesn't pass CI. Anything stand out to you?

I know the code formatting check is also failing but really the troubling thing is the segfault in the test runner on for example next-linux-clang8-release. It happens during mbgl::gl::Program<mbgl::CircleProgram>::draw() when attempting mbgl::gl::value::VertexAttribute::Set.

@asheemmamoowala asheemmamoowala added the GL JS parity For feature parity with Mapbox GL JS label Feb 3, 2020
1ec5
1ec5 reviewed Feb 25, 2020
View reviewed changes
scripts/generate-style-code.js
@@ -6,9 +6,6 @@ const ejs = require('ejs');
const spec = require('./style-spec');
const colorParser = require('csscolorparser');

// FIXME: https://github.com/mapbox/mapbox-gl-native/issues/15008
delete spec.layout_circle["circle-sort-key"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Also remove these lines from the Darwin version of this script:

mapbox-gl-native/platform/darwin/scripts/generate-style-code.js

Lines 18 to 19 in 1f266b4

// FIXME: https://github.com/mapbox/mapbox-gl-native/issues/15008
delete spec.layout_circle["circle-sort-key"]

Copy link
Contributor

Choose a reason for hiding this comment

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

In the future, these properties should be deleted in scripts/style-spec.js instead of both scripts/generate-style-code.js and platform/darwin/scripts/generate-style-code.js. This is the reason for the layout of indirection in scripts/style-spec.js.

Copy link
Contributor

Choose a reason for hiding this comment

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

Now that most of platform/darwin/ has been deleted in favor of the sources in the mapbox/mapbox-gl-native-ios repository, mapbox/mapbox-gl-native-ios#272 will take care of deleting the remnants of this override.

This was referenced Feb 25, 2020
Merged
Merged
@tobrun tobrun mentioned this pull request Feb 25, 2020
Closed
@ahk ahk force-pushed the port-circle-sort-key branch 2 times, most recently from 58f2e01 to 6759618 Compare March 10, 2020 08:15
@chloekraw chloekraw requested review from pozdnyakov and removed request for pozdnyakov March 12, 2020 22:26
pozdnyakov
pozdnyakov approved these changes Mar 13, 2020
View reviewed changes
Copy link
Contributor

@pozdnyakov pozdnyakov left a comment

Choose a reason for hiding this comment

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

🚢

@pozdnyakov pozdnyakov self-assigned this Apr 9, 2020
@pozdnyakov pozdnyakov marked this pull request as draft April 9, 2020 16:50
@pozdnyakov pozdnyakov changed the title Port circle-sort-key [core] Enable circle-sort-key property Apr 14, 2020
@pozdnyakov pozdnyakov marked this pull request as ready for review April 14, 2020 11:58
@pozdnyakov pozdnyakov added Core The cross-platform C++ core, aka mbgl and removed needs changelog Indicates PR needs a changelog entry prior to merging. labels Apr 14, 2020
@pozdnyakov pozdnyakov merged commit 66ac554 into master Apr 14, 2020
@pozdnyakov pozdnyakov deleted the port-circle-sort-key branch April 14, 2020 19:30
@1ec5 1ec5 mentioned this pull request Apr 18, 2020
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pozdnyakov pozdnyakov pozdnyakov approved these changes

@alexshalamov alexshalamov alexshalamov approved these changes

@1ec5 1ec5 1ec5 left review comments

Assignees

@pozdnyakov pozdnyakov

Labels
Core The cross-platform C++ core, aka mbgl GL JS parity For feature parity with Mapbox GL JS
Projects
None yet
Milestone
release-unicorn
Development

Successfully merging this pull request may close these issues.

Port line-sort-key circle-sort-key and fill-sort-key
6 participants
@ahk @chloekraw @pozdnyakov @alexshalamov @1ec5 @asheemmamoowala