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

Add hit-detection support for WebGL #3065

Merged
merged 9 commits into from
Jan 22, 2015

Conversation

tsauerwein
Copy link
Member

This PR implements forEachFeatureAtPixel for WebGL. From the idea it uses the same technique as the canvas renderer: We have a 1x1px framebuffer in which we draw feature-by-feature (in reverse order) and check if there is some color.

Commit e01e556 uses an optimization, that should also be used for the canvas hit-detection: Using the new renderBuffer we build a box around the given position. And then we only check those features that intersect this box. This drastically improves the performance for WebGL.

Speaking about performance, canvas gives better results. I made some tests showing 1000, 5000, ... features on a map (zoomed-out so that really all features are shown). Then I measured the time to do a forEachFeatureAtPixel call.

# of features   Canvas    WebGL
1000            10 ms     20 - 80 ms
5000            35 ms     70 - 350 ms
10000           60 ms     140 - 650 ms
20000           120 ms    340 - 1400 ms
50000           320 ms    800 - 3000 ms 

For WebGL I have given a range, because it depends on where you click with the optimization made in e01e556. If you click somewhere in the map where are no features, it's faster.

But still, for WebGL, it's too slow to run forEachFeatureAtPixel on mousemove. We came up with a hasFeatureAtPixel method, for which I will open a separate PR (here is already an example to give you an idea).

gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_MAG_FILTER, gl.LINEAR);
gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_MIN_FILTER, gl.LINEAR);
gl.texImage2D(
gl.TEXTURE_2D, 0, gl.RGBA, 1, 1, 0, gl.RGBA, gl.UNSIGNED_BYTE, null);
Copy link
Member

Choose a reason for hiding this comment

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

The above, texture-related, code should probably shared. It is not specific to hit detection. If we change the filtering parameters for the rendering to the screen I don't want to have to change some other code for the hit detection.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree.

@elemoine
Copy link
Member

I am looking forward to reviewing this in more detail, but it looks great already! Thanks a lot.

goog.webgl.UNSIGNED_INT : goog.webgl.UNSIGNED_SHORT;
var elementSize = context.hasOESElementIndexUint ? 4 : 2;

var i, groupStart, groupEnd, numItems, start, end, feature;
Copy link
Member

Choose a reason for hiding this comment

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

The groupEnd variable is not used. And it looks like the linter (or compiler) is not able to catch this when the variables are all defined on the same line.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, fixed with tsauerwein@abb5fad

@elemoine
Copy link
Member

elemoine commented Jan 8, 2015

This looks very good to me. I just added two comments, one about an unused variable (groupEnd), and one suggesting to factor out some WebGL texture-related code.

@elemoine
Copy link
Member

elemoine commented Jan 8, 2015

One thing: we need to implement "skip feature", but I think this can be done as a separate PR.

/**
* The last layer state.
* @private
* @type {?ol.layer.LayerState}
Copy link
Member

Choose a reason for hiding this comment

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

The ? is not needed (untested)

Copy link
Member Author

Choose a reason for hiding this comment

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

In fact it is required, because ol.layer.LayerState is only a typedef.

@elemoine
Copy link
Member

This looks good to merge to me.

Any objections?

@fredj
Copy link
Member

fredj commented Jan 19, 2015

LGTM

@gberaudo
Copy link
Member

+1

tsauerwein pushed a commit that referenced this pull request Jan 22, 2015
@tsauerwein tsauerwein merged commit 53f98dc into openlayers:master Jan 22, 2015
@tsauerwein
Copy link
Member Author

Thanks for the reviews!

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

Successfully merging this pull request may close these issues.

None yet

5 participants