Add hit tolerance parameter to ol.Map#forEachFeatureAtPixel #5995

Merged
merged 24 commits into from Dec 8, 2016

Projects

None yet

7 participants

@simonseyock
Contributor
simonseyock commented Oct 19, 2016 edited

TODO:

  • Write a test
  • Change the signature of map.forEachFeatureAtCoordinate and map.forEachFeatureAtPixel in the source code, the examples and the tests
  • Add a hitTolerance parameter to all internal forEachFeatureAtCoordinate methods of the renderer
  • Make ol.render.canvas.ReplayGroup.prototype.forEachFeatureAtCoordinate work with a hitContext of variable size
  • Implement the midpoint circle algorithm and checking only pixels inside the circle
  • Update documentation
  • hitTolerance should also be added to ol.Map#hasFeatureAtPixel.
  • example
  • caching
  • Test with retina screens (devicePixelRatio > 1)
  • Handle VectorTiles.
    Write a test for VectorTiles.
  • Update changelog.
  • Created test for ol.render.canvas.ReplayGroup#getCircleArray_

See discussion: #5862

@@ -276,7 +276,7 @@ ol.renderer.canvas.VectorTileLayer.prototype.createReplayGroup = function(tile,
/**
* @inheritDoc
*/
-ol.renderer.canvas.VectorTileLayer.prototype.forEachFeatureAtCoordinate = function(coordinate, frameState, callback, thisArg) {
+ol.renderer.canvas.VectorTileLayer.prototype.forEachFeatureAtCoordinate = function(coordinate, frameState, hitTolerance, callback, thisArg) {
var resolution = frameState.viewState.resolution;
var rotation = frameState.viewState.rotation;
var layer = this.getLayer();
@simonseyock
simonseyock Oct 19, 2016 Contributor

While updating this method i recognized that here is some code which probably needs to be updated, too. Is it sufficient to buffer up the extent by hitTolerance*resolution?

@simonseyock
simonseyock Oct 20, 2016 Contributor

After looking further into this, i realized there is more to do.

@simonseyock
simonseyock Oct 21, 2016 edited Contributor

As i don't know how to setup a test for this, i can't really work on this problem.
How could i set up a vectortilelayer containing only one feature which is very near to a corner of tile?
i tried this, but it does not work:

  // 1 x 1 pixel black icon
  var img = document.createElement('img');
  img.onload = function() {
    var target = document.createElement('div');
    target.style.width = '256px';
    target.style.height = '256px';
    document.body.appendChild(target);

    var tileGrid = ol.tilegrid.createXYZ();
    var zoom = 10;
    //choose coordinate at an edge of the tileGrid
    var coordinate = ol.extent.getTopLeft(tileGrid.getTileCoordExtent([zoom, 0, 0]));

    var map = new ol.Map({
      view: new ol.View({
        center: coordinate,
        zoom: zoom
      }),
      target: target
    });

    var feature = new ol.Feature(new ol.geom.Point(coordinate));
    feature.setStyle([new ol.style.Style({
      image: new ol.style.Icon({
        img : img,
        imgSize: [1, 1]
      })
    })]);

    var TileClass = function() {
      ol.VectorTile.apply(this, arguments);
      if (this.tileCoord[0] === zoom && this.tileCoord[1] === 0 && this.tileCoord[2] === 0) {
        this.setFeatures([feature]);
      }
      this.setState('loaded');
      this.setProjection(ol.proj.get('EPSG:3857'));
    };
    ol.inherits(TileClass, ol.VectorTile);

    var source = new ol.source.VectorTile({
      format: new ol.format.MVT(),
      tileClass: TileClass,
      tileGrid: tileGrid
    });
    var layer = new ol.layer.VectorTile({
      source: source
    });
    map.addLayer(layer);
    map.renderSync();

    var cb0 = sinon.spy();

    var pixel = map.getPixelFromCoordinate(coordinate);

    map.forEachFeatureAtPixel(pixel, cb0);
    expect(cb0.callCount).to.be(1);  // this already fails
    expect(cb0.firstCall.args[1]).to.be(layer);

Another question about the vectortilelayer for me is: how are features stored which are in multiple tiles? Could a simple buffering up the extent result in multiple calls of the callback for the same feature?

@tsauerwein
tsauerwein Oct 25, 2016 Member

Another question about the vectortilelayer for me is: how are features stored which are in multiple tiles? Could a simple buffering up the extent result in multiple calls of the callback for the same feature?

Yes, that could happen. We probably want to check that the callback is only called once for features with the same id?

@tsauerwein
tsauerwein Oct 25, 2016 Member

Maybe @ahocevar can help you with your other questions.

@simonseyock
simonseyock Oct 27, 2016 Contributor

@ahocevar do you have an idea how to setup a test for the hitTolerance used on a vectortilelayer? Important would be that the feature is in a corner of a tile.

src/ol/render/canvas/replaygroup.js
- coordinate, resolution, rotation, skippedFeaturesHash, callback) {
+ coordinate, resolution, rotation, hitTolerance, skippedFeaturesHash, callback) {
+
+ hitTolerance = Math.round(hitTolerance);
@simonseyock
simonseyock Oct 19, 2016 Contributor

an other possibility would be to assert it to be an integer, but i think this approach is easier to use.

src/ol/map.js
* @api stable
*/
-ol.Map.prototype.forEachFeatureAtPixel = function(pixel, callback, opt_this, opt_layerFilter, opt_this2) {
+ol.Map.prototype.forEachFeatureAtPixel = function(pixel, callback, opt_this, opt_options) {
@simonseyock
simonseyock Oct 19, 2016 Contributor

The compilers demands all optional parameters to appear at the end of the signature. To be concise with the other uses of opt_this i left it right behind the callback and inserted the opt_options behind that.

@simonseyock simonseyock changed the title from (WIP) Introduce hit tolerance parameter added to ol.Map#forEachFeatureAt to (WIP) Add hit tolerance parameter to ol.Map#forEachFeatureAt Oct 19, 2016
@simonseyock simonseyock changed the title from (WIP) Add hit tolerance parameter to ol.Map#forEachFeatureAt to (WIP) Add hit tolerance parameter to ol.Map#forEachFeatureAtPixel Oct 19, 2016
@simonseyock
Contributor

This development has been funded by KLAUS BENNDORF and GIS3W. The review has been funded by CAMPTOCAMP.

@ischas
Contributor
ischas commented Oct 20, 2016

The message above had to be added for @giohappy's company as proof of expenses for their customers.

@simonseyock
Contributor

signature of ol.Map.forEachFeatureAtPixel is now:
forEachFeatureAtPixel(pixel, callback, opt_this, opt_options) (reasons see above)
For the internal methods i added the hitTolerance after the frameState.

@tsauerwein
Member

I will reserve some time next week to do the review!

But already one comment: Changing the signature of ol.Map#forEachFeatureAtPixel breaks backward-compatibility. This would mean that the change would have to be described in the upgrade notes and everyone using forEachFeatureAtPixel with a layer filter would have to adapt the method call.

A method with 6 parameters is not optimal, but breaking backward-compatibility is something that we usually try to avoid (especially for a widely used functionality). What do others think?

@ischas
Contributor
ischas commented Oct 20, 2016

The optional options-object was proposed by @tschaub here: #5862 (comment)

@tsauerwein

This is looking quite good! 👍 I left a couple of comments.

And here are some more points:

  • hitTolerance should also be added to ol.Map#hasFeatureAtPixel.
  • I think it would be good to have an example demonstrating this feature. Either adapt an existing example or create a new one.
  • Test with retina screens (devicePixelRatio > 1)
  • Like I mentioned before, the signature change of ol.Map#forEachFeatureAtPixel must be mentioned in the changelogs.
  • Also mention in the docs that this feature is only supported for the canvas renderer and not WebGL.
externs/olx.js
+
+
+/**
+ * Layer filter function. Only layers which are visible and for which this function returns
externs/olx.js
+
+/**
+ * Value to use as `this` when executing `layerFilter`.
+ * @type {Object}
@tsauerwein
tsauerwein Oct 25, 2016 Member

This parameter is optional? The type should be Object|undefined in that case.

externs/olx.js
+
+
+/**
+ * Value of a radius in whichs area around the given coordinate features are
@tsauerwein
tsauerwein Oct 25, 2016 Member

Maybe: Hit-detection tolerance. Pixels inside the radius around the given position will be checked for features.?

externs/olx.js
+/**
+ * Value of a radius in whichs area around the given coordinate features are
+ * called.
+ * @type {number}
@tsauerwein
tsauerwein Oct 25, 2016 Member

It should be number|undefined.

externs/olx.js
+ * Value of a radius in whichs area around the given coordinate features are
+ * called.
+ * @type {number}
+ * @api stable
@tsauerwein
tsauerwein Oct 25, 2016 Member

Let's start with @api (experimental instead of stable). :)

@simonseyock
simonseyock Oct 25, 2016 Contributor

sounds reasonable ;)

src/ol/render/canvas/replaygroup.js
+ * @returns {Array.<Array.<Boolean>>} an array with marked circel points.
+ * @private
+ */
+ol.render.canvas.ReplayGroup.createPixelCircle_ = function(radius) {
@tsauerwein
tsauerwein Oct 25, 2016 Member

Do you think it would make sense to add a test for this function?

@simonseyock
simonseyock Oct 25, 2016 Contributor

I was thinking about it, but it is an almost 1:1 implementation of the standard algorithm. A test would be possible, though.

@simonseyock
simonseyock Nov 2, 2016 Contributor

I wrote a test for this (ol.test.render.canvas.ReplayGroup#getCircle_)

src/ol/render/canvas/replaygroup.js
+ var arr = new Array(arraySize);
+ for (var i = 0; i < arraySize; i++) {
+ arr[i] = new Array(arraySize);
+ for (var j = 0; j < arraySize; j++) {
@tsauerwein
tsauerwein Oct 25, 2016 Member

Is it needed to initialize the values with false? Would the default value undefined not be fine?

@simonseyock
simonseyock Oct 25, 2016 Contributor

good idea, that should work.

src/ol/render/canvas/replaygroup.js
- * @type {ol.Transform}
- */
- this.hitDetectionTransform_ = ol.transform.create();
+ function fillRowToMiddle(x, y) {
@tsauerwein
tsauerwein Oct 25, 2016 Member

Any reason to have this function inline? Don't you want to make it a private function like createPixelCircle_?

src/ol/render/canvas/replaygroup.js
@@ -101,6 +143,7 @@ ol.render.canvas.ReplayGroup.prototype.finish = function() {
* @param {ol.Coordinate} coordinate Coordinate.
* @param {number} resolution Resolution.
* @param {number} rotation Rotation.
+ * @param {number} hitTolerance hit tolerance.
@tsauerwein
tsauerwein Oct 25, 2016 Member

minor: Upper-case

src/ol/render/canvas/replaygroup.js
1 / resolution, -1 / resolution,
-rotation,
-coordinate[0], -coordinate[1]);
- var context = this.hitDetectionContext_;
- context.clearRect(0, 0, 1, 1);
+ var context = ol.dom.createCanvasContext2D(contextSize, contextSize);
@tsauerwein
tsauerwein Oct 25, 2016 Member

Creating a new canvas element for every call to forEachFeatureAtPixel/hasFeatureAtPixel sounds expensive. Could you maybe keep the context and resize when necessary?

src/ol/render/canvas/replaygroup.js
- var context = this.hitDetectionContext_;
- context.clearRect(0, 0, 1, 1);
+ var context = ol.dom.createCanvasContext2D(contextSize, contextSize);
+ context.clearRect(0, 0, contextSize, contextSize);
@tsauerwein
tsauerwein Oct 25, 2016 Member

Do you need to clear the context after having created a new one?

src/ol/render/canvas/replaygroup.js
+ if (hitTolerance === 0) {
+ mask = [[true]];
+ } else {
+ mask = ol.render.canvas.ReplayGroup.createPixelCircle_(hitTolerance);
@tsauerwein
tsauerwein Oct 25, 2016 Member

Could the mask be cached and be reused if the hit-tolerance is the same? To avoid creating new objects on every call (forEachFeatureAtCoordinate can be called many times when setting up a mouse-move listener).

src/ol/render/canvas/replaygroup.js
+ for (var i = 0; i < contextSize; i++) {
+ for (var j = 0; j < contextSize; j++) {
+ if (mask[i][j]) {
+ imageData = context.getImageData(i, j, i + 1, j + 1).data;
@tsauerwein
tsauerwein Oct 25, 2016 Member

You are calling getImageData for every entry in the mask? Performance-wise it would probably be better to call getImageData only once for the whole hit-detection canvas and do the checks on imageData.

src/ol/render/canvas/replaygroup.js
+ return result;
+ }
+ i = contextSize;
+ j = contextSize;
@tsauerwein
tsauerwein Oct 25, 2016 edited Member

Could you return undefined instead of setting i and j to contextSize to directly exit the function?

@@ -276,7 +276,7 @@ ol.renderer.canvas.VectorTileLayer.prototype.createReplayGroup = function(tile,
/**
* @inheritDoc
*/
-ol.renderer.canvas.VectorTileLayer.prototype.forEachFeatureAtCoordinate = function(coordinate, frameState, callback, thisArg) {
+ol.renderer.canvas.VectorTileLayer.prototype.forEachFeatureAtCoordinate = function(coordinate, frameState, hitTolerance, callback, thisArg) {
var resolution = frameState.viewState.resolution;
var rotation = frameState.viewState.rotation;
var layer = this.getLayer();
@tsauerwein
tsauerwein Oct 25, 2016 Member

Another question about the vectortilelayer for me is: how are features stored which are in multiple tiles? Could a simple buffering up the extent result in multiple calls of the callback for the same feature?

Yes, that could happen. We probably want to check that the callback is only called once for features with the same id?

@tsauerwein
tsauerwein Oct 25, 2016 Member

Maybe @ahocevar can help you with your other questions.

@simonseyock
Contributor
simonseyock commented Oct 25, 2016 edited

The replaygroup now caches the arrays. Both methods and the cache are static so the arrays can be used by all replaygroups.
I added a hitTolerance parameter to the select interaction and integrated the hitTolerance into the select example.

@simonseyock
Contributor
simonseyock commented Oct 26, 2016 edited

I forgot to mention i added hitTolerance to hasFeatureAtPixel. What about forEachLayerAtPixel?

@tsauerwein
Member

I forgot to mention i added hitTolerance to hasFeatureAtPixel. What about forEachLayerAtPixel?

forEachFeatureAtPixel is usually used in combination with hasFeatureAtPixel. That's why it was important for me that hasFeatureAtPixel has support for the tolerance.

forEachLayerAtPixel is something different, but if you also want to work on it as well, you are welcome! Maybe in a separate PR to not bloat this one.

@simonseyock
Contributor
simonseyock commented Oct 26, 2016 edited

Ok, i will leave it out for now, but maybe will work on it later if i find some spare time :)

changelog/upgrade-notes.md
+```
+
+This change is due to the introduction of the `hitTolerance` parameter which can be passed in via this `ol.AtPixelOptions` object, too.
+
@simonseyock
simonseyock Oct 28, 2016 Contributor

Is this the right place? (i will move it up after the rebase)

@marcjansen
marcjansen Oct 28, 2016 Member

This is the right file, yes. Add your notes to the very top, directly under

## Upgrade notes

Side note: Since this is such a big change which will affect a lot of our users, we should be very detailled here. Please include an example of the old code for ol.Map#forEachFeatureAtPixel and ol.Map#hasFeatureAtPixel and matching examples for the new way of specifying stuff

@simonseyock
Contributor

I recognized that i could test the pixelRatio with a unit-test.
What is the expected behaviour? Should a click on a device with a higher pixel ratio hit the same features as on a device with a lower pixel ratio or should it hit more features?

@simonseyock
Contributor
simonseyock commented Oct 28, 2016 edited

Or to say it with other words: should the hitTolerance depend on the physical distance on the screen or should that problem be left up to the developer to decide? (She/he could just use hitTolerance * devicePixelRatio if she/he would want that). I think that normally you would want that it stays the same physical distance.

@simonseyock
Contributor
simonseyock commented Nov 10, 2016 edited

I buffer the tile extent by hitTolerance * pointResolution in ol.renderer.canvas.VectorTileLayer#forEachFeatureAtCoordinate. I don't know if this works correctly.
As there is already a check in there that ensures that the callback is called only once for each feature i don't need to do that.

@simonseyock
Contributor

@tsauerwein: can you clarify how the pixelRatio should be handled? can you review this whole thing again (maybe after i implemented everything regarding the pixelRatio)?

@giohappy
Contributor

@tsauerwein could you review it again? we're funding this improvement and it is critical to us having it merged. it seems that pixelRatio handling (for retina displays) is the only remaining part to implement, isn'it?

@tsauerwein
Member
tsauerwein commented Nov 17, 2016 edited

@tsauerwein could you review it again? we're funding this improvement and it is critical to us having it merged.

Well, you are not providing funding for the company I work for. This week is quite busy for me, I will plan some time for next week.

@giohappy
Contributor

@tsauerwein I didn't meant to be controversial. We just needed a feedback ;)
Thanks for planning some time for it.

@giohappy
Contributor

great work @simonseyock !
I've just tested your branch with our webgis editing tools taking advantage of the new hit tolerance, and it works great from (Huawei) smartphone and (Samsung) tablet. I will do more tests with different devices. While pixelRatio won't be managed automatically, we will tweek the hit tolerance value by ourselves.

@tonio tonio referenced this pull request in Geoportail-Luxembourg/geoportailv3 Nov 18, 2016
Closed

sensibility on clic #1445

changelog/upgrade-notes.md
+
+Old syntax:
+```
+ map.forEachFeatureAtPixel(pixel, callback, callback_this, layerFilter_function, layerFilter_this);
@tsauerwein
tsauerwein Nov 23, 2016 Member

Minor: Prefer camel-case (callbackThis instead of callback_this, layerFilterFn instead of layerFilter_function, ...)?

externs/olx.js
@@ -303,6 +303,45 @@ olx.MapOptions.prototype.view;
/**
+ * Object literal with options for the forEachFeatureAtPixel and
+ * hasFeatureAtPixel methods.
@tsauerwein
tsauerwein Nov 23, 2016 Member

You could use: {@link ol.Map#forEachFeatureAtPixel} and {@link ol.Map#hasFeatureAtPixel}

src/ol/map.js
* @api stable
*/
-ol.Map.prototype.forEachFeatureAtPixel = function(pixel, callback, opt_this, opt_layerFilter, opt_this2) {
+ol.Map.prototype.forEachFeatureAtPixel = function(pixel, callback, opt_this,opt_options) {
@tsauerwein
tsauerwein Nov 23, 2016 Member

Doesn't the linter complain about the missing space before opt_options?

@simonseyock
simonseyock Nov 25, 2016 Contributor

Apparently not ... i added the space.

src/ol/render/canvas/replaygroup.js
+ * @type {Object.<number, Array.<Array.<(boolean|undefined)>>>}
+ * @private
+ */
+ol.render.canvas.ReplayGroup.circleArrayCache_ = {
@tsauerwein
tsauerwein Nov 23, 2016 Member

Maybe you could leave a short comment what this cache is used for.

src/ol/render/canvas/replaygroup.js
@@ -130,20 +204,30 @@ ol.render.canvas.ReplayGroup.prototype.forEachFeatureAtCoordinate = function(
ol.extent.buffer(hitExtent, resolution * this.renderBuffer_, hitExtent);
@tsauerwein
tsauerwein Nov 23, 2016 Member

I am wondering if you would also have to take the hitTolerance into account when calculating the hitExtent.

For example: If you have a point at 0,0 (in pixel coordinates) which has a symbol of 300x300 pixels, you would set the renderBuffer to 150px, so that the point can be selected properly. Even when clicking at 150,0 the point could be selected.
But now if you set the hitTolerance to 10px, you would expect to get the point if you click at 155,0. But the hitExtent would be 5,-150,305,150, so the point would not be considered for the hit detection.

Does that make sense? What do you think?

@simonseyock
simonseyock Nov 25, 2016 edited Contributor

Yes that is true, as all draw instructions not intersecting the hitContext are skipped in the replay. Normally this would not happen as the the renderBuffer defaults to 100px and therefore is much greater than the hitTolerance.

I add this.

Seeing here that the hitBuffer is only multiplied with the normal resolution i will apply the same method to the extent of the vectortiles.

@tsauerwein
Member

@simonseyock I agree with you that it would make sense to adapt the hit-tolerance depending on the pixel-ratio. And this also seems to be what @giohappy is proposing.

@simonseyock
Contributor

@tsauerwein: thanks for looking into this again

@simonseyock
Contributor

The hitTolerance is multiplied by the pixelRatio now.

As @giohappy mentioned they want to use the hitTolerance with the TranslateInteraction, i added a hitTolerance parameter to it.

For my part I have done everything I can think of for now.

@tsauerwein
Member

Thanks for your continuous effort and good work on this!

Before we merge, I would like to get a thumbs-up from the rest of the core-team about the proposed API change. As documented in the changelogs, all users that previously used a layer filter in ol.Map#forEachFeatureAtPixel or ol.Map#hasFeatureAtPixel will have to adapt their code.

The proposed syntax for these methods is:

    map.forEachFeatureAtPixel(pixel, callback, callbackThis, {
        layerFilter: layerFilterFn,
        layerFilterThis: layerFilterThis,
        hitTolerance: 10
    });

    map.hasFeatureAtPixel(pixel, {
        layerFilter: layerFilterFn,
        layerFilterThis: layerFilterThis,
        hitTolerance: 10
    });

@tschaub, you always have good ideas about API design. Does this look ok for you?

@ahocevar
Member
ahocevar commented Dec 2, 2016

My suggestion would be a slightly simpler API:

    map.forEachFeatureAtPixel(pixel, callback, {
        layerFilter: layerFilterFn,
        hitTolerance: 10
    });

    map.hasFeatureAtPixel(pixel, {
        layerFilter: layerFilterFn,
        hitTolerance: 10
    });

In other words: let's get rid of callbackThis and layerFilterThis. With Function.bind() being standard JavaScript now, it does not make sense to carry around this configurations.

@simonseyock
Contributor

@ahocevar: i agree that the this are not needed nowadays.

But this would be inconsistent with the other code in the library (just search for opt_this). So we might need to do a follow up PR that gets rid of opt_this in the whole source ... but this is quite much work.

@ahocevar
Member
ahocevar commented Dec 2, 2016 edited

@simonseyock In this case we introduce an object literal with options, and we do not have this options in any other options object literal. So no need to worry about inconsistencies.

@ischas
Contributor
ischas commented Dec 6, 2016

Any chance to get this into v3.20.0?

@ahocevar
Member
ahocevar commented Dec 6, 2016 edited

Any chance to get this into v3.20.0?

@ischas If the remaining review comments get addressed today (and the merge conflicts resolved), then there is a chance.

@ischas
Contributor
ischas commented Dec 6, 2016

No free ressources here to make it today. Looking forward to get it into v3.21.0 (and using our fork until then).

@ahocevar
Member
ahocevar commented Dec 6, 2016

I'm adding this ticket to the v3.20 milestone regardless. When the comments are addressed by the time the release is cut, it will be in, otherwise it will be moved to the v3.21 milestone.

@ahocevar ahocevar added this to the v3.20.0 milestone Dec 6, 2016
@simonseyock
Contributor

I simplified the api and left a short note in the upgrade-notes about function.bind with a link to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/bind

externs/olx.js
- * @type {Object|undefined}
- * @api stable
- */
-olx.AtPixelOptions.prototype.layerFilterThis;
@fredj
fredj Dec 7, 2016 Member

please also remove layerFilterThis in the typedef (line 309)

@simonseyock
simonseyock Dec 7, 2016 Contributor

done

@ischas
Contributor
ischas commented Dec 7, 2016

Once again, conflicts generated by another merge. @simonseyock: Please resolve the conflicts and remove the "(WIP)" since the only missing point is the VectorTiles-test which we are not able to provide (so maybe you should strike this out in the todo list). This can be a task for a follow-up pull request of someone who is familar with VectorTiles. At last, please ping the involved core team members after resolving the conflicts to get this in before this becomes a never ending story (and before you have to change your user name to Sisyphus ;-) ).

@ahocevar

Because it was mentioned that you cannot provide tests for vector tiles, I took brief look at the vector tile renderer changes. And found one thing that might not be correct.

@@ -212,7 +213,7 @@ ol.renderer.canvas.VectorTileLayer.prototype.forEachFeatureAtCoordinate = functi
}
replayGroup = tile.getReplayState().replayGroup;
found = found || replayGroup.forEachFeatureAtCoordinate(
- tileSpaceCoordinate, resolution, rotation, {},
+ tileSpaceCoordinate, resolution, rotation, hitTolerance, {},
@ahocevar
ahocevar Dec 7, 2016 edited Member

I do not think this will yield the expected result, unless hitTolerance is in pixels. Is it? Otherwise this should probably be hitTolerance * resolution.

@simonseyock
simonseyock Dec 8, 2016 Contributor

hitTolerance is in pixels.

src/ol/render/canvas/replaygroup.js
@@ -101,6 +174,7 @@ ol.render.canvas.ReplayGroup.prototype.finish = function() {
* @param {ol.Coordinate} coordinate Coordinate.
* @param {number} resolution Resolution.
* @param {number} rotation Rotation.
+ * @param {number} hitTolerance Hit tolerance.
@ahocevar
ahocevar Dec 7, 2016 Member

What are the units of hitTolerance? Pixels or map units?

@marcjansen
Member

TBH I left this PR unmerged because @tsauerwein had reviewed it. He should also merge it IMO.

Once the conflicts are resolved, I will merge it nonetheless, given how much traction it has.

deal?

@marcjansen
Member

@ahocevar has the ball right now.

@ahocevar

I didn't do a thourough review (I trust @tsauerwein in that), but I found a few things that really should be addressed before this gets merged.

src/ol/render/canvas/replaygroup.js
- 0.5, 0.5,
+ hitTolerance = Math.round(hitTolerance);
+ var contextSize = hitTolerance * 2 + 1;
+ var transform = ol.transform.compose(ol.transform.create(),
@ahocevar
ahocevar Dec 8, 2016 Member

To produce less garbage, it would be better here to reuse an existing transform, like this.hitDetectionTransform_ in the current code.

@simonseyock
simonseyock Dec 8, 2016 Contributor

Well, yes. Thanks for noticing this. I don't know why I removed it.

src/ol/render/canvas/replaygroup.js
- context.clearRect(0, 0, 1, 1);
+ context.canvas.width = contextSize;
+ context.canvas.height = contextSize;
+ context.clearRect(0, 0, contextSize, contextSize);
@ahocevar
ahocevar Dec 8, 2016 edited Member

For performance reasons, you should either set width and height on the canvas, or call clearRect(). Not both. My recommendation for best performance would be to only set width and height when contextSize is different than the current width and height, and call clearRect() otherwise. This is also what we do elsewhere.

@simonseyock
simonseyock Dec 8, 2016 Contributor

Allright, i didn't knew setting width and height would clear the canvas.

+ * @type {number|undefined}
+ * @api
+ */
+olx.AtPixelOptions.prototype.hitTolerance;
@ahocevar
ahocevar Dec 8, 2016 Member

You should document the units of hitTolerance. Pixels? Map units?

@simonseyock
simonseyock Dec 8, 2016 Contributor

Done.

simonseyock added some commits Sep 28, 2016
@simonseyock @simonseyock simonseyock Added hitTolerance test 8589364
@simonseyock @simonseyock simonseyock Make forEachFeatureAtCoordinate work with variable hitContext size a6c768a
@simonseyock @simonseyock simonseyock Change signature of api methods 80188b2
@simonseyock @simonseyock simonseyock changed signature of internal methods 5ce0d8a
@simonseyock @simonseyock simonseyock Implemented midpoint circle algorithm 2493eb2
@simonseyock @simonseyock simonseyock Satisfying linter, jsdoc & compiler 80e392e
@simonseyock @simonseyock simonseyock Removed olx.LayerFilter 63f5776
@simonseyock @simonseyock simonseyock Added hitTolerance to hasFeatureAtPixel. Corrected JsDoc problems. 2ea41af
@simonseyock @simonseyock simonseyock Added hitTolerance to select interaction. Added hitTolerance to selec…
…t interaction example.
e0edefb
@simonseyock @simonseyock simonseyock Added cache for circle arrays. Reusing context. da020d7
@simonseyock @simonseyock simonseyock minor typos f043968
@simonseyock @simonseyock simonseyock Renamed FeatureAtPixelOptions to AtPixelOptions for future use in for…
…EachLayerAtPixel
5608e72
@simonseyock @simonseyock simonseyock Added upgrade note c93b0ba
@simonseyock @simonseyock simonseyock Added old syntax and hasFeatureAtPixel to changelog 4640c44
@simonseyock @simonseyock simonseyock removed uses of lazy evaluation of || e9d2b91
@simonseyock @simonseyock simonseyock Added test for ol.render.canvas.ReplayGroup#getCircleArray_ 3a08cfd
@simonseyock simonseyock Added buffer to ol.renderer.canvas.VectorTileLayer#forEachFeatureAtCo…
…ordinate

buffering by pointResolution times hitTolerance
eede027
@simonseyock simonseyock Documentation and linting f6ee11b
@simonseyock simonseyock buffering extent by resolution*hitTolerance 00a4f3b
@simonseyock @simonseyock simonseyock adjusted hitTolerance by pixelRatio a469803
@simonseyock @simonseyock simonseyock Added hitTolerance to TranslateInteraction 8edc2be
@simonseyock simonseyock Simplified api 55ec0af
@simonseyock simonseyock Adressed review.
Reusing transform
Changing either size of the canvas or clearing it
adding unit of hitTolerance to jsdoc
f10ae6c
@simonseyock simonseyock Added hitTolerance parameter to reworked files
f28e0eb
@simonseyock
Contributor
simonseyock commented Dec 8, 2016 edited

Ok. I did address the requested changes. Thanks for finding these @ahocevar

@simonseyock simonseyock changed the title from (WIP) Add hit tolerance parameter to ol.Map#forEachFeatureAtPixel to Add hit tolerance parameter to ol.Map#forEachFeatureAtPixel Dec 8, 2016
@tsauerwein
Member

Thanks for your additional comments, @ahocevar! I will let you do the merge, when you are satisfied with the changes.

@ahocevar
Member
ahocevar commented Dec 8, 2016

Thanks to everyone involved in this pull request. I'm going to merge this now.

Personally I'd be more in favour of a separate example that shows the effect of the new hitTolerance parameter, instead of squeezing it in the already bloated select-features.html example. But that can be addressed in a follow-up pull request.

@ahocevar ahocevar merged commit 26f31e3 into openlayers:master Dec 8, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 86.677%
Details
@ischas
Contributor
ischas commented Dec 8, 2016

Thank you all for the continuous effort to get this in.

@ischas ischas deleted the KlausBenndorf:hitTolerance branch Dec 8, 2016
@giohappy
Contributor

Great work. Thanks everyone!

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