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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add "renderstable" event #2471

Closed
wants to merge 2 commits into from
Closed

Add "renderstable" event #2471

wants to merge 2 commits into from

Conversation

lucaswoj
Copy link
Contributor

@lucaswoj lucaswoj commented Apr 19, 2016

This pull request isn't quite ready. I am publishing now to get some 馃憖 on the proposal and implementation.

Mapbox GL JS downloads many resources (glyphs, sprites, tiles, and styles) over the network. Giving our developers the right primitives to understand the state of these resources and respond to changes in that state is a hard problem.

To that end, this PR adds the data and dataend events:

  • data event a resource has been loaded, changed, or removed.
  • dataend event all resources for the current viewport have been loaded. Fired at most once per frame.

This PR also renames Map#loaded to isDataStable, to disambiguate the concept of "data stable" (all resources for the current viewport have been loaded) from "loaded" (all resources for the FIRST viewport have been loaded).

We could do more to simplify the existing events infrastructure. Once this pull request is merged, we should consider deprecating some granular *.add, *.remove, *.load, and *.change events.

fixes #1715

cc @ansis @jfirebaugh @mourner @tmcw @scothis

@mourner
Copy link
Member

mourner commented Apr 20, 2016

TypeError: this.sprite.isDataStable is not a function

@lucaswoj
Copy link
Contributor Author

@mourner Sorry about that! Fixed now.

@jfirebaugh
Copy link
Contributor

For the data event, I propose that it be fired exactly once for each load, modification, or removal of a distinct resource, and that the event object contain information about the resource -- at a minimum, the resource type and action. Since the proposal is to remove type- and action-specific events like tile.load, this would ensure that what you can currently do with the existing events, you can also do with the all-purpose data event.

@lucaswoj
Copy link
Contributor Author

lucaswoj commented Apr 20, 2016

fun state diagram

screen shot 2016-04-20 at 2 30 23 pm

EDIT: New diagram, first one was incorrect

@lucaswoj lucaswoj changed the title Add "data" and "dataend" events Add "renderstable" event Apr 20, 2016
@lucaswoj
Copy link
Contributor Author

lucaswoj commented Apr 20, 2016

I have modified my approach, adding a single new event: renderstable. This event is fired whenever the value of Map#isRenderStable() changes from false to true.

Map#isRenderStable() (formerly Map#loaded) returns true iff every resource needed to render the current viewport has been loaded.

@lucaswoj
Copy link
Contributor Author

馃憖 @jfirebaugh @mourner @ansis

@jfirebaugh
Copy link
Contributor

Last time we discussed this, the consensus was that this event should be called dataend. I think dataend is a better name for the behavior you've implemented. I would expect renderstable not to be fired if there's an active gesture, easing animation, or style transition, and these changes don't implement that.

@jfirebaugh
Copy link
Contributor

Hmm, actually, I think I might be misremembering our conversation.

@lucaswoj
Copy link
Contributor Author

Next Stems

  • unrename isRenderStable
  • rename renderstable to renderend
  • add loadend event

@jfirebaugh
Copy link
Contributor

Yeah, so I'm reversing my earlier proposal of having "render" in the event name somewhere. Two reasons:

  • I think we should reserve names like renderstable or renderend for the case where the map isn't going to render any more frames until perturbed. Such an event should take into consideration animated easing, property transitions, and fading behaviors.
  • On Friday there was talk about this event being fired "iff queryRenderedFeatures changes", which is what led me to suggest the renderend name. But the event implemented here isn't fired on small pans that don't load new data, so "iff queryRenderedFeatures changes" isn't actually its definition.

I'm not sure "iff queryRenderedFeatures changes" is the right definition anyway -- it could be pretty complex to implement that. The simpler event that's implemented here is "all data for the current viewport is loaded". I think this is a good, useful event. I'm going to go back to calling it dataend for now.

With dataend, the sidebar can still be updated on small pans that don't load new data, by combining event handlers:

map.on('dataend', renderSidebar);
map.on('moveend', function () {
    if (map.isFullyLoaded()) {
       renderSidebar();
    }
});

What else can cause queryRenderedFeatures to change besides data loading and the map moving? Changing style properties, for one. The user can handle that by calling renderSidebar after making relevant style property changes. Anything else?

@jfirebaugh
Copy link
Contributor

A thought on naming: conceptually, this could be considered a family of three events:

  • An event that is fired once, for the lifetime of the map (currently named load).
  • An event that is fired once per perturbation that causes new data to be loaded (new event).
  • An event that is fired any number of times, whenever a new "chunk" of data has been loaded (currently various *.add / *.load / *.change events, proposed to be unified in the future).

We don't necessarily have to have symmetry in the three names, but it would be nice if that fell out naturally.

@lucaswoj
Copy link
Contributor Author

  • An event that is fired once, for the lifetime of the map (currently named load).
  • An event that is fired once per perturbation that causes new data to be loaded (new event).
  • An event that is fired any number of times, whenever a new "chunk" of data has been loaded (currently various *.add / *.load / *.change events, proposed to be unified in the future).

Do these events correlate with my proposal of data, dataend and load events?

@lucaswoj
Copy link
Contributor Author

I am closing this PR because it has been inactive for a long time. The branch isn't going anywhere so please keep working on this feature and re-open the PR when it is ready!

Let me know if you have any questions.

@lucaswoj lucaswoj closed this Jul 20, 2016
@jfirebaugh jfirebaugh deleted the dataend-1715 branch February 3, 2017 18:02
@GertjanVandenKeybus
Copy link

Any updates on this feature? I need to upload a map with HTTPS and I need to know when to actually start my data upload.

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.

Add data, datastart and dataend events
4 participants