Introduce hasFeatureAtPixel

[tsauerwein]

This PR is based on #3065.
#3065 implements hit-detection for WebGL, but unfortunately the performance is not good enough to do hit-detection on mousemove (if you have many features). This is because we are calling drawElements for every feature, which is expensive.

For mousemove you are usually not interested in the actual features that forEachFeatureAtPixel provides, you just want to know if there is a feature or not, so that you can change the mouse pointer. So in this case you can draw all features at "once" (or to be precise: per texture group) for the hit-detection, and then check if there is a feature or not.

That's why we introduced a hasFeatureAtPixel method, which simply returns true if there is a feature at the given pixel. For WebGL this function is very fast, for 50000 features it takes less than 20ms (on my system).

Example usage:

$(map.getViewport()).on('mousemove', function(evt) {
  var pixel = map.getEventPixel(evt.originalEvent);
  var hit = map.hasFeatureAtPixel(pixel);
  map.getTarget().style.cursor = hit ? 'pointer' : '';
});

Example: icon-sprite-webgl.html

[tschaub]

I'm confused why this wouldn't call hasFeatureAtPixel for each layer renderer (with a visible layer).

[tsauerwein]

I'm confused why this wouldn't call hasFeatureAtPixel for each layer renderer (with a visible layer).

Am I not doing exactly that by calling forEachFeatureAtPixel?

[elemoine]

I'm confused why this wouldn't call hasFeatureAtPixel for each layer renderer (with a visible layer).

This one is the default implementation, used by the Canvas and DOM renderers. It is based on the forEachFeatureAtPixel function.

The WebGL implementation does call hasFeatuteAtPixel for each layer renderer.

[tschaub]

I was expecting to see something like this at the map renderer level:

ol.renderer.Map.prototype.hasFeatureAtPixel =
    function(coordinate, frameState, layerFilter, thisArg) {
  var layerStates = this.getMap().getLayerGroup().getLayerStatesArray();
  for (var i = layerStates.length - 1; i >= 0; --i) {
    var layerState = layerStates[i];
    var layer = layerState.layer;
    if (ol.layer.Layer.visibleAtResolution(layerState, viewState.resolution) &&
        layerFilter.call(thisArg, layer) &&
        this.getLayerRenderer(layer).hasFeatureAtPixel(coordinate, frameState)) {
      return true;
    }
  }
  return false;
};

And then see the default layer renderer implementation of hasFeatureAtPixel delegate to forEachFeatureAtPixel.

But I see now that this doesn't account for feature overlays and the specific work that the WebGL renderer has to do there. This can happen later, but I still can imagine that we could have one hasFeatureAtPixel implementation at the map renderer level that could call a specific overlayHasFeatureAtPixel method for each map renderer type.

[tsauerwein]

Ok, I see. I think this would make sense if there was a layer renderer which had an optimized implementation for hasFeatureAtPixel. As this is currently not the case, I would prefer to stick with the solution of simply falling back to forEachFeatureAtPixel of the map renderer.

[tschaub]

I like the idea of hasFeatureAtPixel. I think it would make sense to have a basic (albeit inefficient) implementation for all renderers (instead of having the Canvas and DOM implementations return false). It could simply use forEachFeatureAtPixel until a more efficient implementation is written.

[tsauerwein]

I like the idea of hasFeatureAtPixel. I think it would make sense to have a basic (albeit inefficient) implementation for all renderers (instead of having the Canvas and DOM implementations return false). It could simply use forEachFeatureAtPixel until a more efficient implementation is written.

In fact, the hasFeatureAtPixel implementation in ol.renderer.Map calls forEachFeatureAtPixel. So there is a default implementation that also works for canvas and DOM.

[tsauerwein]

I think it would make sense instead to update this example to use the new hasFeatureAtPixel method (assuming it works for all renderers).

Good point!

[elemoine]

Shouldn't this be goog.isDef(hasFeature) ? true : false; ?

[gberaudo]

Even better: return goog.isDef(hasFeature) !

[elemoine]

Obviously. I was about to edit my comment :-)

[tsauerwein]

You are right.

[elemoine]

I've just added a pretty minor comment, otherwise looks very good to me.

[elemoine]

This should be an @enum.

[tsauerwein]

I guess you meant @const. :)

[elemoine]

Yes I did! ;-)

[tschaub]

Very minor, but the last argument could be named thisArg (without the 2).

[tsauerwein]

Any other comments? cc @tschaub

[elemoine]

opt_this2 can be changed to opt_this.

[tsauerwein]

Sorry, I thought I did this already. :) Fixed in #3108

[elemoine]

I just made a comment that @tschaub already made regarding the name opt_this2. This is minor.

Otherwise it looks very good to me. It's good to merge to me.

[tsauerwein]

Thanks for reviewing!