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

UTFGrid Support #3015

Merged
merged 17 commits into from Dec 12, 2014

Conversation

Projects
None yet
5 participants
@klokan
Member

klokan commented Dec 5, 2014

Implementation of the UTFGrid interaction according https://github.com/mapbox/utfgrid-spec

Done as ol.source.TileUTFGrid - as discussed in the ticket #922

A complete example of use is included - with displaying MBTiles tiles via TileJSON with the interactivity - showing flags for countries.
It is based on: https://www.mapbox.com/demo/visiblemap/

There is a possibility to use Mustache templates typically generated by TileMill.

Made by @petrsloup and contributed by Klokan Technologies GmbH.

Closes #922

@klokan klokan referenced this pull request Dec 5, 2014

Closed

UTFGrid Support #922

@elemoine

This comment has been minimized.

Show comment
Hide comment
@elemoine

elemoine Dec 5, 2014

Member

Awesome. I'm looking forward to reviewing this.

Member

elemoine commented Dec 5, 2014

Awesome. I'm looking forward to reviewing this.

olx.source.TileUTFGridOptions;
/**

This comment has been minimized.

@tschaub

tschaub Dec 5, 2014

Member

You'll need to also include documentation blocks for each of the option members (see other source option properties for examples).

@tschaub

tschaub Dec 5, 2014

Member

You'll need to also include documentation blocks for each of the option members (see other source option properties for examples).

Show outdated Hide outdated examples/tileutfgrid.html
<div id="tags">utfgrid, tileutfgrid, tilejson</div>
</div>
<div class="span4">
<div id="info" class="alert alert-success">

This comment has been minimized.

@tschaub

tschaub Dec 5, 2014

Member

I think we should stop using this (cargo culted) "alert alert-success" class. This example demonstrates some great new functionality, and I think it would be great to make the presentation a bit less clunky.

I think showing the flag and country name in situ (or near the pointer) would be nice. But I'm also a bit biased.

@tschaub

tschaub Dec 5, 2014

Member

I think we should stop using this (cargo culted) "alert alert-success" class. This example demonstrates some great new functionality, and I think it would be great to make the presentation a bit less clunky.

I think showing the flag and country name in situ (or near the pointer) would be nice. But I'm also a bit biased.

/**
* @inheritDoc
*/

This comment has been minimized.

@tschaub

tschaub Dec 5, 2014

Member

I don't think the lack of type checking is a major concern, but I'm wondering if there is a different way to handle this (instead of a public method that returns an instance of a private constructor - and yes, I know that is how the ol.source.TileDebug does it as well).

@tschaub

tschaub Dec 5, 2014

Member

I don't think the lack of type checking is a major concern, but I'm wondering if there is a different way to handle this (instead of a public method that returns an instance of a private constructor - and yes, I know that is how the ol.source.TileDebug does it as well).

This comment has been minimized.

@elemoine

elemoine Dec 5, 2014

Member

I personally fail to understand what you mean here.

@elemoine

elemoine Dec 5, 2014

Member

I personally fail to understand what you mean here.

Show outdated Hide outdated src/ol/source/tileutfgridsource.js
/**
* @private
* @type {?Array.<string>}

This comment has been minimized.

@tschaub

tschaub Dec 5, 2014

Member

All object types are nullable by default. This should be {Array.<string>} instead, right? Same below.

@tschaub

tschaub Dec 5, 2014

Member

All object types are nullable by default. This should be {Array.<string>} instead, right? Same below.

This comment has been minimized.

@elemoine

elemoine Dec 5, 2014

Member

True.

New compiler versions allow to use Array<string> but I would rather use the . for consistency reasons.

@elemoine

elemoine Dec 5, 2014

Member

True.

New compiler versions allow to use Array<string> but I would rather use the . for consistency reasons.

This comment has been minimized.

@tschaub

tschaub Dec 5, 2014

Member

In case it was not clear, I was suggesting {Array.<string>} instead of {?Array.<string>} (not anything to do with ..

@tschaub

tschaub Dec 5, 2014

Member

In case it was not clear, I was suggesting {Array.<string>} instead of {?Array.<string>} (not anything to do with ..

This comment has been minimized.

@elemoine

elemoine Dec 5, 2014

Member

I know. Sorry for creating confusion. I just took the opportunity to mention the new notation.

@elemoine

elemoine Dec 5, 2014

Member

I know. Sorry for creating confusion. I just took the opportunity to mention the new notation.

Show outdated Hide outdated src/ol/source/tileutfgridsource.js
}
var code = row.charCodeAt(Math.floor(xRelative * row.length));
if (code >= 93) code--;

This comment has been minimized.

@elemoine

elemoine Dec 5, 2014

Member

Please use curly braces.

@elemoine

elemoine Dec 5, 2014

Member

Please use curly braces.

Show outdated Hide outdated src/ol/source/tileutfgridsource.js
/**
* Synchronously returns data at given coordinate (if available).
* @param {ol.Coordinate} coordinate Coordinate.
* @return {Object|undefined}

This comment has been minimized.

@tschaub

tschaub Dec 5, 2014

Member

Seems like it would be nicer to deal with {Object} here. Then all callers would treat null as no data.

@tschaub

tschaub Dec 5, 2014

Member

Seems like it would be nicer to deal with {Object} here. Then all callers would treat null as no data.

This comment has been minimized.

@elemoine

elemoine Dec 5, 2014

Member

Agree.

@elemoine

elemoine Dec 5, 2014

Member

Agree.

Show outdated Hide outdated src/ol/source/tileutfgridsource.js
var code = row.charCodeAt(Math.floor(xRelative * row.length));
if (code >= 93) code--;
if (code >= 35) code--;

This comment has been minimized.

@tschaub

tschaub Dec 5, 2014

Member

These would be the first single-line conditionals in the library. Not a big deal, but I'd suggest dedicated blocks for consistency. We should really bring in jscs to enforce this sort of thing.

@tschaub

tschaub Dec 5, 2014

Member

These would be the first single-line conditionals in the library. Not a big deal, but I'd suggest dedicated blocks for consistency. We should really bring in jscs to enforce this sort of thing.

Show outdated Hide outdated examples/tileutfgrid.js
var flag = document.getElementById('flag');
var admin_name = document.getElementById('admin_name');
map.on('pointermove', function(evt) {

This comment has been minimized.

@elemoine

elemoine Dec 5, 2014

Member

pointermove events are triggered on touch devices while drag-panning the map. Not sure this is desired. You want to listen to mousemove instead, and to click events for touch devices. We have other examples where we use that already.

@elemoine

elemoine Dec 5, 2014

Member

pointermove events are triggered on touch devices while drag-panning the map. Not sure this is desired. You want to listen to mousemove instead, and to click events for touch devices. We have other examples where we use that already.

Show outdated Hide outdated examples/tileutfgrid.js
});
var flag = document.getElementById('flag');
var admin_name = document.getElementById('admin_name');

This comment has been minimized.

@elemoine

elemoine Dec 5, 2014

Member

We do use the underscore notation. Please use adminName here.

@elemoine

elemoine Dec 5, 2014

Member

We do use the underscore notation. Please use adminName here.

This comment has been minimized.

@elemoine

elemoine Dec 5, 2014

Member

I am talking about the name of the js variable here, not the id of the HTML element.

@elemoine

elemoine Dec 5, 2014

Member

I am talking about the name of the js variable here, not the id of the HTML element.

Show outdated Hide outdated src/ol/source/tileutfgridsource.js
* @protected
* @type {ol.TileCache}
*/
this.tileCache = new ol.TileCache();

This comment has been minimized.

@elemoine

elemoine Dec 5, 2014

Member

Any reason for tileUrlFunction and tileCache for being @protected rather than @private?

@elemoine

elemoine Dec 5, 2014

Member

Any reason for tileUrlFunction and tileCache for being @protected rather than @private?

Show outdated Hide outdated src/ol/source/tileutfgridsource.js
/**
* @protected
* @type {ol.TileCache}

This comment has been minimized.

@elemoine

elemoine Dec 5, 2014

Member

You could use {!ol.TileCache} here to make sure that tileCacheis never set to null.

@elemoine

elemoine Dec 5, 2014

Member

You could use {!ol.TileCache} here to make sure that tileCacheis never set to null.

goog.asserts.assert(tileJSON.scheme == 'xyz');
}
var minZoom = tileJSON.minzoom || 0;
var maxZoom = tileJSON.maxzoom || 22;

This comment has been minimized.

@tschaub

tschaub Dec 5, 2014

Member

Curious what others think about adding these as options.

@tschaub

tschaub Dec 5, 2014

Member

Curious what others think about adding these as options.

This comment has been minimized.

@elemoine

elemoine Dec 5, 2014

Member

Makes sense.

@elemoine

elemoine Dec 5, 2014

Member

Makes sense.

This comment has been minimized.

@petrsloup

petrsloup Dec 6, 2014

Member

So you propose adding minZoom, maxZoom constructor options that could be used to further clamp the zoom range specified in the json ?

Note: this part is identical to the ol.source.TileJSON (there are no options for this as well).

@petrsloup

petrsloup Dec 6, 2014

Member

So you propose adding minZoom, maxZoom constructor options that could be used to further clamp the zoom range specified in the json ?

Note: this part is identical to the ol.source.TileJSON (there are no options for this as well).

This comment has been minimized.

@petrsloup

petrsloup Dec 11, 2014

Member

@tschaub @elemoine Is this what you meant here? Please, let me know, so I can add the options.

@petrsloup

petrsloup Dec 11, 2014

Member

@tschaub @elemoine Is this what you meant here? Please, let me know, so I can add the options.

Show outdated Hide outdated src/ol/source/tileutfgridsource.js
return undefined;
}
var xRelative = (coordinate[0] - this.extent_[0]) /
(this.extent_[2] - this.extent_[0]);

This comment has been minimized.

@elemoine

elemoine Dec 5, 2014

Member

Very minor: you just need four spaces for the indent here.

@elemoine

elemoine Dec 5, 2014

Member

Very minor: you just need four spaces for the indent here.

Show outdated Hide outdated src/ol/source/tileutfgridsource.js
ol.source.TileUTFGridTile_.prototype.forDataAtCoordinate =
function(coordinate, f, opt_this, opt_noRequest) {
if (this.state == ol.TileState.IDLE && opt_noRequest !== true) {
this.listenOnce(goog.events.EventType.CHANGE, function(e) {

This comment has been minimized.

@elemoine

elemoine Dec 5, 2014

Member

If think we use goog.events.listenOnce every where else in the code.

@elemoine

elemoine Dec 5, 2014

Member

If think we use goog.events.listenOnce every where else in the code.

Show outdated Hide outdated src/ol/source/tileutfgridsource.js
/**
* @param {ol.Coordinate} coordinate Coordinate.
* @param {number} resolution Resolution.
* @param {function(this: T, (Object|undefined))} f Callback.

This comment has been minimized.

@elemoine

elemoine Dec 5, 2014

Member

We use callback rather than f else where in the lib.

@elemoine

elemoine Dec 5, 2014

Member

We use callback rather than f else where in the lib.

@tschaub

This comment has been minimized.

Show comment
Hide comment
@tschaub

tschaub Dec 5, 2014

Member

This is a really nice addition.

I'm interested to hear what others think about the public methods. The vector sources expose forEachFeature and getFeatures type methods. Both are synchronous (one type calls a callback synchronously and the other returns data).

Here, the forDataAtCoordinate* methods sometimes call the callback synchronously, sometimes call it asynchronously, and sometimes never call it at all (if tileGrid is ever null). First, I think these methods should always call the callback either synchronously or asynchronously. My vote would be always synchronously since that is what the feature based methods do. But I can imagine an argument for always asynchronously (e.g. why accept a callback at all, instead it would be more straightforward to return a data array).

Member

tschaub commented Dec 5, 2014

This is a really nice addition.

I'm interested to hear what others think about the public methods. The vector sources expose forEachFeature and getFeatures type methods. Both are synchronous (one type calls a callback synchronously and the other returns data).

Here, the forDataAtCoordinate* methods sometimes call the callback synchronously, sometimes call it asynchronously, and sometimes never call it at all (if tileGrid is ever null). First, I think these methods should always call the callback either synchronously or asynchronously. My vote would be always synchronously since that is what the feature based methods do. But I can imagine an argument for always asynchronously (e.g. why accept a callback at all, instead it would be more straightforward to return a data array).

Show outdated Hide outdated src/ol/source/tileutfgridsource.js
*/
ol.source.TileUTFGrid.prototype.forDataAtCoordinateAndResolution = function(
coordinate, resolution, f, opt_this, opt_noRequest) {
if (!goog.isNull(this.tileGrid)) {

This comment has been minimized.

@tschaub

tschaub Dec 5, 2014

Member

The callback should always be called. If we're going to go with an async callback, it should always be called asynchronously. If sync callback, it should always be called synchronously. In either case, we should be consistent.

@tschaub

tschaub Dec 5, 2014

Member

The callback should always be called. If we're going to go with an async callback, it should always be called asynchronously. If sync callback, it should always be called synchronously. In either case, we should be consistent.

@elemoine

This comment has been minimized.

Show comment
Hide comment
@elemoine

elemoine Dec 5, 2014

Member

My vote would be always synchronously since that is what the feature based methods do.

What if preemptive is false and the tile is not loaded yet? In that case the callback will be called asynchronously.

Member

elemoine commented Dec 5, 2014

My vote would be always synchronously since that is what the feature based methods do.

What if preemptive is false and the tile is not loaded yet? In that case the callback will be called asynchronously.

@tschaub

This comment has been minimized.

Show comment
Hide comment
@tschaub

tschaub Dec 5, 2014

Member

What if preemptive is false and the tile is not loaded yet? In that case the callback will be called asynchronously.

If we say we want synchronous callbacks, then the callback is called with null when there is no data or the tile has not loaded yet.

This would be the same behavior you would get with a vector source (e.g. if you call forEachFeature* on pointermove while data is loading).

Member

tschaub commented Dec 5, 2014

What if preemptive is false and the tile is not loaded yet? In that case the callback will be called asynchronously.

If we say we want synchronous callbacks, then the callback is called with null when there is no data or the tile has not loaded yet.

This would be the same behavior you would get with a vector source (e.g. if you call forEachFeature* on pointermove while data is loading).

@elemoine

This comment has been minimized.

Show comment
Hide comment
@elemoine

elemoine Dec 5, 2014

Member

If we say we want synchronous callbacks, then the callback is called with null when there is no data or the tile has not loaded yet.

This would be the same behavior you would get with a vector source (e.g. if you call forEachFeature* on pointermove while data is loading).

I see, and like the idea of going "always sync".

Member

elemoine commented Dec 5, 2014

If we say we want synchronous callbacks, then the callback is called with null when there is no data or the tile has not loaded yet.

This would be the same behavior you would get with a vector source (e.g. if you call forEachFeature* on pointermove while data is loading).

I see, and like the idea of going "always sync".

Show outdated Hide outdated src/ol/source/tileutfgridsource.js
}, false, this);
this.loadInternal_();
} else {
f.call(opt_this, this.getData(coordinate));

This comment has been minimized.

@elemoine

elemoine Dec 5, 2014

Member

Currently is there a way for the user (the callback) to distinguish "no data" from "request error"?

@elemoine

elemoine Dec 5, 2014

Member

Currently is there a way for the user (the callback) to distinguish "no data" from "request error"?

This comment has been minimized.

@petrsloup

petrsloup Dec 6, 2014

Member

No, there is currently no way to distinguish that. Maybe we could reintroduce the undefined value for this (undefined -- error; null -- no data).. ?

@petrsloup

petrsloup Dec 6, 2014

Member

No, there is currently no way to distinguish that. Maybe we could reintroduce the undefined value for this (undefined -- error; null -- no data).. ?

@petrsloup

This comment has been minimized.

Show comment
Hide comment
@petrsloup

petrsloup Dec 6, 2014

Member

Thanks for the comments. I fixed the issues related to the code style and improved the example.
I think there are now two points left to be discussed:

  • zoom level options #3015 (diff)
  • callback always sync vs. always async (+ always called)

I think it would be nice to have the callback synchronous (to be consitent with the forEachFeature*), but what should happen if preemptive is false -- what would cause the tile to load? The callback would be called with null, but the request would be created in the background?
How would the developer check if the data is finally ready? I think we need some callback for this anyway.
Also note, that the opt_noRequest parameter can be used to ensure that the callback is called synchronously.

Member

petrsloup commented Dec 6, 2014

Thanks for the comments. I fixed the issues related to the code style and improved the example.
I think there are now two points left to be discussed:

  • zoom level options #3015 (diff)
  • callback always sync vs. always async (+ always called)

I think it would be nice to have the callback synchronous (to be consitent with the forEachFeature*), but what should happen if preemptive is false -- what would cause the tile to load? The callback would be called with null, but the request would be created in the background?
How would the developer check if the data is finally ready? I think we need some callback for this anyway.
Also note, that the opt_noRequest parameter can be used to ensure that the callback is called synchronously.

@petrsloup

This comment has been minimized.

Show comment
Hide comment
@petrsloup

petrsloup Dec 11, 2014

Member

@tschaub @elemoine The current idea is to always call the callback ASAP, but I see the problem with sometimes sync, sometimes async.
In my opinion, if the callback would be called always synchronously, we would have to add some other way to let the user know the data is ready (especially for the preemptive: false case) -- maybe firing an event?
The "always async" approach is pretty straight-forward (probably using goog.async.nextTick), but I see the inconsistency problem (with the forEachFeature* methods).

Could you please comment so we can move towards some solution?

Member

petrsloup commented Dec 11, 2014

@tschaub @elemoine The current idea is to always call the callback ASAP, but I see the problem with sometimes sync, sometimes async.
In my opinion, if the callback would be called always synchronously, we would have to add some other way to let the user know the data is ready (especially for the preemptive: false case) -- maybe firing an event?
The "always async" approach is pretty straight-forward (probably using goog.async.nextTick), but I see the inconsistency problem (with the forEachFeature* methods).

Could you please comment so we can move towards some solution?

@ahocevar

This comment has been minimized.

Show comment
Hide comment
@ahocevar

ahocevar Dec 12, 2014

Member

I'd say the most straightforward way to solve this is to change opt_noRequest to opt_request. So the default is a synchronous callback, which is called with null if no data is loaded. If the user sets opt_request to true, then the callback will be asynchronous.

Regarding the zoom level options, I think these can be handled in a separate pull request.

Member

ahocevar commented Dec 12, 2014

I'd say the most straightforward way to solve this is to change opt_noRequest to opt_request. So the default is a synchronous callback, which is called with null if no data is loaded. If the user sets opt_request to true, then the callback will be asynchronous.

Regarding the zoom level options, I think these can be handled in a separate pull request.

@petrsloup

This comment has been minimized.

Show comment
Hide comment
@petrsloup

petrsloup Dec 12, 2014

Member

@ahocevar Thanks for the suggestion.

I modified the methods to be always sync by default (without creating any requests).
If opt_request is true, the callback is always async (even if the data is already loaded) and possibly creates request.

There is now minor problem with the default behavior:
Since both the preemptive option and the opt_request parameter are both false, no data will be ever loaded. (Callback always called synchronously with null.)

Member

petrsloup commented Dec 12, 2014

@ahocevar Thanks for the suggestion.

I modified the methods to be always sync by default (without creating any requests).
If opt_request is true, the callback is always async (even if the data is already loaded) and possibly creates request.

There is now minor problem with the default behavior:
Since both the preemptive option and the opt_request parameter are both false, no data will be ever loaded. (Callback always called synchronously with null.)

@ahocevar

This comment has been minimized.

Show comment
Hide comment
@ahocevar

ahocevar Dec 12, 2014

Member

I think it would make sense to make preemptive default to true. It would be good to add a bit more documentation to point out the relationship between the preemptive constructor option and the opt_request option for #forDataAtCoordinateAndResolution().

Member

ahocevar commented Dec 12, 2014

I think it would make sense to make preemptive default to true. It would be good to add a bit more documentation to point out the relationship between the preemptive constructor option and the opt_request option for #forDataAtCoordinateAndResolution().

@petrsloup

This comment has been minimized.

Show comment
Hide comment
@petrsloup

petrsloup Dec 12, 2014

Member

Yes, that is probably the best solution. The default behavior is now preemptive with callback of forDataAtCoordinateAndResolution being called synchronously. I also added a note to the documentation of the preemptive option about the opt_request parameter.

Please review if there are any more issues that need to be addressed.

Member

petrsloup commented Dec 12, 2014

Yes, that is probably the best solution. The default behavior is now preemptive with callback of forDataAtCoordinateAndResolution being called synchronously. I also added a note to the documentation of the preemptive option about the opt_request parameter.

Please review if there are any more issues that need to be addressed.

@ahocevar

This comment has been minimized.

Show comment
Hide comment
@ahocevar

ahocevar Dec 12, 2014

Member

Thanks @klokan for the contribution, and thanks @petrsloup for the continued effort on this. It really looks great now. All review comments from @elemoine, @tschaub and myself have been addressed. Other potential improvements, like making minZoom and maxZoom configurable, can be added later. And I have no fear that you or @klokan are going to disappear once this is merged, so I'm merging this now, and we look forward to any other contributions you can make!

Member

ahocevar commented Dec 12, 2014

Thanks @klokan for the contribution, and thanks @petrsloup for the continued effort on this. It really looks great now. All review comments from @elemoine, @tschaub and myself have been addressed. Other potential improvements, like making minZoom and maxZoom configurable, can be added later. And I have no fear that you or @klokan are going to disappear once this is merged, so I'm merging this now, and we look forward to any other contributions you can make!

ahocevar added a commit that referenced this pull request Dec 12, 2014

@ahocevar ahocevar merged commit 2c9fab2 into openlayers:master Dec 12, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@elemoine

This comment has been minimized.

Show comment
Hide comment
@elemoine

elemoine Dec 12, 2014

Member

Thanks @petrsloup for the contribution and thanks @ahocevar for the final review!

Member

elemoine commented Dec 12, 2014

Thanks @petrsloup for the contribution and thanks @ahocevar for the final review!

@petrsloup petrsloup deleted the klokantech:utfgrid branch Sep 17, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment