Port line-sort-key and fill-sort-key #15839

ahk · 2019-10-21T22:49:25Z

Adds support for line-sort-key and fill-sort-key properties, consistent with symbol-sort-key.

It took me quite a while to gain proficiency with the C++ usage in the project (I was rustier than I thought). Consider this a draft, and I would love to hear your comments and suggestions. I'm sure I may have style or design issues with my approach. For quite a while I pursued a design that modified the PatternLayout contructor only (rather than introducing my new function maybeApplySortKey). This was difficult for me to figure out because fill extrusion layer type does not support *-sort-key but there is a decent chunk of shared code among fill, line, and fill extrusion layers around Layout creation (which is most of what PatternLayout class is). Hopefully someone can point me to a better design.

I attempted to be consistent with the original PR for symbol-sort-key as much as possible.

asheemmamoowala · 2019-10-21T22:56:58Z

I haven't tried to rebase this, it's obviously out of date. Is it preferred to rebase this against master or to merge master?

Rebasing is preferable. I prefer rebasing regularly so as not to fall behind too much.

ahk · 2019-10-22T23:51:00Z

I haven't tried to rebase this, it's obviously out of date. Is it preferred to rebase this against master or to merge master?

Rebasing is preferable. I prefer rebasing regularly so as not to fall behind too much.

Rebased against master now, apparently that made the darwin platform code generation a noop.

src/mbgl/layermanager/fill_layer_factory.cpp

src/mbgl/layout/pattern_layout.hpp

pozdnyakov · 2019-11-13T09:40:23Z

The new approach looks great! @ahk what is the status of this pr? is it ready for review & merge (I see it is still a draft)?

ahk · 2019-11-14T23:48:40Z

is it ready for review & merge (I see it is still a draft)?

I still need to update the sorting logic to incrementally maintain the sort in the destination as you requested earlier. I'll clean that up and move it out of draft. It looks like it I'm failing more of the circleci ARM and Linux builds than I was before ... I might need some help figuring that out.

ahk · 2019-12-05T00:20:42Z

Hi @pozdnyakov I've updated this branch to maintain the sort of features during layout step and use a std::list over a std::vector. I also found an indexing numeric limits bug.

I still can't figure out how to make the sanity-check format step happy. When I apply the patch it suggests and it reruns it shows me a new patch that undoes the previous patch, can't explain that one.

Also apparently some of our linux targets use a compiler that doesn't allow fully specializing a template function in a class template. I know partial specialization isn't allowed but if you have any tips on how to resolve that I would appreciate it. Is this a funny parsing quirk I'm not familiar with?

Moved this to review because I think it does everything it's supposed to and I resolved all the comments as requested.

pozdnyakov

Looks great overall! just few nits

src/mbgl/layout/pattern_layout.hpp

…15839) - Generate style code for 'line-sort-key' and 'symbol-sort-key' - Add new layout properties to FillLayer::Impl, FillBucket, and FillLayerFactory - Fix consistency of paint and layout properties type alias usage in FillBucket, LineBucket - Add optional feature sorting to fill and line Layout creation - Enable node render tests for fill-sort-key and line-sort-key - Fix FillBucket test construction - Prefer emplace_back to push_back for PatternFeature container - Fix buggy static_cast for PatternFeature indices - Maintain sort of features as they are created - Switch pattern layout features container to list from vector for better insert performance - Fix formatting expected by sanity check - Use subclass PatternLayoutSorted to work around lack of template functions - Fix to retain source order for features with equivalent sort keys during sorting - [core] Fix clang-format - [core] Address review comments - [core] Pass inserting strategy class at compile time - [core] Use sorted strategy only if sort key is defined in layout - [core] Update style generator - [core] Merge PatternLayout and PatternLayoutSorted classes - Use static methods for inserter strategies - Merge PatternLayout and PatternLayoutSorted classes

chloekraw · 2019-12-18T21:28:08Z

@alexshalamov thanks for taking this over the finish line!

1ec5 · 2020-02-25T07:24:07Z

scripts/generate-style-code.js

@@ -8,8 +8,6 @@ const colorParser = require('csscolorparser');

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


These lines are duplicated in the Darwin version of this script:

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

Lines 20 to 21 in 1f266b4

delete spec.layout_line["line-sort-key"]

delete spec.layout_fill["fill-sort-key"]

mapbox/mapbox-gl-native-ios#179 updates the gl-native-ios repository to reflect this change. @tobrun, heads-up that the Android map SDK is missing runtime styling methods due to similar delete statements in platform/android/scripts/generate-style-code.js.

/ref #15875 (comment)

@Chaoba could you follow up on above?

ahk mentioned this pull request Oct 21, 2019

Port line-sort-key circle-sort-key and fill-sort-key #15008

Closed

ahk force-pushed the port-fill-line-sort-key branch from 16fd05e to ec31f15 Compare October 22, 2019 23:21

ahk requested a review from pozdnyakov October 23, 2019 20:16

pozdnyakov reviewed Oct 23, 2019

View reviewed changes

src/mbgl/layermanager/fill_layer_factory.cpp Outdated Show resolved Hide resolved

src/mbgl/layout/pattern_layout.hpp Outdated Show resolved Hide resolved

src/mbgl/layout/pattern_layout.hpp Outdated Show resolved Hide resolved

ahk force-pushed the port-fill-line-sort-key branch from 31e0c64 to f3993ba Compare October 31, 2019 20:52

chloekraw added the needs changelog Indicates PR needs a changelog entry prior to merging. label Nov 21, 2019

ahk force-pushed the port-fill-line-sort-key branch from 03b03fb to f667059 Compare December 5, 2019 00:11

ahk marked this pull request as ready for review December 5, 2019 00:15

ahk force-pushed the port-fill-line-sort-key branch 2 times, most recently from 01e7367 to 190dafc Compare December 5, 2019 06:03

pozdnyakov reviewed Dec 9, 2019

View reviewed changes

src/mbgl/layout/pattern_layout.hpp Outdated Show resolved Hide resolved

src/mbgl/layout/pattern_layout.hpp Outdated Show resolved Hide resolved

alexshalamov force-pushed the port-fill-line-sort-key branch 2 times, most recently from c640bab to 9cf1846 Compare December 10, 2019 16:39

pozdnyakov reviewed Dec 11, 2019

View reviewed changes

src/mbgl/layout/pattern_layout.hpp Outdated Show resolved Hide resolved

src/mbgl/layout/pattern_layout.hpp Outdated Show resolved Hide resolved

src/mbgl/layout/pattern_layout.hpp Outdated Show resolved Hide resolved

pozdnyakov approved these changes Dec 11, 2019

View reviewed changes

alexshalamov force-pushed the port-fill-line-sort-key branch 2 times, most recently from d1f318a to 94730f9 Compare December 17, 2019 09:45

alexshalamov removed the needs changelog Indicates PR needs a changelog entry prior to merging. label Dec 17, 2019

alexshalamov force-pushed the port-fill-line-sort-key branch 2 times, most recently from 22c6fe7 to 9432744 Compare December 17, 2019 10:41

ahk and others added 3 commits December 17, 2019 13:37

[core] Unskip render tests

b36c03a

[metrics] Add new baselines

b2d080a

alexshalamov force-pushed the port-fill-line-sort-key branch from 9432744 to ae7ea2b Compare December 17, 2019 11:37

[core] Update changelog

1f266b4

alexshalamov force-pushed the port-fill-line-sort-key branch from ae7ea2b to 1f266b4 Compare December 17, 2019 11:37

alexshalamov merged commit 493d2ca into master Dec 17, 2019

chloekraw added this to the release-unicorn milestone Dec 18, 2019

chloekraw mentioned this pull request Dec 18, 2019

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

Merged

1ec5 reviewed Feb 25, 2020

View reviewed changes

1ec5 mentioned this pull request Feb 25, 2020

Restore runtime styling code generation mapbox/mapbox-gl-native-ios#179

Merged

tobrun mentioned this pull request Feb 25, 2020

Port circle-sort-key mapbox/mapbox-gl-native-android#197

Closed

Chaoba mentioned this pull request Mar 2, 2020

[android] Add jni binding for line-sort-key and fill-sort-key #16256

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port line-sort-key and fill-sort-key #15839

Port line-sort-key and fill-sort-key #15839

ahk commented Oct 21, 2019 •

edited

asheemmamoowala commented Oct 21, 2019

ahk commented Oct 22, 2019

pozdnyakov commented Nov 13, 2019

ahk commented Nov 14, 2019

ahk commented Dec 5, 2019

pozdnyakov left a comment

chloekraw commented Dec 18, 2019

1ec5 Feb 25, 2020

1ec5 Feb 25, 2020 •

edited

tobrun Feb 25, 2020

	delete spec.layout_line["line-sort-key"]
	delete spec.layout_fill["fill-sort-key"]

Port line-sort-key and fill-sort-key #15839

Port line-sort-key and fill-sort-key #15839

Conversation

ahk commented Oct 21, 2019 • edited

asheemmamoowala commented Oct 21, 2019

ahk commented Oct 22, 2019

pozdnyakov commented Nov 13, 2019

ahk commented Nov 14, 2019

ahk commented Dec 5, 2019

pozdnyakov left a comment

Choose a reason for hiding this comment

chloekraw commented Dec 18, 2019

1ec5 Feb 25, 2020

Choose a reason for hiding this comment

1ec5 Feb 25, 2020 • edited

Choose a reason for hiding this comment

tobrun Feb 25, 2020

Choose a reason for hiding this comment

ahk commented Oct 21, 2019 •

edited

1ec5 Feb 25, 2020 •

edited