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

[core, darwin, android] Add onDidBecomeIdle to MapObserver. #13513

Merged
merged 2 commits into from
Dec 11, 2018

Conversation

ChrisLoer
Copy link
Contributor

'didEnterIdle' fires whenever render completes and no repaint is scheduled.
Fixes issue #13469

Potential issues:

  • This logic doesn't know anything about animations that are being driven by the SDK instead of through the core transition logic. In practice I think this probably won't be a huge issue because if the SDK is running an animation, it will keep triggering placement, which will keep the renderer from entering idle.
  • Potentially confusing to add an extra event, since we've already got "didFinishRendering/fullyRendered" and "didFinishLoadingMap". However, I think the name "idle" is a pretty good description of this event, and matches up better with what users have expected to get from the other events.

cc @tobrun @julianrex @ansis

@tobrun
Copy link
Member

tobrun commented Dec 6, 2018

@ChrisLoer
did a quick test with Android SDK animators and idle is only fired at the end of the animation 🚀

@1ec5
Copy link
Contributor

1ec5 commented Dec 6, 2018

In practice I think this probably won't be a huge issue because if the SDK is running an animation, it will keep triggering placement, which will keep the renderer from entering idle.

On iOS, CADisplayLink continually rerenders the map 60 or 120 times per second by default, irrespective of whether any re-placement is necessary. Does that mean the renderer will never idle on iOS?

@1ec5
Copy link
Contributor

1ec5 commented Dec 6, 2018

Potentially confusing to add an extra event, since we've already got "didFinishRendering/fullyRendered" and "didFinishLoadingMap". However, I think the name "idle" is a pretty good description of this event, and matches up better with what users have expected to get from the other events.

Note that our implementation of -[MGLMapViewDelegate mapViewDidFinishRenderingMap:fullyRendered:] is incomplete precisely because it doesn’t get called when errors have prevented one or more tiles from loading: #1804. But that method doesn’t relate to animations anyways, just style and tile loading.

platform/macos/src/MGLMapViewDelegate.h Outdated Show resolved Hide resolved
platform/macos/src/MGLMapViewDelegate.h Outdated Show resolved Hide resolved
@ChrisLoer
Copy link
Contributor Author

Does that mean the renderer will never idle on iOS?

No, the mbgl core rendering itself idles. If it didn't, we'd be wasting a lot of energy!

One related thing I noticed in manual testing, however, is that when you touch the screen to begin an interaction, the first touch will trigger a paint before there's actually any movement (which would trigger placement/animation/etc.). So if you drag the screen to pan somewhere, you see two idle events -- the first when you initially touch the screen, and the second when the pan is done and all animations have finished. This is right from the point of view of the renderer, but may be surprising to someone listening to the idle event.

didEnterIdle fires whenever render completes and no repaint is scheduled.
@ChrisLoer ChrisLoer changed the title [core, darwin, android] Add onDidEnterIdle to MapObserver. [core, darwin, android] Add onDidBecomeIdle to MapObserver. Dec 10, 2018
@ChrisLoer
Copy link
Contributor Author

@1ec5 @tobrun I think this is ready for another round of review.

Copy link
Contributor

@julianrex julianrex left a comment

Choose a reason for hiding this comment

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

Thanks for adding this! LGTM

@ChrisLoer ChrisLoer merged commit 662a495 into master Dec 11, 2018
@ChrisLoer ChrisLoer deleted the idle-event branch December 11, 2018 19:46
@friedbunny friedbunny added iOS Mapbox Maps SDK for iOS Android Mapbox Maps SDK for Android Core The cross-platform C++ core, aka mbgl labels Jan 2, 2019
@friedbunny friedbunny added this to the release-java milestone Jan 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android Core The cross-platform C++ core, aka mbgl iOS Mapbox Maps SDK for iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants