-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Port line-sort-key and fill-sort-key #15839
Conversation
Rebasing is preferable. I prefer rebasing regularly so as not to fall behind too much. |
16fd05e
to
ec31f15
Compare
Rebased against master now, apparently that made the darwin platform code generation a noop. |
31e0c64
to
f3993ba
Compare
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)? |
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. |
03b03fb
to
f667059
Compare
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. |
01e7367
to
190dafc
Compare
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.
Looks great overall! just few nits
c640bab
to
9cf1846
Compare
d1f318a
to
94730f9
Compare
22c6fe7
to
9432744
Compare
…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
9432744
to
ae7ea2b
Compare
ae7ea2b
to
1f266b4
Compare
…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
@alexshalamov thanks for taking this over the finish line! |
@@ -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"] |
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.
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"] |
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.
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)
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.
@Chaoba could you follow up on above?
Adds support for
line-sort-key
andfill-sort-key
properties, consistent withsymbol-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 functionmaybeApplySortKey
). 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 aroundLayout
creation (which is most of whatPatternLayout
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.