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

viewport-collision: symbol cross-fading #10468

Merged
merged 1 commit into from
Nov 16, 2017

Conversation

ChrisLoer
Copy link
Contributor

This is a proof-of-concept of doing symbol cross fading by holding on to render tiles until after their symbols have had a chance to fade out. It doesn't take care of issues like avoiding over-draw of non-symbol layers, but it's enough to get a feel for how it might work.

I think it looks fairly promising, although it'll introduce enough complications (questions like: how many of these "hold for fading" tiles will you hold on to during rapid pan/fade operations?) that I'd prefer to push it to a follow-up PR after viewport-collision merges, if possible.

This PR: cross fading
cross_fade

Master: instant switch
master_instant_switch

/cc @ansis @jfirebaugh @kkaefer @nickidlugash

@ChrisLoer
Copy link
Contributor Author

Point label cross fading is nice-to-have too, although probably less of a big deal:

This PR
point_cross_fade

Master
point_swap_master

@mb12
Copy link

mb12 commented Nov 15, 2017

@ChrisLoer Would this also work for icons added to the map via styling geojson or image annotations?

@ChrisLoer
Copy link
Contributor Author

Would this also work for icons added to the map via styling geojson or image annotations?

@mb12 sorry if I'm not understanding your question correctly, but this wouldn't directly interact with annotations. Labels (whether text or icon) added as part of a geojson source would get the fade-out behavior, although this change just relates to being able to fade them out at the time the tile they're part of disappears due to a viewport change (e.g. zoom in/out).

@ChrisLoer
Copy link
Contributor Author

I think this is probably fine, but one effect of the current implementation is that if you zoom out quickly enough, you can end up with a really densely placed area of the map -- even though all the old high-zoom labels are fading out, they'll all be overlapping each other.

This might be an argument for limiting the "hold for fade" behavior to cross just one or two zoom levels.

dense_fade_out

@ChrisLoer ChrisLoer removed the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Nov 16, 2017
@ChrisLoer
Copy link
Contributor Author

As a sanity check, I re-ran the iOS Bench app on my 5s with the 10 second animation cases (pitched rotation + zoom level changes). The pitched rotation case still came in at 59 FPS, and the zoom case still came in at 60 FPS (the zoom case now has more rendering work to do because of the extra tiles it holds on to: for roughly 10% of the animation the symbol layers will be doubled-up which means lots of extra line label projection).

@ChrisLoer ChrisLoer changed the title Experiment: symbol cross-fading viewport-collision: symbol cross-fading Nov 16, 2017
Copy link
Contributor

@kkaefer kkaefer left a comment

Choose a reason for hiding this comment

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

This looks excellent! Looking at the code, it looks like we're not adding a lot of complexity either.

I think having overlapping labels for a split second while they fade out is totally acceptable, however we can experiment with accelerating fadeout (or instant removal) for rapid zooms like you suggested in a followup PR.

Another tweak (out of scope for this PR) that we could do is to not fade labels that were previously off screen (e.g. when panning)

// Don't render non-symbol layers for tiles that we're only holding on to for symbol fading
sortedTiles.erase(std::remove_if(sortedTiles.begin(), sortedTiles.end(),
[](const auto& tile) { return tile.get().tile.holdForFade(); }),
sortedTiles.end());
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we somehow remove these from the "render tiles" array to begin with?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked at this a little bit. I think it would look something like: add a getFadingRenderTiles() method to RenderSource, and then implement it in the various children of that class (any source that doesn't have fading tiles can use a default empty implementation). In the renderer, just concatenate getFadingRenderTiles() onto getRenderTiles() for symbol layers. Then in TilePyramid, we could either maintain a parallel set of "hold for fading" tiles, or we could just do the filtering in the accessors (since internally to TilePyramid you usually want to iterate over all of them at the same time).

I think it could be an improvement, but it wasn't such an obvious simplification that I really wanted to get it in. Unless you're concerned, I'll defer it to future refactorings.

 Hold onto tiles after they've been removed from the render tree long enough to run a fade animation on their symbols.
@ChrisLoer ChrisLoer merged this pull request into viewport-collision Nov 16, 2017
ChrisLoer added a commit to mapbox/mapbox-gl-js that referenced this pull request Jul 11, 2018
ChrisLoer added a commit to mapbox/mapbox-gl-js that referenced this pull request Jul 16, 2018
@jfirebaugh jfirebaugh deleted the viewport-collision-cross-fade branch July 27, 2018 22:47
pirxpilot pushed a commit to pirxpilot/mapbox-gl-js that referenced this pull request Sep 11, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants