[ios, macos] Add crossSourceCollissions property. #12941

fabian-guerra · 2018-09-21T21:58:08Z

1ec5 · 2018-09-24T04:37:51Z

platform/darwin/src/MGLRendererConfiguration.h

+ A Boolean value indicating whether symbol layers may enable cross-source symbol
+ collision detection.
+
+ Set `MGLCrossSourceCollisions` in your containing app's Info.plist


Are we sure this is a setting that should be uniform application-wide? Or is there a use case for enabling this setting for one map view but not another in the same application? If the latter, then there needs to be a method on MGLStyle that controls this setting. That means it needs to be possible to pass the setting into mbgl::Map at an arbitrary point after the mbgl::Map is constructed. (The developer can’t be expected to provide this setting as part of an MGLMapView initializer, because views are typically initialized with nothing but a frame.)

/cc @ChrisLoer

FWIW, in the long back-and-forth on the JS side that led to us making this flag, we ended up conceptualizing this flag as much as possible as a "backwards compatibility switch", just in order to constrain the scope of what can quickly become a confusing design space. In my mind, making the setting application-wide is consistent with the "backwards compatibility" approach.

If we consider this to be legacy behavior, then perhaps we should name the option somewhat more disparagingly and document it as a legacy behavior.

1ec5 · 2018-09-24T22:53:10Z

platform/darwin/src/MGLRendererConfiguration.h

+ Setting this property to `NO`, symbol layers will only run collision detection
+ against other symbol layers that are part of the same source.
+
+ The default value of this property is `YES`.


It’s a bit awkward that leaving the Info.plist option unset is equivalent to YES. Normally leaving an option unset would be equivalent to NO. Could we rename the option to make it more naturally have NO as a default value? Perhaps MGLPerSourceCollisions or MGLV3CollisionBehavior.

1ec5 · 2018-09-24T22:53:56Z

platform/darwin/src/MGLRendererConfiguration.h

+ A Boolean value indicating whether symbol layers may enable cross-source symbol
+ collision detection.
+
+ Set `MGLCrossSourceCollisions` in your containing app's Info.plist


Since the header is internal, this documentation isn’t visible to developers. Add an entry about the Info.plist key to the “Info.plist Keys” guide.

friedbunny · 2018-09-25T21:46:21Z

platform/ios/docs/guides/Info.plist Keys.md

@@ -23,3 +23,7 @@ If you have implemented custom opt-out of Mapbox Telemetry within the user inter
 ## MGLIdeographicFontFamilyName

 The name of the font family to use for client-side text rendering of CJK ideographs. Set this to the name of a font family which will be available at run time, e.g. `PingFang TC` (iOS 9+), `Heiti TC` (iOS 8+), another appropriate built-in font, or a font provided by your application. Note that if a non-existent font is specified, iOS will fall back to using Helvetica which is likely not to include support for the glyphs needed to render maps in your application.
+
+## MGLPerSourceCollisions


Per #12941 (comment), I think we should name this more discouragingly — this should be a key that shouts “legacy! only enable if you know what you’re doing.”

/cc @1ec5 @ChrisLoer

Or at least the practical consequences of enabling it should be made more clear: that symbols in different sources, such as Streets versus a shape source, may overlap unexpectedly.

friedbunny · 2018-09-25T21:46:46Z

platform/ios/docs/guides/Info.plist Keys.md

+
+## MGLPerSourceCollisions
+
+Use this key to enable per-source symbol collision detection.


If this is legacy functionality, let’s also say so here.

1ec5

Here’s some revised wording that explains in more detail how the behavior differs.

1ec5 · 2018-09-26T04:36:10Z

platform/ios/docs/guides/Info.plist Keys.md

+
+## MGLCollisionBehaviorPre4_0
+
+If enabled symbol layers will only run legacy collision detection against other symbol layers that are part of the same source. Symbols in different sources, such as Streets versus a shape source, may overlap unexpectedly.


This is a pretty convoluted behavior to explain in lay terms, but I think this more or less covers it:

If this key is set to YES (true), collision detection is performed only between symbol style layers based on the same source, as in versions 2.0–3.7 of the Mapbox Maps SDK for iOS. In other words, symbols in an MGLSymbolStyleLayer based on one source (for example, an MGLShapeSource) may overlap with symbols in another layer that is based on a different source (such as the Mapbox Streets source). This is the case regardless of the MGLSymbolStyleLayer.iconAllowsOverlap, MGLSymbolStyleLayer.iconIgnoresPlacement, MGLSymbolStyleLayer.textAllowsOverlap, and MGLSymbolStyleLayer.textIgnoresPlacement properties.

Beginning in version 4.0, the SDK also performs collision detection between style layers based on different sources by default. For the default behavior, omit the MGLCollisionBehaviorPre4_0 key or set it to NO (false).

1ec5 · 2018-09-26T04:41:35Z

platform/macos/docs/guides/Info.plist Keys.md

+The name of the font family to use for client-side text rendering of CJK ideographs. Set this to the name of a font family which will be available at run time, e.g. `PingFang TC` (iOS 9+), `Heiti TC` (iOS 8+), another appropriate built-in font, or a font provided by your application. Note that if a non-existent font is specified, iOS will fall back to using Helvetica which is likely not to include support for the glyphs needed to render maps in your application.
+
+## MGLCollisionBehaviorPre4_0


Heh, I just realized it isn’t “4.0” for macOS. Not a big deal I suppose.

1ec5 · 2018-09-26T04:42:46Z

platform/macos/docs/guides/Info.plist Keys.md

+## MGLCollisionBehaviorPre4_0
+
+If enabled symbol layers will only run legacy collision detection against other symbol layers that are part of the same source. Symbols in different sources, such as Streets versus a shape source, may overlap unexpectedly.


If this key is set to YES (true), collision detection is performed only between symbol style layers based on the same source, as in versions 0.1–0.7 of the Mapbox Maps SDK for iOS. In other words, symbols in an MGLSymbolStyleLayer based on one source (for example, an MGLShapeSource) may overlap with symbols in another layer that is based on a different source (such as the Mapbox Streets source). This is the case regardless of the MGLSymbolStyleLayer.iconAllowsOverlap, MGLSymbolStyleLayer.iconIgnoresPlacement, MGLSymbolStyleLayer.textAllowsOverlap, and MGLSymbolStyleLayer.textIgnoresPlacement properties.

Beginning in version 0.7, the SDK also performs collision detection between style layers based on different sources by default. For the default behavior, omit the MGLCollisionBehaviorPre4_0 key or set it to NO (false). This property is so named because version 0.7 of the Mapbox Maps SDK for macOS corresponds to version 4.0 of the Mapbox Maps SDK for iOS.

friedbunny · 2018-09-26T15:50:24Z

platform/darwin/src/MGLRendererConfiguration.h

+ A Boolean value indicating whether symbol layers may enable per-source symbol
+ collision detection.
+
+ Set `MGLCollisionBehaviorPre4_0` in your containing app's Info.plist


Nit: add a period at the end of this sentence.

friedbunny · 2018-09-26T15:51:00Z

platform/darwin/src/MGLRendererConfiguration.h

+
+ Set `MGLCollisionBehaviorPre4_0` in your containing app's Info.plist
+
+ Setting this property to `YES`, symbol layers will only run collision detection


Setting this property to YES,

This could be confusing, as the property defined in this header is readonly. Here’s my suggestion:

Setting this property to YES in the plist results in symbol layers only running collision detection against other symbol layers that are part of the same source.

platform/ios/docs/guides/Info.plist Keys.md

platform/macos/docs/guides/Info.plist Keys.md

1ec5 · 2018-09-27T01:28:10Z

platform/macos/CHANGELOG.md

@@ -16,6 +16,7 @@
 * Fixed an issue where `-[MGLMapSnapshotter startWithQueue:completionHandler:]` failed to call its completion handler in some cases. ([#12355](https://github.com/mapbox/mapbox-gl-native/pull/12355))
 * Fixed an issue where `MGLMapView` produced a designable error in Interface Builder storyboards in Xcode 10. ([#12883](https://github.com/mapbox/mapbox-gl-native/pull/12883))
 * Fixed bugs in coercion expression operators ("to-array" applied to empty arrays, "to-color" applied to colors, and "to-number" applied to null) [#12864](https://github.com/mapbox/mapbox-gl-native/pull/12864)
+* Added the `MGLCollisionBehaviorPre4_0` Info.plist key for applications that require the collision detection behavior in version v3.7 of the SDK. ([#12941](https://github.com/mapbox/mapbox-gl-native/pull/12941))


On macOS, it was version v0.6, not v3.7.

/cc @fabian-guerra

fabian-guerra added iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS needs changelog Indicates PR needs a changelog entry prior to merging. labels Sep 21, 2018

fabian-guerra added this to the ios-v4.5.0 milestone Sep 21, 2018

fabian-guerra self-assigned this Sep 21, 2018

fabian-guerra requested review from akitchen and 1ec5 September 21, 2018 21:58

1ec5 reviewed Sep 24, 2018

View reviewed changes

fabian-guerra force-pushed the fabian-cross-source-12847 branch from 29e96ef to f24e913 Compare September 25, 2018 21:35

friedbunny reviewed Sep 25, 2018

View reviewed changes

fabian-guerra force-pushed the fabian-cross-source-12847 branch from f24e913 to 25b4a13 Compare September 25, 2018 22:35

1ec5 approved these changes Sep 26, 2018

View reviewed changes

friedbunny reviewed Sep 26, 2018

View reviewed changes

fabian-guerra added 2 commits September 26, 2018 10:28

[ios, macos] Add crossSourceCollissions property.

e218fbc

[ios, macos] Add perSourceCollisions property.

e545960

fabian-guerra force-pushed the fabian-cross-source-12847 branch from 25b4a13 to e545960 Compare September 26, 2018 17:29

[ios, macos] Update changelogs.

b5abc81

fabian-guerra removed the needs changelog Indicates PR needs a changelog entry prior to merging. label Sep 26, 2018

fabian-guerra merged commit 5783744 into master Sep 26, 2018

fabian-guerra deleted the fabian-cross-source-12847 branch September 26, 2018 22:17

1ec5 reviewed Sep 27, 2018

View reviewed changes

samfader mentioned this pull request Oct 22, 2018

Respect user default (NSUserDefaults) for MGLCollisionBehaviorPre4_0 Info.plist key #13155

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ios, macos] Add crossSourceCollissions property. #12941

[ios, macos] Add crossSourceCollissions property. #12941

fabian-guerra commented Sep 21, 2018

1ec5 Sep 24, 2018

ChrisLoer Sep 24, 2018

1ec5 Sep 24, 2018

1ec5 Sep 24, 2018

1ec5 Sep 24, 2018

friedbunny Sep 25, 2018

1ec5 Sep 25, 2018 •

edited

Loading

friedbunny Sep 25, 2018

1ec5 left a comment

1ec5 Sep 26, 2018

1ec5 Sep 26, 2018

1ec5 Sep 26, 2018

friedbunny Sep 26, 2018

friedbunny Sep 26, 2018

1ec5 Sep 27, 2018


		## MGLPerSourceCollisions

		Use this key to enable per-source symbol collision detection.


		## MGLCollisionBehaviorPre4_0

		If enabled symbol layers will only run legacy collision detection against other symbol layers that are part of the same source. Symbols in different sources, such as Streets versus a shape source, may overlap unexpectedly.

		The name of the font family to use for client-side text rendering of CJK ideographs. Set this to the name of a font family which will be available at run time, e.g. `PingFang TC` (iOS 9+), `Heiti TC` (iOS 8+), another appropriate built-in font, or a font provided by your application. Note that if a non-existent font is specified, iOS will fall back to using Helvetica which is likely not to include support for the glyphs needed to render maps in your application.

		## MGLCollisionBehaviorPre4_0


		Set `MGLCollisionBehaviorPre4_0` in your containing app's Info.plist

		Setting this property to `YES`, symbol layers will only run collision detection

[ios, macos] Add crossSourceCollissions property. #12941

[ios, macos] Add crossSourceCollissions property. #12941

Conversation

fabian-guerra commented Sep 21, 2018

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

1ec5 Sep 25, 2018 • edited Loading

Choose a reason for hiding this 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

1ec5 Sep 25, 2018 •

edited

Loading