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

Preloading tiles for camera animation #11328

Merged
merged 19 commits into from
Dec 22, 2021
Merged

Preloading tiles for camera animation #11328

merged 19 commits into from
Dec 22, 2021

Conversation

stepankuzmin
Copy link
Contributor

@stepankuzmin stepankuzmin commented Dec 3, 2021

Launch Checklist

This PR adds the preload option to Map#fitBounds, and Map#flyTo, allowing deferring animation until all tiles along the easing path are loaded.

map.flyTo({center: [0, 0], zoom: 9, preload: true});
const bbox = [[-79, 43], [-73, 45]];
map.fitBounds(bbox, {
  preload: true,
  padding: {top: 10, bottom:25, left: 15, right: 5}
});
fit-bounds-with-preload.mov
  • briefly describe the changes in this PR
  • include before/after visuals or gifs if this PR includes visual changes
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page
  • tagged @mapbox/gl-native if this PR includes shader changes or needs a native port
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
  • add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog></changelog>

@karimnaaji
Copy link
Contributor

karimnaaji commented Dec 3, 2021

That's a nice approach! One thing I noticed if we analyze the video is that at:

  • 1.36 seconds: map.fitBounds gets gets entered
  • 3.98 seconds: The animation starts

That means we're left with no visual feedback for about 2.5 seconds after you interacted with the map. I assume this was on a fast network, so possibly the optimistic case. Could you try the same with simulated slow mode or on mobile network?

I wonder if this idea could also be extended to improve the default use case to benefit more users (when not setting explicit option to preload): Kick flyTo animation as the user clicks, always. In the meantime that the animation starts and is running, sort tiles for the order in which they would be traversed throughout the animation and download them as soon as possible (in the sorted order). For this specific case, with an animation of 10 seconds, we may have tiles downloaded during the first 2 seconds, then ensure that 80% of remaining animation duration has all potential tiles loaded.

The above wouldn't always ensure that all tiles are always downloaded, but redistributes the work we know we will have to do ahead of time.

Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

Per discussion in a call: first of all, this looks awesome, and I'd use it extensively myself — especially for demo video recordings through our debug/video-export.html page. Especially impressive how little code it takes for a substantial feature!

The main thing it's missing right now is being able to separate the two stages — preloading and animation. One option that comes to mind is to make it so that if preload: true is set, flyTo will only do preloading without animating afterwards — then you can do await map.once('idle') to wait until all tiles load, do something (e.g. start the video recording), and then call flyTo again with the same options but preload: false so that the actual animation happens.

Also, if we're going with this route, for consistency, all other camera movement methods like easeTo will need to support the option. Maybe we could even enable the "preload bbox" use case by supporting it for instant methods like jumpTo.

@stepankuzmin
Copy link
Contributor Author

I've separated the preloading and animation stages. The preload option will emulate linear animation with 15fps and load all affected tiles in the animation order in all source caches without triggering animation or map render.

You can use preload options with any animation method:

map.flyTo({center: [-119.5375, 37.7425], preload: true});
map.easeTo({center: [-119.5375, 37.7425], preload: true});
map.fitBounds([[-122.4943, 37.7247], [-122.34, 37.82]], {padding: 20, preload: true});

After the map finishes preloading, it becomes idle so that you can start the animation:

map.flyTo({center: [-119.5375, 37.7425], preload: true});
await map.once('idle');
map.flyTo({center: [-119.5375, 37.7425]});

You can also achieve preloading tiles for an arbitrary region with:

map.jumpTo({center: [-119.5375, 37.7425], zoom: 8, preload: true});
await map.once('idle');
map.jumpTo({center: [-119.5375, 37.7425], zoom: 8});

This PR also introduces optimization for flyTo/easeTo — it starts preloading tiles for the destination view as soon as possible.

@stepankuzmin stepankuzmin changed the title WIP add preload option to fitBounds and flyTo Preloading tiles for camera animation Dec 13, 2021
@karimnaaji
Copy link
Contributor

I've separated the preloading and animation stages.
This PR also introduces optimization for flyTo/easeTo — it starts preloading tiles for the destination view as soon as possible.

@stepankuzmin 👍 I think those last changes are great and give quite some flexibility.

One concern about the API, when I read

map.flyTo({center: [-119.5375, 37.7425], preload: true});

I expect the original behavior of this PR (e.g. executes a flyTo after preloading, or while preloading?), instead of the current behavior of only preloading without executing the flyTo. That seems ambiguous.

Instead, what do you think of the following:

map.preloadTiles(Action.FlyTo, ...)
await map.once('idle');
map.flyTo(...)

This has two advantages:

  • Makes a clear intent from the callee that we are preloading data only, and not executing any map action
  • Allows further extensions, if needed:
    • How much memory we should be preloading, at most
    • How much time should be spent preloading, at most

Any thoughts on that?

@mourner
Copy link
Member

mourner commented Dec 16, 2021

map.preloadTiles(Action.FlyTo, ...)

This feels a little Java-esque to me, and has no similarly structured methods elsewhere in the API. How about keeping the option by rename to preloadOnly to clarify intent? We could expand this later by supplying an options object instead of true, similar to how we handle interaction options.

@karimnaaji
Copy link
Contributor

preloadOnly works with me, the option Object extension is a good way to go forward if we ever need to.

@stepankuzmin
Copy link
Contributor Author

I like the idea of the renaming option to preloadOnly 👍

Also, I was thinking about sequential animations — when you move from one view to another and so on. We can create an API that allows users to define a list of animations between different views. Say jump to the initial animation point, then fly to the next point, ease to another, etc. What do you think about this approach as a next step? I think we can consider this after the initial implementation.

@mourner
Copy link
Member

mourner commented Dec 16, 2021

We can create an API that allows users to define a list of animations between different views.

Yeah, I thought about it too. Maybe a method to return a Camera-like object where you can chain methods? E.g.:

await map.preload().flyTo(...).easeTo(...).zoomTo(...).finish();

src/ui/camera.js Outdated Show resolved Hide resolved
Copy link
Member

@mourner mourner 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 awesome overall. My only reservations are that we should make sure:

  • That CPU performance isn't hit by cloning so many transforms at once (e.g. it recalculates all matrices on clone).
  • That there will be no race conditions due to different methods of loading tiles. E.g. if we ask to preload the last transform, then the animation finishes before preload finishes, do we request the same tiles in parallel?

src/ui/camera.js Outdated Show resolved Hide resolved
src/ui/camera.js Outdated Show resolved Hide resolved
src/ui/camera.js Outdated Show resolved Hide resolved
src/ui/camera.js Outdated Show resolved Hide resolved
Copy link
Contributor

@karimnaaji karimnaaji left a comment

Choose a reason for hiding this comment

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

Looks good. Could you add that new page as part of release testing in https://github.com/mapbox/mapbox-gl-js/tree/main/test/release before merging? It'll help testing that feature when we do the release.

@stepankuzmin
Copy link
Contributor Author

Just a tiny sneak peek of the feature. I used "fast 3G" throttling to make the distinction clearer

iphone-x-fast-3g-comparison-with-full-preload-small.mp4

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

Successfully merging this pull request may close these issues.

3 participants