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

Let placement transitions use the transition duration, if set, and allow disabling them entirely #13035

Merged
merged 2 commits into from
Nov 21, 2018

Conversation

brunoabinader
Copy link
Member

This PR:

  • Lets property evaluation transitions use the transition duration, if set, with fallback to the default value (300ms) otherwise;
  • Lets label placement transitions use the transition duration, if set, with fallback to the default value (300ms) otherwise;
  • Exposes a boolean flag that allows manual override of label placement transitions, allowing the client to disable transitions for that particular case.

@ChrisLoer @ansis please review.

@brunoabinader brunoabinader added feature Core The cross-platform C++ core, aka mbgl labels Oct 4, 2018
@brunoabinader brunoabinader self-assigned this Oct 4, 2018
@ChrisLoer
Copy link
Contributor

This looks to me like it "does what it says", and I played around with it in the macosapp and it works as expected. I like that you can change the value while the map is running. A couple notes:

  • When !enablePlacementTransitions, placement happens on every frame. This is analogous to behavior in GL JS when fadeDuration == 0 (https://github.com/mapbox/mapbox-gl-js/blob/5d9027cf8127b5d5972e7d5aa162d8d8d391afd7/src/symbol/placement.js#L503-L506). That's OK, but if someone's using this to try to make initial map load "faster", it might be somewhat counter-productive because of the extra placement work involved.
  • If the enablePlacementTransitions flag switches from true to false, it won't take effect until the next time placement occurs on the old timer (because the old Placement will keep being used until its stillRecent returns false). I think this is probably fine and should give consistent animations, just something to keep in mind.

If we're going to add this to the API, my preference would be to go all the way to implementing the controllable "fadeDuration" used by GL JS, just in the name of consistency. I think we would have to switch the "hold for fade" logic to be closer to GL JS (see https://github.com/mapbox/mapbox-gl-js/blob/5d9027cf8127b5d5972e7d5aa162d8d8d391afd7/src/source/source_cache.js#L537-L544) instead of the current approach on native which just holds tiles until two placements have happened.

Copy link
Contributor

@ChrisLoer ChrisLoer left a comment

Choose a reason for hiding this comment

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

I started looking at this again with an eye towards making it closer to GL JS and decided the behavior's actually close enough already. I think we can just merge -- although it would be good to add a changelog entry and some hint for the platform teams that there's a new value to expose (cc @julianrex @LukasPaczos )

@brunoabinader brunoabinader merged commit 594d207 into master Nov 21, 2018
@brunoabinader brunoabinader deleted the placement-transition branch November 21, 2018 14:43
@brunoabinader
Copy link
Member Author

Added a follow-up in #13438 that adds a new style setter for optional persistent transition options. Style or runtime-defined transition options are reset whenever the style changes. Thus, this allows transition options to persist in such cases.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Core The cross-platform C++ core, aka mbgl feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants