Skip to content

Commit

Permalink
Cleanup osm service notes and caches, remove state variable
Browse files Browse the repository at this point in the history 
This seems like a lot but the main things here are:
- remove the _loadingTiles "global" variable.  It was causing problems because
it was being checked from the callbacks, which are async.
- cleanup the caches
- use DOM API getElementsByTagName('id') to get note id
- set a lower tilezoom z13 for notes (meaning: fetch larger bounding boxes)
  • Loading branch information
@bhousel
bhousel committed Jun 30, 2018
1 parent ede5610 commit 229484a
Show file tree
Hide file tree
Showing 2 changed files with 89 additions and 102 deletions.
180 changes: 85 additions & 95 deletions modules/services/osm.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,21 +10,20 @@ import _uniq from 'lodash-es/uniq';

import rbush from 'rbush';

var _notesCache = {};

import { dispatch as d3_dispatch } from 'd3-dispatch';
import { xml as d3_xml } from 'd3-request';

import osmAuth from 'osm-auth';
import { JXON } from '../util/jxon';
import { d3geoTile as d3_geoTile } from '../lib/d3.geo.tile';
import { geoExtent } from '../geo';

import {
osmEntity,
osmNode,
osmNote,
osmRelation,
osmWay,
osmNote
osmWay
} from '../osm';

import { utilRebind, utilIdleWorker } from '../util';
Expand All @@ -41,17 +40,18 @@ var oauth = osmAuth({
});

var _blacklists = ['.*\.google(apis)?\..*/(vt|kh)[\?/].*([xyz]=.*){3}.*'];
var _tiles = { loaded: {}, inflight: {} };
var _tileCache = { loaded: {}, inflight: {} };
var _noteCache = { loaded: {}, inflight: {}, rtree: rbush() };
var _changeset = {};
var _entityCache = {};
var _noteTileCache = { loaded: {}, inflight: {}, rtree: rbush() };
var _seenEntity = {};

var _connectionID = 1;
var _tileZoom = 16;
var _noteZoom = 13;
var _rateLimitError;
var _userChangesets;
var _userDetails;
var _off;
var _loadingNotes = false;


function authLoading() {
Expand Down Expand Up @@ -146,7 +146,7 @@ var parsers = {
node: function nodeData(obj, uid) {
var attrs = obj.attributes;
return new osmNode({
id:uid,
id: uid,
visible: getVisible(attrs),
version: attrs.version.value,
changeset: attrs.changeset && attrs.changeset.value,
Expand Down Expand Up @@ -230,29 +230,19 @@ function parse(xml, callback, options) {

function parseChild(child) {
var parser = parsers[child.nodeName];
if (parser) {
if (!parser) return;

var uid;
var cache = _loadingNotes ? _notesCache : _entityCache;
var uid;
if (child.nodeName === 'note') {
uid = child.getElementsByTagName('id')[0].textContent;
} else {
uid = osmEntity.id.fromOSM(child.nodeName, child.attributes.id.value);
}

// if loading notes, get the note id
if (_loadingNotes) {
var childNodes = child.childNodes;
_forEach(childNodes, function(node) {
if (node.nodeName === 'id') {
uid = child.nodeName + node.innerHTML;
}
});
}
// otherwise, get the osmEntity id
else {
uid = osmEntity.id.fromOSM(child.nodeName, child.attributes.id.value);
}
if (options.cache && cache[uid]) {
return null;
}
return parser(child, uid);
if (options.cache && _seenEntity[uid]) {
return null;
}
return parser(child, uid);
}

utilIdleWorker(children, parseChild, callback);
Expand All @@ -271,17 +261,15 @@ export default {
_userChangesets = undefined;
_userDetails = undefined;
_rateLimitError = undefined;
_forEach(_tiles.inflight, abortRequest);

_forEach(_tileCache.inflight, abortRequest);
_forEach(_noteCache.inflight, abortRequest);
if (_changeset.inflight) abortRequest(_changeset.inflight);
_tiles = { loaded: {}, inflight: {} };
_changeset = {};
_entityCache = {};

if (_noteTileCache && _noteTileCache.inflight) {
_forEach(_noteTileCache.inflight, abortRequest);
}
_noteTileCache = { loaded: {}, inflight: {}, rtree: rbush() };
_notesCache = {};
_tileCache = { loaded: {}, inflight: {} };
_noteCache = { loaded: {}, inflight: {}, rtree: rbush() };
_changeset = {};
_seenEntity = {};

return this;
},
Expand Down Expand Up @@ -355,11 +343,7 @@ export default {
parse(xml, function (entities) {
if (options.cache) {
for (var i in entities) {
if (_loadingNotes) {
_notesCache[entities[i].id] = true;
} else {
_entityCache[entities[i].id] = true;
}
_seenEntity[entities[i].id] = true;
}
}
callback(null, entities);
Expand Down Expand Up @@ -656,56 +640,65 @@ export default {
},


loadTiles: function(projection, dimensions, callback) {
loadTiles: function(projection, dimensions, callback, loadingNotes) {
if (_off) return;

var that = this;

var cache = _loadingNotes ? _noteTileCache : _tiles;
// check if loading entities, or notes
var path, cache, tilezoom;
if (loadingNotes) {
path = '/api/0.6/notes?bbox=';
cache = _noteCache;
tilezoom = _noteZoom;
} else {
path = '/api/0.6/map?bbox=';
cache = _tileCache;
tilezoom = _tileZoom;
}

var s = projection.scale() * 2 * Math.PI;
var z = Math.max(Math.log(s) / Math.log(2) - 8, 0);
var ts = 256 * Math.pow(2, z - _tileZoom);
var ts = 256 * Math.pow(2, z - tilezoom);
var origin = [
s / 2 - projection.translate()[0],
s / 2 - projection.translate()[1]
];

var tiles = d3_geoTile()
.scaleExtent([_tileZoom, _tileZoom])
// what tiles cover the view
var tiler = d3_geoTile()
.scaleExtent([tilezoom, tilezoom])
.scale(s)
.size(dimensions)
.translate(projection.translate())()
.map(function(tile) {
var x = tile[0] * ts - origin[0];
var y = tile[1] * ts - origin[1];

return {
id: tile.toString(),
extent: geoExtent(
projection.invert([x, y + ts]),
projection.invert([x + ts, y]))
};
});
.translate(projection.translate());

var tiles = tiler().map(function(tile) {
var x = tile[0] * ts - origin[0];
var y = tile[1] * ts - origin[1];

return {
id: tile.toString(),
extent: geoExtent(
projection.invert([x, y + ts]),
projection.invert([x + ts, y])
)
};
});

_filter(cache.inflight, function(v, i) {
var wanted = _find(tiles, function(tile) {
return i === tile.id;
});
if (!wanted) delete cache.inflight[i];
var wanted = _find(tiles, function(tile) { return i === tile.id; });
if (!wanted) {
delete cache.inflight[i];
}
return !wanted;
}).map(abortRequest);

// check if loading entities, or notes
var path = _loadingNotes ? '/api/0.6/notes?bbox=' : '/api/0.6/map?bbox=';

tiles.forEach(function(tile) {
var id = tile.id;

if (cache.loaded[id] || cache.inflight[id]) return;

if (_isEmpty(cache.inflight)) {
dispatch.call('loading');
if (!loadingNotes && _isEmpty(cache.inflight)) {
dispatch.call('loading'); // start the spinner
}

cache.inflight[id] = that.loadFromAPI(
Expand All @@ -715,23 +708,19 @@ export default {
if (!err) {
cache.loaded[id] = true;
}
// NOTE: if zoom above min zoom & notes turned on before osm, osm won't render
// TODO: either pick this option, or the next one
if (_loadingNotes) {
cache.rtree.load(parsed);
}

// TODO: figure out how this callback should handle parsed results
if (callback) {
callback(err, _extend({ data: parsed }, tile));
}

if (_isEmpty(cache.inflight)) {
if (_loadingNotes) {
dispatch.call('loadedNotes');
if (loadingNotes) {
cache.rtree.load(parsed);
dispatch.call('loadedNotes');
} else {
if (callback) {
callback(err, _extend({ data: parsed }, tile));
}
if (_isEmpty(cache.inflight)) {
dispatch.call('loaded'); // stop the spinner
}
dispatch.call('loaded');
}

}
);
});
Expand Down Expand Up @@ -761,8 +750,8 @@ export default {


loadedTiles: function(_) {
if (!arguments.length) return _tiles.loaded;
_tiles.loaded = _;
if (!arguments.length) return _tileCache.loaded;
_tileCache.loaded = _;
return this;
},

Expand Down Expand Up @@ -800,30 +789,31 @@ export default {
return oauth.authenticate(done);
},

loadNotes: function(projection, dimensions, callback) {
var that = this;
_loadingNotes = true;
that.loadTiles(projection, dimensions, callback);

loadNotes: function(projection, dimensions) {
this.loadTiles(projection, dimensions, null, true); // true = loadingNotes
},

// TODO: possibly remove rtree & refactor caches

notes: function(projection) {
var viewport = projection.clipExtent();
var min = [viewport[0][0], viewport[1][1]];
var max = [viewport[1][0], viewport[0][1]];
var bbox = geoExtent(projection.invert(min), projection.invert(max)).bbox();

return _noteTileCache.rtree.search(bbox)
return _noteCache.rtree.search(bbox)
.map(function(d) { return d.data; });
},


loadedNotes: function(_) {
if (!arguments.length) return _noteTileCache.loaded;
_noteTileCache.loaded = _;
if (!arguments.length) return _noteCache.loaded;
_noteCache.loaded = _;
return this;
},


notesCache: function() {
return _noteTileCache;
return _noteCache;
}
};
11 changes: 4 additions & 7 deletions modules/svg/notes.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,15 +104,12 @@ export function svgNotes(projection, context, dispatch) {
}

function drawNotes(selection) {
var enabled = svgNotes.enabled,
service = getService();
var enabled = svgNotes.enabled;
var service = getService();

function dimensions() {
return [window.innerWidth, window.innerHeight];
}
function done() {
console.log('placeholder done within svg/notes.upload.done');
}

layer = selection.selectAll('.layer-notes')
.data(service ? [0] : []);
Expand All @@ -130,7 +127,7 @@ export function svgNotes(projection, context, dispatch) {
if (service && ~~context.map().zoom() >= minZoom) {
editOn();
update();
service.loadNotes(projection, dimensions(), done);
service.loadNotes(projection, dimensions());
} else {
editOff();
}
Expand All @@ -151,4 +148,4 @@ export function svgNotes(projection, context, dispatch) {

init();
return drawNotes;
}
}

2 comments on commit 229484a

@thomas-hervey
Copy link
Collaborator

Choose a reason for hiding this comment

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

@bhousel these are great cleaning updates. To clarify, the _seenEntity cache will hold all entities seen, notes and osm alike, correct?

@bhousel
Copy link
Member Author

@bhousel bhousel commented on 229484a Jul 2, 2018

Choose a reason for hiding this comment

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

@bhousel these are great cleaning updates. To clarify, the _seenEntity cache will hold all entities seen, notes and osm alike, correct?

Yeah - it can, although I was thinking about it more, and I think we can just skip adding notes to this cache.

The purpose of this _seenEntity cache is to avoid re-parsing an entity that we've already parsed - it gives iD a slight performance improvement. Large objects like roads or forests can span across several tiles, so as iD fetches more tiles, we'd end up parsing the same object again.

Notes don't really work this way, so there is no advantage to adding them to the cache.

Please sign in to comment.