Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

skip symbol fading for tiles immediately after reload #5741

Merged
merged 1 commit into from
Nov 29, 2017

Conversation

ansis
Copy link
Contributor

@ansis ansis commented Nov 22, 2017

fix #5716

The new symbol fading was causing flickering for symbols animated with setData. When the new location of the symbol was far enough away from the old it would not identify them as being the same icon, resulting in fading. Solving this properly would include figuring out a good way to fix this matching problem.

Instead, this commit works around the problem by skipping fading for any tiles that have just been reloaded.

The new symbol fading was causing flickering for symbols animated with
`setData`. When the new location of the symbol was far enough away from
the old it would not identify them as being the same icon, resulting in
fading. Solving this properly would include figuring out a good way to
fix this matching problem.

Instead, this commit works around the problem by skipping fading for any
tiles that have just been reloaded.
@jfirebaugh
Copy link
Contributor

LGTM, but tagging @ChrisLoer since he's more familiar with this.

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.

Here's how I understand the logic:

  • No change when a reload starts, since justReloaded isn't set until loadVectorData is called.
  • loadVectorData will flow through to SourceCache#_tileLoaded which will flag a full placement before the next frame is rendered.
  • The placement will make all animations instant for that tile, so for instance any collision fade animations (even for unrelated layers) currently in progress will get aborted partway through. Also anything newly (un)placed will be instant.
  • justLoaded will only be true for one frame at a time

The part where fade animations get disabled by the tile reload is unfortunate, but I don't see an easy way around it. We could probably allow fade animations already in progress to continue, though, right? If the OpacityState has an intermediate value between 0 and 1, then doesn't that tell us that the CrossTileSymbolIndex successfully copied the opacity from a previous symbol? Unless we're worried that the duplicate detection could also be getting false positives because there are lots of icons on the screen with overlapping locations...

I don't know if we have any good way to make an automated test for this, but we should pay special attention to this logic when we back-port the placement logic from native, because it'll break the "one frame" assumption above.

@ChrisLoer
Copy link
Contributor

I filed mapbox/mapbox-gl-native#10536 for the native side. It occurs to me that maybe within a single tile we could do a better job of detecting duplicates based on ID or feature index.

I looked at @ryanbaumann's example, and the points don't have IDs, but they do have stable indices in each call to setData -- so at least in that case, an algorithm that said "you're a duplicate if your index/id matches the previous index/id" could work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

symbol layer fades in upon setData
3 participants