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

[ios] Change MGLAnnotationView.scalesWithViewingDistance default value to NO #11636

Merged
merged 1 commit into from
Apr 12, 2018

Conversation

friedbunny
Copy link
Contributor

@friedbunny friedbunny commented Apr 9, 2018

Changes the default value of MGLAnnotationView.scalesWithViewingDistance to NO, which removes various inescapable overhead and improves out-of-box performance (FPS) by:

  • 7% when pitch = 0
  • 18% when pitch = 30

I measured this on iPhone X, 6s, and iPad Pro 10.5 running iOS 11.3/11.4b1 using a cobbled together ~3000 annotation, blank map style, camera movement benchmark — see the fb-bench-view-annotations branch for details.

This change is prompted by the fact that annotation views do not generally perform well (even on top-of-the-line devices) and we need all the help we can get to maintain a smooth frame rate, otherwise the sync between annotation views and the map can become distractingly juddery.

See #5040 for past discussion of this default behavior (which was implemented in #5085). This PR follows up on #10951, also in ios-v4.0.0.

/cc @mapbox/maps-ios

@friedbunny friedbunny added iOS Mapbox Maps SDK for iOS performance Speed, stability, CPU usage, memory usage, or power usage annotations Annotations on iOS and macOS or markers on Android labels Apr 9, 2018
@friedbunny friedbunny added this to the ios-v4.0.0 milestone Apr 9, 2018
@friedbunny friedbunny self-assigned this Apr 9, 2018
The default value of this property is `YES`. Set this property to `NO` if the
view’s legibility is important.
The default value of this property is `NO`. Keep this property set to `NO` if
the view’s legibility is important.
Copy link
Contributor

@1ec5 1ec5 Apr 10, 2018

Choose a reason for hiding this comment

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

Per #5040, mapbox/mapbox-gl-js#4120 (comment), and mapbox/mapbox-gl-js#4547, we don’t allow developers to turn off scaling GL point annotations or symbol layers based on viewing distance:

…or if the map also represents some annotations with MGLAnnotationImage objects, or if the style also contains symbol layers that the user can interact with.

This brings the change in behavior here into question to some extent. It’s already confusing enough that there are three ways to put a marker on the map, without one of those ways scaling different than the others.

/cc @ChrisLoer

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not familiar enough with how MGLAnnotationView is typically used to have a strong opinion here. But if I'm reading this all correctly, isn't it the case that either way, the scaling for annotation views will be 50% off from symbol layers (because they use the hardwired 50% "attenuated" pitch-scaling, and this functionality is either 0 or 100%)?

Copy link
Contributor

Choose a reason for hiding this comment

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

I’m not sure if the pitch-scaling for view-backed annotations would be 100% – it’s a bit of a hack because we don’t have access to the map transform at this level (see also #10498). So I guess there’s inconsistency regardless. 👍

Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

I’m OK with this change, given the following considerations:

  • Redrawing performance is an area for improvement for view-backed annotations.
  • View-backed annotations differ significantly from symbol layers in that the former is interactive out of the box, and interactivity makes a consistent hit target desirable.
  • There’s an easy way for developers to return to the previous behavior.
  • Non-view-backed annotations are only recommended for a narrow range of use cases (though we do feature them in installation guides).
  • This may create a stronger case for more symbol layer styling flexibility in the future.

@friedbunny friedbunny force-pushed the fb-undefault-scalesWithViewingDistance branch from 12ea064 to 2088868 Compare April 12, 2018 17:54
@friedbunny friedbunny force-pushed the fb-undefault-scalesWithViewingDistance branch from 2088868 to be3fc44 Compare April 12, 2018 20:49
@friedbunny friedbunny merged commit be3fc44 into release-boba Apr 12, 2018
@friedbunny friedbunny deleted the fb-undefault-scalesWithViewingDistance branch April 12, 2018 21:42
@timotheeduran
Copy link

I think that you forgot to change it here

The default value of this property is YES.

But I understand that it's FALSE now.

@julianrex
Copy link
Contributor

@burgercode the link you shared is for 3.7.3. It should be correct in 4.0.0 onwards.

@timotheeduran
Copy link

Ow, I'm sorry!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
annotations Annotations on iOS and macOS or markers on Android iOS Mapbox Maps SDK for iOS performance Speed, stability, CPU usage, memory usage, or power usage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants