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

Indexed vector sources #1377

Closed
wants to merge 28 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
9b91ca7
add the originally changed files from Esri as esri-*.js
chelm Jul 17, 2015
1fa5f37
request an index file if its given then extend the tile pyramid with …
chelm Jul 17, 2015
2e90988
if pyramid has an index then find a tiles parentID in the index
chelm Jul 17, 2015
0caefd7
when loading tiles use the parentID tile for the url
chelm Jul 17, 2015
cea6ca4
when a parent ID is defined determine the dz and x/y offset of a tile…
chelm Jul 17, 2015
dbe8533
pass dz and x/y position down to the feature
chelm Jul 17, 2015
838048e
delinting; tests all pass
chelm Jul 17, 2015
8c93f73
fixing ref to parentID
chelm Jul 17, 2015
340c79a
adding the demo code to show the index working (but ps... its not wor…
chelm Jul 17, 2015
0d90c5d
adding the mb gl dev code to the demo
chelm Jul 17, 2015
051fffb
pointing to the dev js
chelm Jul 17, 2015
507fe72
when using an index missing tile request are replaced with requests f…
chelm Jul 19, 2015
53eb537
delinting tile_pyramid - no functions inside loops
chelm Jul 19, 2015
f3bc25b
removing unneeded code from the branch to make it easier to understand
chelm Jul 20, 2015
e6e7736
dont create a tile pyramid until we have the index
chelm Jul 20, 2015
b8c3092
adding tests for searching an index in the tile_pyramid
chelm Jul 20, 2015
a3b3371
better pyramid creation when using an index
chelm Jul 20, 2015
b7fd5d7
a bit cleaner indexSearch function
chelm Jul 20, 2015
a94d882
do not add empty object as parameter to buildPyramid if there is not …
jcardonadcdev Sep 16, 2015
8362c6d
check for null lines
jcardonadcdev Sep 17, 2015
4a854be
fix index search to return correct tile id
jcardonadcdev Sep 18, 2015
c5f91bd
only update tileExtent for clipped tiles
mbriat Sep 18, 2015
fa0e42a
account for coord.w
mbriat Sep 18, 2015
64b0916
modify tests for null geometries
mbriat Sep 18, 2015
bc54795
fixed parentId and tileExtent>4096
mbriat Sep 21, 2015
0a6807b
pulled child position calc to separate function
jcardonadcdev Sep 29, 2015
8140de4
check for null polygon geometry
jcardonadcdev Oct 5, 2015
6a1fee7
Merge commit '97244c9a2eb40bb18c92f1ddf17b8f54a52880b9' into indexed-…
ycabon Nov 1, 2015
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
5 changes: 4 additions & 1 deletion js/data/fill_bucket.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,10 @@ FillBucket.prototype.addFeatures = function() {
var features = this.features;
for (var i = 0; i < features.length; i++) {
var feature = features[i];
this.addFeature(feature.loadGeometry());
var geom = feature.loadGeometry();
if (geom) {
this.addFeature(geom);
}
}
};

Expand Down
4 changes: 3 additions & 1 deletion js/data/line_bucket.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ LineBucket.prototype.addFeatures = function() {
var features = this.features;
for (var i = 0; i < features.length; i++) {
var feature = features[i];
this.addFeature(feature.loadGeometry());
var geom = feature.loadGeometry();
if (geom)
this.addFeature(geom);
}
};

Expand Down
42 changes: 33 additions & 9 deletions js/source/source.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,10 @@ var TileCoord = require('./tile_coord');
var normalizeURL = require('../util/mapbox').normalizeSourceURL;

exports._loadTileJSON = function(options) {
var loaded = function(err, tileJSON) {
if (err) {
this.fire('error', {error: err});
return;
}

util.extend(this, util.pick(tileJSON,
['tiles', 'minzoom', 'maxzoom', 'attribution']));

var buildPyramid = function (index) {
this._pyramid = new TilePyramid({
index: index,
tileSize: this.tileSize,
cacheSize: 20,
minzoom: this.minzoom,
Expand All @@ -31,8 +25,34 @@ exports._loadTileJSON = function(options) {
remove: this._removeTile.bind(this),
redoPlacement: this._redoTilePlacement ? this._redoTilePlacement.bind(this) : undefined
});
}.bind(this);

var loaded = function(err, tileJSON) {
if (err) {
this.fire('error', {error: err});
return;
}

util.extend(this, util.pick(tileJSON,
['tiles', 'minzoom', 'maxzoom', 'attribution']));

// if index is defined, fetch the index json, then extend the pyramid
if (tileJSON.index) {
Copy link

Choose a reason for hiding this comment

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

This is the entry point for the difference in behavior. If an index exists (as a url) on the data source "tileJSON" the index gets requested. The returned index is added to the tile pyramid.

ajax.getJSON(normalizeURL(tileJSON.index), function (err, index) {
if (err) {
this.fire('error', {error: err});
return;
}

buildPyramid(index.index);
this.fire('load');

}.bind(this));
} else {
buildPyramid();
this.fire('load');
}

this.fire('load');
}.bind(this);

if (options.url) {
Expand Down Expand Up @@ -61,6 +81,10 @@ exports._renderTiles = function(layers, painter) {
// so calculate the matrix the maxzoom tile would use.
z = Math.min(z, this.maxzoom);

// leaf tiles and clipped tiles always use 4096
if (tile.tileExtent > 4096 || tile.parentId)
tile.tileExtent = 4096;

x += w * (1 << z);
tile.calculateMatrices(z, x, y, painter.transform, painter);

Expand Down
55 changes: 55 additions & 0 deletions js/source/tile_pyramid.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ function TilePyramid(options) {
this.maxzoom = options.maxzoom;
this.roundZoom = options.roundZoom;
this.reparseOverscaled = options.reparseOverscaled;
// esri/chelm
this.index = options.index;
Copy link

Choose a reason for hiding this comment

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

make sure the index is accessible on the tile pyramid


this._load = options.load;
this._abort = options.abort;
Expand Down Expand Up @@ -271,6 +273,10 @@ TilePyramid.prototype = {
var zoom = coord.z;
var overscaling = zoom > this.maxzoom ? Math.pow(2, zoom - this.maxzoom) : 1;
tile = new Tile(wrapped, this.tileSize * overscaling);
// esri/chelm
if (this.index) {
tile.parentId = this.indexSearch(coord.id);
Copy link

Choose a reason for hiding this comment

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

When a tile is added to the map, we search the pyramid's index for a "parentId". A parentId should only exist when a requested tile is not found in the tile index. The index is searched recursively until the next lowest zoom level tile is found.

After explaining that I think the name "parentId" should be changed to be a bit more representative of what it is...

}
this._load(tile);
}

Expand Down Expand Up @@ -372,5 +378,54 @@ TilePyramid.prototype = {
}

return result;
},

/**
* For a given tile id find its parent tile from the index
* @param {string|number} id tile id
* @returns {Object} tile
* @private
*/
indexSearch: function (id) {
var tile = TileCoord.fromID(id);

var ids = [id];

var parentTile = tile;
while (parentTile.z !== 0) {
parentTile = parentTile.parent();
ids.push(parentTile.id);
}

var cursor = this.index,
cursorId = ids.pop(),
index;

var pluckId = function (coord) {
return coord.id;
};

while (ids.length) {
id = ids.pop();
tile = TileCoord.fromID(cursorId);
index = tile.children(this.maxzoom).map(pluckId).indexOf(id);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this hot code? Seems like the creation of function, a parallel array, and then another o(n) indexOf search would be a potential perf problem.

Copy link

Choose a reason for hiding this comment

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

Inefficient code for sure, can be much cleaner. A single loop would do what we need.

if (cursor) {
if (cursor[index] === 0) {
cursorId = id;
break;
} else if (cursor[index] === 1) {
cursorId = id;
break;
} else {
cursorId = id;
cursor = cursor[index];
}
}
}

// don't return a parentId if we found the original tile
if (ids.length === 0) return null;

return cursorId;
}
};
9 changes: 8 additions & 1 deletion js/source/vector_tile_source.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
var util = require('../util/util');
var Evented = require('../util/evented');
var Source = require('./source');
var TileCoord = require('./tile_coord');

module.exports = VectorTileSource;

Expand Down Expand Up @@ -72,9 +73,15 @@ VectorTileSource.prototype = util.inherit(Evented, {
overscaling: overscaling,
angle: this.map.transform.angle,
pitch: this.map.transform.pitch,
collisionDebug: this.map.collisionDebug
collisionDebug: this.map.collisionDebug,
parentId: tile.parentId
};

// request the tile parentID if it exists
if (tile.parentId) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

afaict this is the first change I've seen that modifies existing behavior rather than extending it with a new source type.

Copy link

Choose a reason for hiding this comment

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

Right. If a tile has a parentId the url for the tile request needs to be used in place of the url built from tile.id

params.url = TileCoord.fromID(tile.parentId).url(this.tiles, this.maxzoom);
}

if (tile.workerID) {
this.dispatcher.send('reload tile', params, this._tileLoaded.bind(this, tile), tile.workerID);
} else {
Expand Down
27 changes: 26 additions & 1 deletion js/source/worker.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ var util = require('../util/util');
var ajax = require('../util/ajax');
var vt = require('vector-tile');
var Protobuf = require('pbf');
var TileCoord = require('./tile_coord');

var geojsonvt = require('geojson-vt');
var GeoJSONWrapper = require('./geojson_wrapper');
Expand Down Expand Up @@ -47,7 +48,18 @@ util.extend(Worker.prototype, {
if (err) return callback(err);

tile.data = new vt.VectorTile(new Protobuf(new Uint8Array(data)));
tile.parse(tile.data, this.layers, this.actor, callback);

// if a parentTile is defined it means the index is missing a tile for this coord
// here the difference between the requested tile and its indexed parent is found
// we pass the dz, x/y pos of the tile's relationship to its parent
if (params.parentId && tile.data.layers) {
var childPos = this.getChildPosition(params.coord.id, params.parentId);

// chelm - i'd prefer to not just tack on params here...
tile.parse(tile.data, this.layers, this.actor, callback, childPos.dz, childPos.xPos, childPos.yPos);
} else {
tile.parse(tile.data, this.layers, this.actor, callback);
}

this.loaded[source] = this.loaded[source] || {};
this.loaded[source][uid] = tile;
Expand Down Expand Up @@ -145,5 +157,18 @@ util.extend(Worker.prototype, {
} else {
callback(null, []);
}
},

'getChildPosition': function(childId, parentId) {
var tilePos = TileCoord.fromID(childId);
var parentPos = TileCoord.fromID(parentId);
var dz = tilePos.z - parentPos.z;
var xPos = tilePos.x & ((1 << dz) - 1);
var yPos = tilePos.y & ((1 << dz) - 1);
return {
dz: dz,
xPos: xPos,
yPos: yPos
};
}
});
14 changes: 10 additions & 4 deletions js/source/worker_tile.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ function WorkerTile(params) {
this.stacks = {};
}

WorkerTile.prototype.parse = function(data, layers, actor, callback) {
WorkerTile.prototype.parse = function(data, layers, actor, callback, dz, xPos, yPos) {

this.status = 'parsing';

Expand Down Expand Up @@ -104,16 +104,22 @@ WorkerTile.prototype.parse = function(data, layers, actor, callback) {
layer = data.layers[k];
if (!layer) continue;
if (layer.extent) extent = layer.extent;
sortLayerIntoBuckets(layer, bucketsBySourceLayer[k]);
sortLayerIntoBuckets(layer, bucketsBySourceLayer[k], dz, xPos, yPos);
}
} else {
// geojson
sortLayerIntoBuckets(data, bucketsBySourceLayer);
}

function sortLayerIntoBuckets(layer, buckets) {
function sortLayerIntoBuckets(layer, buckets, dz, xPos, yPos) {
for (var i = 0; i < layer.length; i++) {
var feature = layer.feature(i);
var feature = layer.feature(i);

// propagate clipped position in tile at the feature level
feature.dz = dz;
Copy link

Choose a reason for hiding this comment

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

All of the changes in worker_tile.js are passing on the dz, xpos, and ypos params. Ultimately these come down to Vector-tile-js and the parsing and rendering of geometries there. (separate PR).

feature.xPos = xPos;
feature.yPos = yPos;

for (var key in buckets) {
var bucket = buckets[key];
if (bucket.filter(feature)) {
Expand Down
3 changes: 3 additions & 0 deletions js/symbol/mergelines.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@ module.exports = function (features, textFeatures, geometries) {
var geom = geometries[k],
text = textFeatures[k];

if (!geom)
continue;

if (!text) {
add(k);
continue;
Expand Down
1 change: 1 addition & 0 deletions test/fixtures/index.json

Large diffs are not rendered by default.

29 changes: 29 additions & 0 deletions test/js/source/tile_pyramid.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ var Transform = require('../../../js/geo/transform');
var LngLat = require('../../../js/geo/lng_lat');
var Coordinate = require('../../../js/geo/coordinate');
var util = require('../../../js/util/util');
var path = require('path');
var fs = require('fs');

test('TilePyramid#coveringTiles', function(t) {
var pyramid = new TilePyramid({
Expand Down Expand Up @@ -373,6 +375,33 @@ test('TilePyramid#clearTiles', function(t) {
});
});

test('TilePyramid#indexSearch', function(t) {
t.test('finds the covering tile for a missing tile in an index index', function(t) {
var coord1 = TileCoord.fromID(81830);
var coord2 = TileCoord.fromID(83878);

var pyramid = createPyramid({
index: JSON.parse(fs.readFileSync(path.join(__dirname, '../../fixtures/index.json')).toString()).index
});

var tile1 = pyramid.addTile(coord1);
t.equal(tile1.parentId, 20421);

var tile2 = pyramid.addTile(coord2);
t.equal(tile2.parentId, null);

t.end();
});

t.test('indexSearch not called if tile pyramid does not have an index', function(t) {
var coord1 = TileCoord.fromID(81830);
var pyramid = createPyramid();
var tile1 = pyramid.addTile(coord1);
t.equal(tile1.parentId, undefined);
t.end();
});
});

test('TilePyramid#tilesIn', function (t) {
var transform = new Transform();
transform.width = 511;
Expand Down
9 changes: 9 additions & 0 deletions test/js/source/worker.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,15 @@ test('remove tile', function(t) {
});
});

test('overzoomed tile position', function(t) {
t.test('x, y pos calculated for overzoomed tile', function(t) {
var worker = new Worker(_self);
var ul = worker.getChildPosition(319335, 20421);
t.deepEqual(ul, { dz: 2, xPos: 3, yPos: 1 });
t.end();
});
});

test('after', function(t) {
server.close(t.end);
});