Add "data" event #3255

lucaswoj · 2016-09-26T21:16:01Z

This PR consolidates various undocumented data lifecycle events into the new data and dataloading events.

Having an event per resource type per possible mutation produced a complex taxonomy of events. Understanding, documenting, and using these events was difficult. Most users did not require the level of granularity exposed by this taxonomy. Some use cases required listening to many different events.

The unified data event is easy to understand, document, and use. Users who require more granularity may inspect the MapDataEvent object.

A MapDataEvent object is attached to the Map#data event.
The Map#data event is fired when any map data loads or changes. The types
of map data are:

'geoJSON': GeoJSON data associated with a geojson source.

'tileJSON': TileJSON metadata associated with a vector or raster source.

'style': The style used by the map

'sprite': The icons and patterns used by the style

'image': A image source's image and coordinates

'video': A video source's video and coordinates

'tile': A vector or raster tile

cc @jfirebaugh @lbud @mollymerp @mapsam @mourner

Launch Checklist

briefly describe the changes in this PR
document any changes to public APIs
post benchmark scores
manually test the debug page

lucaswoj · 2016-09-26T21:51:40Z

Benchmarks

map-load

master 24f2135: 119 ms
datastart 1ef4213: 105 ms

style-load

master 24f2135: 109 ms
datastart 1ef4213: 113 ms

buffer

master 24f2135: 961 ms
datastart 1ef4213: 941 ms

fps

master 24f2135: 60 fps
datastart 1ef4213: 60 fps

frame-duration

master 24f2135: 6.9 ms, 0% > 16ms
datastart 1ef4213: 7.2 ms, 1% > 16ms

query-point

master 24f2135: 1.04 ms
datastart 1ef4213: 1.12 ms

query-box

master 24f2135: 85.34 ms
datastart 1ef4213: 85.61 ms

geojson-setdata-small

master 24f2135: 13 ms
datastart 1ef4213: 14 ms

geojson-setdata-large

master 24f2135: 125 ms
datastart 1ef4213: 121 ms

jfirebaugh · 2016-09-27T17:38:43Z

Can you post a full rundown of the new taxonomy -- what are the valid combinations of event types and dataType values, and when are they emitted?

lucaswoj · 2016-09-27T18:28:55Z

Below is the type specification for the data and dataloading events:

interface DataEvent {
    type: 'dataloading' | 'data';
    dataType: (
        'geoJSON' |
        'tileJSON' |
        'style' |
        'sprite' |
        'image' |
        'video' |
        'tile'
    );
}

The Map#data event is fired when any map data loads or changes.

The Map#dataloading event is fired when any map data begins loading or changing asynchronously, to be followed by a data event when the operation is complete.

jfirebaugh · 2016-09-27T18:33:30Z

But there's also source.load and style.load still?

jfirebaugh · 2016-09-27T18:37:41Z

What's the rationale for having separate geoJSON, tileJSON, image, and video values for dataType, rather than a single source value? It seems like a source value would simplify the taxonomy and keep it consistent with the domain objects used in rest of the API and in the style specification.

lucaswoj · 2016-09-27T18:41:53Z

But there's also source.load and style.load still?

The plan is to have data subsume those events in a follow-up PR. See the plan in #1715 (comment)

I originally had planned to do so in this PR but the diff size was larger than I liked.

What's the rationale for having separate geoJSON, tileJSON, image, and video values for dataType, rather than a single source value? It seems like a source value would simplify the taxonomy and keep it consistent with the domain objects used in rest of the API and in the style specification.

The rationale was to have a dataType per specific network resource type. Merging geoJSON, tileJSON, image, and video into source is a good idea. I'll push a commit shortly.

jfirebaugh · 2016-09-27T18:39:03Z

js/source/image_source.js

@@ -53,6 +53,7 @@ function ImageSource(id, options, dispatcher) {

        this.image = image;
        this._loaded = true;
+        this.fire('data', {type: 'image'});


type ⇢ dataType

jfirebaugh · 2016-09-27T18:39:26Z

js/source/raster_tile_source.js

@@ -17,6 +17,7 @@ function RasterTileSource(id, options, dispatcher) {
            return this.fire('error', err);
        }
        util.extend(this, tileJSON);
+        this.fire('data', {type: 'tileJSON'});


type ⇢ dataType

jfirebaugh · 2016-09-27T18:42:00Z

bench/lib/set_data_perf.js

@@ -7,7 +7,9 @@ module.exports = function(source, numCalls, geojson, cb) {
    var startTime = null;
    var times = [];

-    source.on('tile.load', function tileCounter() {
+    source.on('data', function tileCounter(event) {
+        if (event.dataType !== 'tile') return;


Does this event binding need to distinguish between "tile loading" and "tile unloading"?

I think so. I will refactor to make this benchmark utility more robust.

jfirebaugh · 2016-09-27T18:42:51Z

js/style/style.js

-function Style(stylesheet, animationLoop, options) {
-    this.animationLoop = animationLoop || new AnimationLoop();
+function Style(stylesheet, map, options) {
+    this.map = map;


I've been trying to reduce or eliminate the dependency of other class on Map. The dependency here is required by Source#on{Add,Remove}(map) -- we should work towards eliminating those two methods.

Removing the dependency was my first instinct as well. Unfortunately it wasn't straightforward to do so and will require a standalone PR.

Even though the dependency is undesirable, it is a real dependency and should be passed explicitly and simply. As our code is architected, Sources need to know about the state of Camera (which is a mixin on Map). I see #2741 as the proper long-term fix.

jfirebaugh · 2016-09-27T18:47:44Z

js/ui/map.js

@@ -197,23 +197,11 @@ var Map = module.exports = function(options) {
        this.style.update(this._classes, {transition: false});
    });

-    this.on('style.change', function() {
+    this.on('data', function() {
        this._update(true);


Not every data event should trigger _update(true) -- that triggers updating and recalculating, which is necessary only in certain cases.

Good catch. Fortunately it looks like filtering on event.dataType === 'style' is equivalent to listening to the style.change event.

lucaswoj · 2016-09-27T20:21:38Z

All feedback addressed, ready for a second pass of 👀 @jfirebaugh

jfirebaugh · 2016-09-29T18:12:01Z

js/ui/control/attribution.js

-        map.on('source.load', this._update.bind(this));
-        map.on('source.change', this._update.bind(this));
-        map.on('source.remove', this._update.bind(this));
+        map.on('data', this._update.bind(this));


Add a dataType === 'source' guard in _update so that it doesn't mutate the DOM more frequently than it did before.

… events

lucaswoj · 2016-09-29T19:06:24Z

Thank you for the review @jfirebaugh 🙇

* source.change -> data * source.load -> sourceload & data * style.change -> data * style.load -> data, styleload * tile.add -> dataloading * tile.load -> data * tile.remove -> data * Unrename "source.load" and "style.load" * source.add & source.remove -> data * layer.add & layer.remove -> data * Remove isDataRemoved flag * Add documentation for "data" event * Fix setDataPerf benchmark utility * Minor doc fix * Add docs for dataloading event * merge 'sprite' and 'style' into 'style' * merge all several types into 'source' * Refactor "set data perf" benchmark utility * Ensure "Map#_update(true)" is only called after style mutations * Ensure Attribution#_update is only called for "data[dataType=source]" events * Attach Source, not SourceCache, to data events

strech345 · 2016-10-07T22:52:41Z

is there a 'unload' function?
i need the event which tile is unload.

lucaswoj · 2016-10-11T17:19:35Z

@strech345 Not right now. Defining what it means for a tile to become "unloaded" isn't easy. Is a tile unloaded

When it is no longer at the desired zoom level for the current viewport but still rendered?
When it is entirely outside of the current viewport?
When it is removed from the cache? How big is the cache? Which cache?
When it is reloaded?

strech345 · 2016-10-15T18:09:57Z

@lucaswoj it the same question like for the loading. So i think it should be a equivalent to the dataloading

jfirebaugh suggested changes Sep 27, 2016

View reviewed changes

lucaswoj force-pushed the datastart branch from 71378d9 to 7b5b93c Compare September 27, 2016 19:07

Lucas Wojciechowski added 19 commits September 27, 2016 13:18

source.change -> data

2106db6

source.load -> sourceload & data

24efa54

style.change -> data

3e4e80e

style.load -> data, styleload

fd04e20

tile.add -> dataloading

702df69

tile.load -> data

73bf845

tile.remove -> data

d44480d

Unrename "source.load" and "style.load"

0650f8d

source.add & source.remove -> data

58e0a6b

layer.add & layer.remove -> data

4abeca4

Remove isDataRemoved flag

04ddd41

Add documentation for "data" event

7e60cc6

Fix setDataPerf benchmark utility

56a1efc

Minor doc fix

506014d

Add docs for dataloading event

ef4f8b7

merge 'sprite' and 'style' into 'style'

8e9c10f

merge all several types into 'source'

6d71d52

Refactor "set data perf" benchmark utility

1ab2e44

Ensure "Map#_update(true)" is only called after style mutations

b7bd4fe

lucaswoj force-pushed the datastart branch from e09aeb7 to b7bd4fe Compare September 27, 2016 20:20

jfirebaugh suggested changes Sep 29, 2016

View reviewed changes

Lucas Wojciechowski added 2 commits September 29, 2016 11:51

Ensure Attribution#_update is only called for "data[dataType=source]"…

87a5f7b

… events

Attach Source, not SourceCache, to data events

5f5085e

jfirebaugh approved these changes Sep 29, 2016

View reviewed changes

lucaswoj merged commit 1d373b0 into master Sep 29, 2016

lucaswoj deleted the datastart branch September 29, 2016 19:06

jfirebaugh mentioned this pull request Oct 12, 2016

Eliminate Source#on{Add,Remove}(map) #3350

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "data" event #3255

Add "data" event #3255

lucaswoj commented Sep 26, 2016 •

edited

Loading

lucaswoj commented Sep 26, 2016

jfirebaugh commented Sep 27, 2016

lucaswoj commented Sep 27, 2016

jfirebaugh commented Sep 27, 2016

jfirebaugh commented Sep 27, 2016

lucaswoj commented Sep 27, 2016 •

edited

Loading

jfirebaugh Sep 27, 2016

lucaswoj Sep 27, 2016

jfirebaugh Sep 27, 2016

lucaswoj Sep 27, 2016

jfirebaugh Sep 27, 2016

lucaswoj Sep 27, 2016

jfirebaugh Sep 27, 2016 •

edited

Loading

lucaswoj Sep 27, 2016

jfirebaugh Sep 27, 2016

lucaswoj Sep 27, 2016

lucaswoj commented Sep 27, 2016

jfirebaugh Sep 29, 2016

lucaswoj Sep 29, 2016

lucaswoj commented Sep 29, 2016

strech345 commented Oct 7, 2016

lucaswoj commented Oct 11, 2016

strech345 commented Oct 15, 2016

Add "data" event #3255

Add "data" event #3255

Conversation

lucaswoj commented Sep 26, 2016 • edited Loading

Launch Checklist

lucaswoj commented Sep 26, 2016

Benchmarks

map-load

style-load

buffer

fps

frame-duration

query-point

query-box

geojson-setdata-small

geojson-setdata-large

jfirebaugh commented Sep 27, 2016

lucaswoj commented Sep 27, 2016

jfirebaugh commented Sep 27, 2016

jfirebaugh commented Sep 27, 2016

lucaswoj commented Sep 27, 2016 • edited Loading

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

Choose a reason for hiding this comment

jfirebaugh Sep 27, 2016 • edited Loading

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

lucaswoj commented Sep 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucaswoj commented Sep 29, 2016

strech345 commented Oct 7, 2016

lucaswoj commented Oct 11, 2016

strech345 commented Oct 15, 2016

lucaswoj commented Sep 26, 2016 •

edited

Loading

lucaswoj commented Sep 27, 2016 •

edited

Loading

jfirebaugh Sep 27, 2016 •

edited

Loading