fire sourcedata when source metadata and tiles are loaded #4347

mollymerp · 2017-03-01T04:43:13Z

This PR does a couple things in order to make the sourcedata event more useful for users:

for vector tile sources, we currently fire sourcedata when the tilejson loads, but that is not really useful for users, so this PR will fire the event when the source + all tiles in the current viewport are loaded
we used to call SourceCache#reload on the sourcedata event, but that was causing all vector tiles to be loaded twice on initial load of the source, and because this is only meant to reload geojson sources after setData and image/video/canvas sources after setCoordinates I created a new event (😬) to handle this called source.update

lucaswoj · 2017-03-01T23:23:16Z

for vector tile sources, we currently fire sourcedata when the tilejson loads, but that is not really useful for users, so this PR will fire the event when the source + all tiles in the current viewport are loaded

I would like to advocate for sourcedata to be fired whenever a source changes or loads new data (not just when it finishes loading) on the basis that

it stays true to the heritage of the data event, indicating that some data has arrived
it would allow us to simplify the API by removing the tiledata event
it creates a clear path towards custom sources which are always updating (for example due to animations or real time data) or untiled
it avoids stepping on the toes of dataend (perhaps we need a sourcedataend event)

we used to call SourceCache#reload on the sourcedata event, but that was causing all vector tiles to be loaded twice on initial load of the source, and because this is only meant to reload geojson sources after setData and image/video/canvas sources after setCoordinates I created a new event (😬) to handle this called source.update

Can we eliminate the need for this new event by calling SourceCache#reload directly within setData and setCoordinates?

anandthakker · 2017-03-02T18:27:30Z

Can we eliminate the need for this new event by calling SourceCache#reload directly within setData and setCoordinates?

@lucaswoj as of now, sources are not 'aware of' / provided a reference to their containing SourceCache. We could either refactor to add this dependency now, or temporarily keep the (private) source.update event and wait for the upcoming SourceCache refactor.

…ties

mollymerp · 2017-03-06T05:19:03Z

src/source/image_source.js

@@ -115,7 +114,8 @@ class ImageSource extends Evented {
        centerCoord.column = Math.round(centerCoord.column);
        centerCoord.row = Math.round(centerCoord.row);

-        this.minzoom = this.maxzoom = centerCoord.zoom;
+        // TODO this is making tests fail... what is it for?
+        // this.minzoom = this.maxzoom = centerCoord.zoom;


@lbud would love to know what this line is for. for some reason it's behaving in unexpected ways with the new eventing system and making the canvas source tests fail 😭

@lbud might be able to say more, but my recollection from a while back is that this had to do with 'tricking' the source cache into always requesting the 'right' tile from the ImageSource.

Yeah, I think @anandthakker is right — because of #3186 (👺) we have to trick the source cache into thinking there's only one appropriate tile to ever request for image + image-inheriting sources. 👀 …

mollymerp · 2017-03-06T05:26:25Z

src/source/source_cache.js

@@ -475,6 +477,7 @@ class SourceCache extends Evented {
            clearTimeout(this._timers[id]);
            this._timers[id] = undefined;
        }
+        // TODO should we keep this??
        this._source.fire('data', { tile: tile, coord: tile.coord, dataType: 'tile' });


@lucaswoj @anandthakker do you think we need to fire an event when a tile is removed now that tiledata is ⚰️? I feel like this isn't useful, but let me know if you disagree.

I agree that this event isn't useful -- if we're not using it anywhere internally, then I'm all for removing it.

What might be useful someday in the future would be a way for users to listen for changes to the list of currently renderable/rendered tile coordinates -- but this doesn't really provide that, and anyway that's a super non-urgent nice-to-have.

mollymerp · 2017-03-06T05:37:20Z

In its current state this PR:

removes source.load and replaces it with a sourcedata with a metadata property on the MapEventData
removes tiledata and replaces it with a sourcedata event with tile information in the MapDataEvent
adds an update property on the event data that triggers a reload of the tiles for use with mutable sources (eg GeoJSONSource#setData and ImageSource#setCoordinates) in order to prevent double loading of sources when they're initially added to the map

TODO:

figure out whats going on with the canvas source + tests
figure out what to do with the remaining vestiges of the tiledata event (on removeTile and redoPlacement)

cc @lucaswoj @anandthakker @lbud

anandthakker · 2017-03-06T13:56:18Z

src/source/source_cache.js

+
+            // for sources with mutable data, this event fires when the underlying data
+            // to a source is changed. (i.e. GeoJSONSource#setData and ImageSource#serCoordinates)
+            if (this._sourceLoaded && e.dataType==="source" && e.update) {


This is great ❤️

Notes/questions:

We should update the internal documentation here to reflect these changes to the Source interface contract.

Is e.metadata too generic a name? I'm not even sure if I like this idea yet, but what about something like e.sourceDataType == 'metadata' and e.sourceDataType == 'update'? (Basically having sourceDataType be analogous to dataType, but specific to source data events.)

Is there still a possibility of a race condition here for vector sources? We need SourceCache#update() to be called after the TileJSON has loaded. This currently happens either here (for events with e.update) or via Style#_updateSources(). I'm imagining this problematic timeline:

map.addSource('mysource', ...) (plus map.addLayer() using mysource) => requests TileJSON, marks map._sourceDirty.

Next render frame fires; map._sourcesDirty being true leads to style._updateSources()... but since the TileJSON is still in flight, we don't get a SourceCache#update() for mysource.

TileJSON lands => VectorTileSource fires data with e.metadata => mysource's SourceCache marks itself _sourceLoaded.

But now, the actual tiles for mysource are not loaded until something else causes _sourcesDirty flag to be set.

anandthakker

Looks good to me. Couple of internal-docs-related comments. Other than that, I'll just vote one last time for replacing 'update' with 'content', as that feels like it's clearer / more parallel with 'metadata'. Approving anyway though so your call 😄

anandthakker · 2017-03-08T00:27:09Z

src/source/source.js

@@ -50,7 +50,7 @@ exports.setType = function (name, type) {
 * @param {string} options.type The source type, matching the value of `name` used in {@link Style#addSourceType}.
 * @param {Dispatcher} dispatcher A {@link Dispatcher} instance, which can be used to send messages to the workers.
 *
- * @fires load to indicate source data has been loaded, so that it's okay to call `loadTile`


Small addition: "@fires data with sourceDataType: 'metadata' to indicate..." -- since that's what source cache now requires of the event fired by Source.

anandthakker · 2017-03-08T00:31:50Z

src/source/geojson_source.js

+            // `update` is included here to prevent a race condition where `Style#_updateSources` is called
+            // before the TileJSON arrives. this makes sure the tiles needed are loaded once TileJSON arrives
+            // ref: https://github.com/mapbox/mapbox-gl-js/pull/4347#discussion_r104418088
+            this.fire('data', {dataType: 'source', sourceDataType: 'metadata'});


In this case (GeoJSONSource), the term 'metadata' is slightly odd, since there isn't really any metadata for geojson sources. As such, it might be more helpful to replace the comment about update in favor of a comment reminding us that we're firing metadata to notify the source cache that it's 👍 to ask us for tiles.

(I.e., reading this source, I think it's metadata that might be confusing; reading the vector/raster sources, it's update that might be confusing)

oh good catch! this should be in vector_tile_source and not here.

anandthakker · 2017-03-08T00:40:54Z

test/unit/source/vector_tile_source.test.js

-        source.on('data', t.end);
+        source.on('data', (e)=>{
+            if (e.sourceDataType === 'metadata') t.end();
+        });


Should this test be checking for the 'update' (or 'content') event, rather than 'metadata'?

mollymerp · 2017-03-08T03:11:34Z

benchmark	master `5fa31ef`	source-event `94c35d6`
map-load	450 ms	86 ms
style-load	120 ms	116 ms
buffer	900 ms	899 ms
fps	60 fps	60 fps
frame-duration	4.2 ms, 0% > 16ms	4.3 ms, 0% > 16ms
query-point	0.71 ms	0.74 ms
query-box	57.15 ms	56.11 ms
geojson-setdata-small	5 ms	1 ms
geojson-setdata-large	92 ms	93 ms

🕐 benchmarks for posterity 🕙

* source sorcery ✨ * fix render tests 🎨 * fix unit tests 🔧 * refactor to make everything dependent on 'data' event with new properties * change language, updates tests * use sourceDataType event property 📆 * fixy tests ✅ * 'update' 👉 'content', update tests, docs 📝

mollymerp added the under development label Mar 1, 2017

mollymerp force-pushed the source-event branch from 45dae5e to a1f38ae Compare March 2, 2017 03:38

lucaswoj added the release blocker ⛔ label Mar 3, 2017

Molly Lloyd added 4 commits March 5, 2017 19:06

source sorcery ✨

e58fecf

fix render tests 🎨

1973eb5

fix unit tests 🔧

17cda5c

refactor to make everything dependent on 'data' event with new proper…

799779c

…ties

mollymerp force-pushed the source-event branch from a1f38ae to 799779c Compare March 6, 2017 05:07

mollymerp commented Mar 6, 2017

View reviewed changes

anandthakker reviewed Mar 6, 2017

View reviewed changes

Molly Lloyd added 3 commits March 6, 2017 15:48

change language, updates tests

89ae98b

use sourceDataType event property 📆

3ccc7e0

fixy tests ✅

c19a117

anandthakker approved these changes Mar 8, 2017

View reviewed changes

mollymerp force-pushed the source-event branch 2 times, most recently from c4ca6f4 to 41157c5 Compare March 8, 2017 02:28

'update' 👉 'content', update tests, docs 📝

94c35d6

mollymerp force-pushed the source-event branch from 41157c5 to 94c35d6 Compare March 8, 2017 02:35

mollymerp merged commit 77fc72b into master Mar 8, 2017

mollymerp deleted the source-event branch March 8, 2017 06:40

This was referenced Mar 14, 2017

Event cleanup master ticket #4253

Open

map.loaded() returns false if tiles error #4425

Closed

attribution_control._updateAttribution causing Style Recalculation and Layout #4447

Closed

mollymerp mentioned this pull request May 8, 2017

Collision boxes don't appear until you move map #4647

Closed

mollymerp mentioned this pull request May 9, 2017

showCollisionBoxes fix #4689

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fire sourcedata when source metadata and tiles are loaded #4347

fire sourcedata when source metadata and tiles are loaded #4347

mollymerp commented Mar 1, 2017

lucaswoj commented Mar 1, 2017

anandthakker commented Mar 2, 2017

mollymerp Mar 6, 2017

anandthakker Mar 6, 2017

lbud Mar 6, 2017 •

edited

Loading

mollymerp Mar 6, 2017

anandthakker Mar 6, 2017

mollymerp commented Mar 6, 2017

anandthakker Mar 6, 2017

anandthakker left a comment

anandthakker Mar 8, 2017

anandthakker Mar 8, 2017

mollymerp Mar 8, 2017

anandthakker Mar 8, 2017

mollymerp commented Mar 8, 2017

fire sourcedata when source metadata and tiles are loaded #4347

fire sourcedata when source metadata and tiles are loaded #4347

Conversation

mollymerp commented Mar 1, 2017

lucaswoj commented Mar 1, 2017

anandthakker commented Mar 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbud Mar 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mollymerp commented Mar 6, 2017

Choose a reason for hiding this comment

anandthakker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mollymerp commented Mar 8, 2017

lbud Mar 6, 2017 •

edited

Loading