[core] Port symbol-z-order symbol layout style spec property #12783

ryanhamley · 2018-08-31T00:35:47Z

Launch Checklist

briefly describe the changes in this PR
- Adds symbol-sort-by-y symbol layout property to style spec
- See discussion in Style spec: add option to disable y-position sort for symbol features mapbox-gl-js#7076
- Add symbol-z-order symbol layout property to style spec mapbox-gl-js#7219 will need to be merged before this

ryanhamley · 2018-08-31T00:38:06Z

I know I will need to change the submodule to point to a commit on master after the JS PR is merged. The render tests were running properly before I ran make style-code so I'm investigating what happened there [EDIT: I reverted that commit while I investigate].

ChrisLoer

Wow so much boilerplate for such a simple change! Thanks to everyone who implemented all the codegen tools...

Overall this looks great. I had one nit about variable naming, and then the other thing (and something I'm still trying to get disciplined about) is that the native SDK teams would like us to include CHANGELOG entries with each PR (in macOS/iOS/Android/Node).

ChrisLoer · 2018-09-06T16:04:39Z

src/mbgl/layout/symbol_layout.cpp

-    const bool mayOverlap = layout.get<TextAllowOverlap>() || layout.get<IconAllowOverlap>() ||
-        layout.get<TextIgnorePlacement>() || layout.get<IconIgnorePlacement>();
+    const bool zOrderByViewport = layout.get<SymbolZOrder>() == SymbolZOrderType::ViewportY;
+    const bool mayOverlap = zOrderByViewport && (layout.get<TextAllowOverlap>() || layout.get<IconAllowOverlap>() ||


nit: Maybe we should follow gl-js and rename mayOverlap to something like sortFeaturesByY? The flag now encodes more information than just whether they can overlap...

ChrisLoer

🍏 🚀

1ec5

This is pretty awesome – it accounts for the difference between puck-like and pin-like markers, which are both very common in mobile applications.

The feedback below largely pertains to improvements we could make to the iOS/macOS API for consistency with Apple APIs. It’s very much a fit-and-finish thing, one that we’d either do in the same release as this PR (perhaps as tail work) or not at all.

1ec5 · 2018-09-07T01:48:07Z

platform/darwin/src/MGLSymbolStyleLayer.h

+     Symbols will be rendered in the same order as the source data with no
+     sorting applied.
+     */
+    MGLSymbolZOrderSource,


Based on similar enumerations in Apple frameworks, perhaps names like MGLSymbolZOrderByScreenY (“relative to their y positions in the screen coordinate system”) and MGLSymbolZOrderBySource would be more familiar-sounding to iOS/macOS developers.

Unfortunately, platform/darwin/scripts/style-spec-cocoa-conventions-v8.json only offers a way to override the names of properties, not enumerations. Maybe overriding the value names in platform/darwin/scripts/style-spec-overrides-v8.json would work, but I’m not certain. On the other hand, these enumerations are not nearly as prominent as they once were, now that expressions can accept embedded string literals.

/cc @friedbunny

So I tried overwriting the enum names and descriptions in style-spec-cocoa-conventions-v8.json but instead of overwriting the existing enums, it simply added two new ones. In other words, there would now be MGLSymbolZOrderSource and MGLSymbolZOrderBySource so I'm not sure this is possible with the current setup, unless I'm missing something.

Huh, looks like you’re right. I guess it isn’t worth building out the capability to override individual enumeration values, given that the literal string values are more common and can’t change anyways (since they can be built up through concatenation etc.).

/cc @friedbunny

1ec5 · 2018-09-07T01:50:57Z

platform/darwin/src/MGLSymbolStyleLayer.h

+ `$zoomLevel` variable or applying interpolation or step functions to feature
+ attributes.
+ */
+@property (nonatomic, null_resettable) NSExpression *symbolZOrder;


For what it’s worth, Apple frameworks like MapKit avoid the terms “z-order” and “z-index” in favor of circumlocutions like “overlay level” (see MKOverlayLevel, NSWindowLevel), while “z” is reserved for non-discrete values. Meanwhile, there’s also a concept of sorting in the standard libraries, as seen in NSSortDescriptor. But maybe “z order” is more discoverable in this case; not sure.

ZOrder does indeed look strange in Cocoa — since we try our best to follow platform conventions, I’d be in favor of renaming iOS/macOS methods/properties. Docs could still reference “z-order”.

Is there a way to target only the iOS/macOS names? I'm not really familiar with the Native codebase yet but I'd be happy to open a new PR to fix this.

Yes, each entry in platform/darwin/scripts/style-spec-cocoa-conventions-v8.json overrides a property name.

1ec5 · 2018-09-07T01:58:31Z

platform/ios/CHANGELOG.md

@@ -2,6 +2,9 @@

 Mapbox welcomes participation and contributions from everyone. Please read [CONTRIBUTING.md](../../CONTRIBUTING.md) to get started.

+## master
+* Add `symbol-z-order` symbol layout property to style spec [#12783](https://github.com/mapbox/mapbox-gl-native/pull/12783)


The iOS and macOS changelogs typically refer to style properties as they’re exposed in the runtime styling API, which is the primary way that these developers control the properties. There also isn’t much of a distinction between paint and layout properties in the runtime styling API.

Added an MGLSymbolStyleLayer.symbolZOrder property for forcing point features in a symbol layer to be layered in the same order that they are specified in the layer’s associated source.

1ec5 · 2018-09-07T02:01:00Z

platform/darwin/src/MGLSymbolStyleLayer.h

+    MGLSymbolZOrderViewportY,
+    /**
+     Symbols will be rendered in the same order as the source data with no
+     sorting applied.


Specify this z order if the order in which features appear in the source is significant.

1ec5 · 2018-09-07T02:02:23Z

platform/darwin/src/MGLSymbolStyleLayer.h

+ */
+typedef NS_ENUM(NSUInteger, MGLSymbolZOrder) {
+    /**
+     Symbols will be sorted by their y-position relative to the viewport.


In platform/darwin/scripts/style-spec-overrides-v8.json, we should be able to override this to say:

Specify this z order if symbols’ appearance relies on lower features overlapping higher features. For example, symbols with a pin-like appearance would require this z order.

friedbunny · 2018-09-07T22:09:19Z

Nice work, congrats on your first commit to gl-native — please read this repo’s contribution guidelines for info on commit and issue tagging. 🙇

ryanhamley · 2018-09-07T22:13:56Z

Ah darn, sorry about that! Thanks for the links. I will make sure I tag everything properly next time.

ryanhamley force-pushed the sort-by-y branch from d7129c2 to 011cde6 Compare August 31, 2018 23:27

ryanhamley mentioned this pull request Aug 31, 2018

Add symbol-z-order symbol layout property to style spec mapbox/mapbox-gl-js#7219

Merged

5 tasks

ryanhamley changed the title ~~Port symbol-sort-relative-to-viewport symbol layout style spec property~~ Port symbol-z-order symbol layout style spec property Sep 1, 2018

ryanhamley requested a review from ChrisLoer September 6, 2018 03:36

ChrisLoer reviewed Sep 6, 2018

View reviewed changes

ryanhamley changed the title ~~Port symbol-z-order symbol layout style spec property~~ [core] Port symbol-z-order symbol layout style spec property Sep 6, 2018

ChrisLoer approved these changes Sep 6, 2018

View reviewed changes

ryanhamley force-pushed the sort-by-y branch 2 times, most recently from d985ad8 to d7dd816 Compare September 7, 2018 00:13

1ec5 reviewed Sep 7, 2018

View reviewed changes

ryanhamley force-pushed the sort-by-y branch from d7dd816 to 7c5fd38 Compare September 7, 2018 17:56

Port symbol-z-order symbol layout style-spec property to Native

31cca05

ryanhamley force-pushed the sort-by-y branch from 81e1655 to 31cca05 Compare September 7, 2018 20:05

ryanhamley merged commit 31cca05 into master Sep 7, 2018

ryanhamley deleted the sort-by-y branch September 7, 2018 21:13

friedbunny added GL JS parity For feature parity with Mapbox GL JS iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS Android Mapbox Maps SDK for Android Core The cross-platform C++ core, aka mbgl runtime styling labels Sep 7, 2018

This was referenced Sep 10, 2018

Integrate Z indexing support for symbols mapbox/mapbox-plugins-android#657

Closed

[android] Single marker z-order: keep a marker on top of everything else, moving it continuously #10193

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Port symbol-z-order symbol layout style spec property #12783

[core] Port symbol-z-order symbol layout style spec property #12783

ryanhamley commented Aug 31, 2018 •

edited

Loading

ryanhamley commented Aug 31, 2018 •

edited

Loading

ChrisLoer left a comment

ChrisLoer Sep 6, 2018

ChrisLoer left a comment

1ec5 left a comment

1ec5 Sep 7, 2018

ryanhamley Sep 7, 2018

1ec5 Sep 8, 2018

1ec5 Sep 7, 2018

friedbunny Sep 7, 2018

ryanhamley Sep 7, 2018

1ec5 Sep 8, 2018

1ec5 Sep 7, 2018

1ec5 Sep 7, 2018

1ec5 Sep 7, 2018

friedbunny commented Sep 7, 2018

ryanhamley commented Sep 7, 2018

[core] Port symbol-z-order symbol layout style spec property #12783

[core] Port symbol-z-order symbol layout style spec property #12783

Conversation

ryanhamley commented Aug 31, 2018 • edited Loading

Launch Checklist

ryanhamley commented Aug 31, 2018 • edited Loading

ChrisLoer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisLoer left a comment

Choose a reason for hiding this comment

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

friedbunny commented Sep 7, 2018

ryanhamley commented Sep 7, 2018

ryanhamley commented Aug 31, 2018 •

edited

Loading

ryanhamley commented Aug 31, 2018 •

edited

Loading