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

Add "data" event #3255

Merged
merged 21 commits into from
Sep 29, 2016
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions bench/benchmarks/geojson_setdata_large.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,8 @@ module.exports = function() {

map.on('load', function() {
map = setupGeoJSONMap(map);
var source = map.getSource('geojson');

setDataPerf(source, 50, data, function(err, ms) {
setDataPerf(map.style.sources['geojson'], data, function(err, ms) {
if (err) return evented.fire('error', {error: err});
map.remove();
evented.fire('end', {message: formatNumber(ms) + ' ms', score: ms});
Expand Down
4 changes: 1 addition & 3 deletions bench/benchmarks/geojson_setdata_small.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,7 @@ module.exports = function() {
map.on('load', function() {
map = setupGeoJSONMap(map);

var source = map.getSource('geojson');

setDataPerf(source, 50, featureCollection, function(err, ms) {
setDataPerf(map.style.sources['geojson'], featureCollection, function(err, ms) {
map.remove();
if (err) return evented.fire('error', {error: err});
evented.fire('end', {message: formatNumber(ms) + ' ms', score: ms});
Expand Down
36 changes: 17 additions & 19 deletions bench/lib/set_data_perf.js
Original file line number Diff line number Diff line change
@@ -1,31 +1,29 @@
'use strict';

var NUM_TILES = 6;

module.exports = function(source, numCalls, geojson, cb) {
var tileCount = 0;
module.exports = function(sourceCache, data, callback) {
var sampleCount = 50;
var startTime = null;
var times = [];

source.on('tile.load', function tileCounter() {
tileCount++;
if (tileCount === NUM_TILES) {
tileCount = 0;
times.push(performance.now() - startTime);
var samples = [];

if (times.length < numCalls) {
sourceCache.on('data', function onData() {
if (sourceCache.loaded()) {
samples.push(performance.now() - startTime);
sourceCache.off('data', onData);
if (samples.length < sampleCount) {
startTime = performance.now();
source.setData(geojson);
sourceCache.clearTiles();
sourceCache.on('data', onData);
sourceCache.getSource().setData(data);
} else {
var avgTileTime = times.reduce(function (v, t) {
return v + t;
}, 0) / times.length;
source.off('tile.load', tileCounter);
cb(null, avgTileTime);
callback(null, average(samples));
}
}
});

startTime = performance.now();
source.setData(geojson);
sourceCache.getSource().setData(data);
};

function average(array) {
return array.reduce(function (sum, value) { return sum + value; }, 0) / array.length;
}
1 change: 1 addition & 0 deletions documentation.yml
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ toc:
- MapMouseEvent
- MapTouchEvent
- MapBoxZoomEvent
- MapDataEvent
- name: Types
description: |
These are types used in the documentation above to describe arguments,
Expand Down
3 changes: 2 additions & 1 deletion js/source/geojson_source.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ function GeoJSONSource(id, options, dispatcher) {
this.fire('error', {error: err});
return;
}
this.fire('data', {dataType: 'source'});
this.fire('source.load');
}.bind(this));
}
Expand Down Expand Up @@ -119,7 +120,7 @@ GeoJSONSource.prototype = util.inherit(Evented, /** @lends GeoJSONSource.prototy
if (err) {
return this.fire('error', { error: err });
}
this.fire('source.change');
this.fire('data', {dataType: 'source'});
}.bind(this));

return this;
Expand Down
3 changes: 2 additions & 1 deletion js/source/image_source.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ function ImageSource(id, options, dispatcher) {

this.image = image;
this._loaded = true;
this.fire('data', {dataType: 'source'});
this.fire('source.load');

if (this.map) {
Expand Down Expand Up @@ -106,7 +107,7 @@ ImageSource.prototype = util.inherit(Evented, /** @lends ImageSource.prototype *
Math.round((zoomedCoord.row - centerCoord.row) * EXTENT));
});

this.fire('source.change');
this.fire('data', {dataType: 'source'});
return this;
},

Expand Down
1 change: 1 addition & 0 deletions js/source/raster_tile_source.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ function RasterTileSource(id, options, dispatcher) {
return this.fire('error', err);
}
util.extend(this, tileJSON);
this.fire('data', {dataType: 'source'});
this.fire('source.load');
}.bind(this));
}
Expand Down
16 changes: 9 additions & 7 deletions js/source/source_cache.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,12 @@ function SourceCache(id, options, dispatcher) {
this._sourceErrored = true;
});

this.on('source.change', function() {
this.reload();
if (this.transform) {
this.update(this.transform, this.map && this.map.style.rasterFadeDuration);
this.on('data', function(event) {
if (this._sourceLoaded && event.dataType === 'source') {
this.reload();
if (this.transform) {
this.update(this.transform, this.map && this.map.style.rasterFadeDuration);
}
}
});

Expand Down Expand Up @@ -166,7 +168,7 @@ SourceCache.prototype = util.inherit(Evented, {

tile.source = this;
tile.timeAdded = new Date().getTime();
this._source.fire('tile.load', {tile: tile});
this._source.fire('data', {tile: tile, dataType: 'tile'});

// HACK this is nescessary to fix https://github.com/mapbox/mapbox-gl-js/issues/2986
if (this.map) this.map.painter.tileExtentVAO.vao = null;
Expand Down Expand Up @@ -407,7 +409,7 @@ SourceCache.prototype = util.inherit(Evented, {

tile.uses++;
this._tiles[coord.id] = tile;
this._source.fire('tile.add', {tile: tile});
this._source.fire('dataloading', {tile: tile, dataType: 'tile'});

return tile;
},
Expand All @@ -425,7 +427,7 @@ SourceCache.prototype = util.inherit(Evented, {

tile.uses--;
delete this._tiles[id];
this._source.fire('tile.remove', {tile: tile});
this._source.fire('data', {dataType: 'tile'});

if (tile.uses > 0)
return;
Expand Down
2 changes: 1 addition & 1 deletion js/source/tile.js
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ Tile.prototype = {

function done(_, data) {
this.reloadSymbolData(data, source.map.painter, source.map.style);
source.fire('tile.load', {tile: this});
source.fire('data', {tile: this, dataType: 'tile'});

// HACK this is nescessary to fix https://github.com/mapbox/mapbox-gl-js/issues/2986
if (source.map) source.map.painter.tileExtentVAO.vao = null;
Expand Down
1 change: 1 addition & 0 deletions js/source/vector_tile_source.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ function VectorTileSource(id, options, dispatcher) {
return;
}
util.extend(this, tileJSON);
this.fire('data', {dataType: 'source'});
this.fire('source.load');
}.bind(this));
}
Expand Down
3 changes: 2 additions & 1 deletion js/source/video_source.js
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ function VideoSource(id, options) {
this.setCoordinates(options.coordinates);
}

this.fire('data', {dataType: 'source'});
this.fire('source.load');
}.bind(this));
}
Expand Down Expand Up @@ -135,7 +136,7 @@ VideoSource.prototype = util.inherit(Evented, /** @lends VideoSource.prototype *
Math.round((zoomedCoord.row - centerCoord.row) * EXTENT));
});

this.fire('source.change');
this.fire('data', {dataType: 'source'});
return this;
},

Expand Down
6 changes: 3 additions & 3 deletions js/style/image_sprite.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ function ImageSprite(base) {
}

this.data = data;
if (this.img) this.fire('style.change');
if (this.img) this.fire('data', {dataType: 'style'});
}.bind(this));

ajax.getImage(normalizeURL(base, format, '.png'), function(err, img) {
Expand All @@ -41,7 +41,7 @@ function ImageSprite(base) {
}

this.img = img;
if (this.data) this.fire('style.change');
if (this.data) this.fire('data', {dataType: 'style'});
}.bind(this));
}

Expand All @@ -58,7 +58,7 @@ ImageSprite.prototype.loaded = function() {
ImageSprite.prototype.resize = function(/*gl*/) {
if (browser.devicePixelRatio > 1 !== this.retina) {
var newSprite = new ImageSprite(this.base);
newSprite.on('style.change', function() {
newSprite.on('data', function() {
this.img = newSprite.img;
this.data = newSprite.data;
this.retina = newSprite.retina;
Expand Down
17 changes: 8 additions & 9 deletions js/style/style.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,9 @@ var getWorkerPool = require('../global_worker_pool');

module.exports = Style;

function Style(stylesheet, animationLoop, options) {
this.animationLoop = animationLoop || new AnimationLoop();
function Style(stylesheet, map, options) {
this.map = map;
Copy link
Contributor

@jfirebaugh jfirebaugh Sep 27, 2016

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

this.animationLoop = (map && map.animationLoop) || new AnimationLoop();
this.dispatcher = new Dispatcher(getWorkerPool(), this);
this.spriteAtlas = new SpriteAtlas(1024, 1024);
this.lineAtlas = new LineAtlas(256, 512);
Expand Down Expand Up @@ -67,6 +68,7 @@ function Style(stylesheet, animationLoop, options) {

this.glyphSource = new GlyphSource(stylesheet.glyphs);
this._resolve();
this.fire('data', {dataType: 'style'});
this.fire('style.load');
}.bind(this);

Expand Down Expand Up @@ -300,7 +302,7 @@ Style.prototype = util.inherit(Evented, {
this._applyClasses(classes, options);

if (this._updates.changed) {
this.fire('style.change');
this.fire('data', {dataType: 'style'});
}

this._resetUpdates();
Expand Down Expand Up @@ -337,7 +339,7 @@ Style.prototype = util.inherit(Evented, {
source.style = this;
source.setEventedParent(this, {source: source});

this._updates.events.push(['source.add', {source: source}]);
if (source.onAdd) source.onAdd(this.map);
this._updates.changed = true;

return this;
Expand All @@ -361,7 +363,7 @@ Style.prototype = util.inherit(Evented, {
delete this._updates.sources[id];
source.setEventedParent(null);

this._updates.events.push(['source.remove', {source: source}]);
if (source.onRemove) source.onRemove(this.map);
this._updates.changed = true;

return this;
Expand All @@ -382,7 +384,6 @@ Style.prototype = util.inherit(Evented, {
* ID `before`, or appended if `before` is omitted.
* @param {StyleLayer|Object} layer
* @param {string=} before ID of an existing layer to insert before
* @fires layer.add
* @returns {Style} `this`
* @private
*/
Expand All @@ -408,7 +409,6 @@ Style.prototype = util.inherit(Evented, {
if (layer.source) {
this._updates.sources[layer.source] = true;
}
this._updates.events.push(['layer.add', {layer: layer}]);

return this.updateClasses(layer.id);
},
Expand Down Expand Up @@ -441,7 +441,6 @@ Style.prototype = util.inherit(Evented, {
this._order.splice(this._order.indexOf(id), 1);

this._updates.allLayers = true;
this._updates.events.push(['layer.remove', {layer: layer}]);
this._updates.changed = true;

return this;
Expand Down Expand Up @@ -718,7 +717,7 @@ Style.prototype = util.inherit(Evented, {
spriteAtlas.setSprite(sprite);
spriteAtlas.addIcons(params.icons, callback);
} else {
sprite.on('style.change', function() {
sprite.on('data', function() {
spriteAtlas.setSprite(sprite);
spriteAtlas.addIcons(params.icons, callback);
});
Expand Down
4 changes: 1 addition & 3 deletions js/ui/control/attribution.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,7 @@ Attribution.prototype = util.inherit(Control, {
container = this._container = DOM.create('div', className, map.getContainer());

this._update();
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));
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

map.on('moveend', this._updateEditLink.bind(this));

return container;
Expand Down
Loading