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

Indexed vector sources #1377

wants to merge 28 commits into from

Conversation

@tmcw
Copy link
Contributor

tmcw commented Jul 19, 2015

Style, context, polish

  • @chelm is there any git history for the initial development on this branch?
  • demo/mapbox-gl-dev.js and demo/streets-mobile.json probably shouldn't be checked in
  • esri- files follow a dashed naming convention that diverges from the underscored naming convention of the rest of the lib
  • esri- code needs JSDoc strings like the rest of the lib
  • esri-tile-pyramid differs more in functionality than proprietariness to the TilePyramid code, and it is exposed via GL styles as indexedVector: should it be titled IndexedTilePyramid instead?
  • The new tile pyramid contains a lot of code verbatim from the existing one: parts in common should be unified by inheritance or modularization.

indexedVector tech

  • Are there any benchmarks or tests that have informed your decision to have an indexed tile pyramid? I can imagine there being a performance benefit, but would prefer concrete numbers to point to. Also, is the avoidance of 404s currently commented-out?

merge blockers

  • Tests
  • vector-tile-js dependency needs open-source release Esri#1 (comment)
  • mapbox-gl-style-spec dependency needs open source release

High-level there are three outcomes:

  1. indexedVector is a performance optimization. If it can't be confirmed as an improvement, then we may not accept it.
  2. If indexedVector's performance wins are clear-cut then it's possible we'll accept it as a default and integrate it into v8 rather than having two ways to load vector tiles
  3. For routes in the middle, we'll help get this branch landed

/cc @chelm @jcardonadcdev @ycabon

@mourner mourner added the in progress label Jul 19, 2015

module.exports = TilePyramid;

function TilePyramid(options) {

This comment has been minimized.

@tmcw

tmcw Jul 19, 2015 Author Contributor

This and the rest of the new functionality needs JSDoc documentation information to match the rest of mapbox-gl-js.

This comment has been minimized.

@tmcw

tmcw Jul 19, 2015 Author Contributor

Could this be re-titled IndexedTilePyramid to match its difference in functionality?

This comment has been minimized.

@chelm

chelm Jul 20, 2015

I've removed the concept of "indexedVector" for now, and all new files (esri-*) and datasources that were originally in this branch/PR are gone. In its place I've modified 5 files (in each very subtly). Will call each change out separately below.


module.exports = VectorTileSource;

function VectorTileSource(options) {

This comment has been minimized.

@tmcw

tmcw Jul 19, 2015 Author Contributor

Similarly to the Pyramid, this needs to be renamed, if for no other reason to appear different than the existing VectorTileSource class in tracebacks.

This comment has been minimized.

@chelm

chelm Jul 20, 2015

yup gone. all the esri-* files were not meant to be PR'd.

@@ -0,0 +1,102 @@
'use strict';

var util = require('mapbox-gl/js/util/util');

This comment has been minimized.

@tmcw

tmcw Jul 19, 2015 Author Contributor

Why are these includes of the library name rather than relative paths like '../js/util/util'? The latter is the case with the rest of the library as well as node.js customs.

This comment has been minimized.

@chelm

chelm Jul 20, 2015

Ahh yes, the way the code was written and built was done outside of the actual code base, as a sort of wrapper. This file is now removed.

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

This comment has been minimized.

@tmcw

tmcw Jul 19, 2015 Author Contributor

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.

This comment has been minimized.

@chelm

chelm Jul 20, 2015

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

};

// request the tile parentID if it exists
if (tile.parentId) {

This comment has been minimized.

@tmcw

tmcw Jul 19, 2015 Author Contributor

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

This comment has been minimized.

@chelm

chelm Jul 20, 2015

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

@chelm
Copy link

chelm commented Jul 20, 2015

@tmcw Thanks for starting this. The intention of checking the original code in (all the esri-* files) was to have a temp reference to how the original code was changed. I'll remove it since what I've done here is actually remove the concept of an "indexedVector" and have changed the behavior of the existing "vector" data sources.

I'll explain the changes better with line notes, and will sync the PR to remove unneeded cruft.

@@ -32,7 +33,22 @@ exports._loadTileJSON = function(options) {
redoPlacement: this._redoTilePlacement ? this._redoTilePlacement.bind(this) : undefined
});

this.fire('load');
// if index is defined, fetch the index json, then extend the pyramid
if (tileJSON.index) {

This comment has been minimized.

@chelm

chelm Jul 20, 2015

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.

@@ -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;

This comment has been minimized.

@chelm

chelm Jul 20, 2015

make sure the index is accessible on the tile pyramid

@@ -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);

This comment has been minimized.

@chelm

chelm Jul 20, 2015

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...

var yPos = tilePos.y & ((1 << dz) - 1);

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

This comment has been minimized.

@chelm

chelm Jul 20, 2015

Passing dz, and x/y position offset to the worker_tile.parse method. I'd like a cleaner way to do this, passing 7 params to a function is no bueno.

for (var i = 0; i < layer.length; i++) {
var feature = layer.feature(i);
//MOB
feature.dz = dz;

This comment has been minimized.

@chelm

chelm Jul 20, 2015

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).

@chelm
Copy link

chelm commented Jul 28, 2015

Just to comment on the status here: The team inside Esri is focusing on cooking up some benchmarks on using a tile index across our stack. This will help us understand the impact of the index on tile generation (less tiles) and the impact at render time (data clipping vs requesting/rendering very small tiles). It may be some time still, but everyone is eager to keep this moving forward.

@ycabon ycabon force-pushed the Esri:indexed-vector-sources branch from 56bb519 to 8140de4 Oct 5, 2015
@odoe
Copy link

odoe commented Oct 13, 2015

Here is a gist that discusses the concept of the Indexing of Vector Tiles and the justification for Clipping.

https://gist.github.com/odoe/ce6a150658526901ef27#file-vector-tile-pr-md

cc @tmcw @ycabon @jcardonadcdev

@ycabon
Copy link

ycabon commented Oct 13, 2015

@jcardonadcdev
Copy link

jcardonadcdev commented Oct 13, 2015

Tests used to compare processing times for indexed tile vs. non-indexed tile - https://github.com/jcardonadcdev/vt-performance-tests

@mcwhittemore mcwhittemore force-pushed the mapbox:master branch from 55b83f4 to ae44326 Dec 1, 2015
@lucaswoj lucaswoj force-pushed the mapbox:master branch from eb89e3a to 2c9a3dd Dec 18, 2015
@lucaswoj lucaswoj removed the in progress label Jul 28, 2016
@tmcw
Copy link
Contributor Author

tmcw commented Aug 15, 2016

The Esri side of this fork hasn't been updated since November 2015, and Mapbox GL JS has changed massively since then. It doesn't look like there are any active branches on the Esri project anymore.

Happy to take a look at an updated branch that resolves some of the initial issues outlined in this PR! Closing this specific PR as stale.

@tmcw tmcw closed this Aug 15, 2016
@chelm
Copy link

chelm commented Aug 15, 2016

😞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.