-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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 support for drawing points with WebGL #2967
Conversation
[webgl-point] Make WebGL image replay support icon sprites
Clean up WebGL replay code
[webgl-point] Refactoring and fixes
[webgl-point] Merge openlayers:master into the branch
[webgl-point] Add support for icon anchors
[webgl-point] Add support for icon opacity
Address precision/jitter problems by using coordinates relative to the Replay max extent rather that the world.
[webgl-point] Address precision/jitter problems
* @private | ||
* @type {number} | ||
*/ | ||
this.maxSize_ = goog.isDef(options.maxSize) ? options.maxSize : 2048; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why isn't this set to ol.has.WEBGL_MAX_TEXTURE_SIZE
by default (or ol.WEBGL_MAX_TEXTURE_SIZE
if renamed)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original idea was that the atlas manager is independent from WebGL. But I have no problem with setting ol.WEBGL_MAX_TEXTURE_SIZE
as default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So maybe it should be called ol.MAX_ATLAS_SIZE
or something more appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ol.WEBGL_MAX_TEXTURE_SIZE
is used as the maximum atlas size, that's right. But I would like to make it clear that this value is actually the WebGL constant MAX_TEXTURE_SIZE
. Also, later the constant might be used elsewhere.
I haven't reviewed thoroughly yet, but I'm curious if you considered making it so a renderer would create an atlas manager. I understand there are cases where it needs to be configured, but it would be nice to avoid poor(er) performance by default if possible. |
<p id="shortdesc">Icon sprite with WebGL.</p> | ||
<div id="docs"> | ||
<p>See the <a href="icon-sprite-webgl.js" target="_blank">icon-sprite-webgl.js source</a> to see how this is done.</p> | ||
<p>In this example a sprite image is used for the icon styles. Using a sprite is required to get good performances with WebGL.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Performance should be singular.
Yes, we discussed and considered having the renderer create an atlas manager, and render symbols in atlases at prepareFrame time. But we identified several issues with that approach:
So, based on our discussions, and because of the issues described above, we decided not to go with the "renderer creates atlas manager" approach. If we really wanted good WebGL performance by default, and we know that using atlases doesn't have a negative impact with Canvas, then I'd personally use a global atlas manager instance (ol.style.atlasManager), while keeping the possibility to pass an atlas manager to the symbol constructor for maximum flexibility. This wouldn't change the API and can be introduced later if necessary. Thank you for your comments. |
We're going to resume the work on WebGL Points this week (hit detection), so we'd like to merge this to avoid too much branch juggling. We're also happy to get more review comments and make changes. Thanks! |
<h4 id="title">Symbols with WebGL example</h4> | ||
<p id="shortdesc">Using symbols in an atlas with WebGL.</p> | ||
<div id="docs"> | ||
<p>See the <a href="symbol-atlas-webgl.js" target="_blank">symbol-atlas-webgl.js source</a> to see how this is done.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you're adding a new concept here, I think this example needs better documentation.
Minor comments above. This is very nice work. Please merge when conflicts are fixed and you think it is ready. |
ed47b80
to
e109be4
Compare
Conflicts: src/ol/style/circlestyle.js src/ol/style/regularshapestyle.js test/spec/ol/style/regularshapestyle.test.js
@tschaub, thanks a lot for your comments. I think we've addressed them all. I'm going to merge this. Feel free to provide more comments on our latest commits, and we will address them with separate PRs. Thanks again. |
Add support for drawing points with WebGL
This PR adds support for drawing points with WebGL.
Both icons and symbols are supported, and all the icon/symbol styling properties supported by the Canvas renderer are also supported by the WebGL renderer.
We've added two new examples, one with icons and one with symbols. See
(These examples use WebGL by default but
?renderer=canvas
can be appended to the URL to use Canvas for comparison.)The vector API is unchanged. But you have to use an
AtlasManager
for symbols if you want to get good performance with WebGL:It should still work w/o the atlas manager, but it will be a lot less performant. The same problem exists for icons. But for icons the application developer is responsible for creating sprites to get good performance.
The
symbol-atlas-webgl
example shows how to use an atlas manager. Theicon-sprite-webgl
example shows how to use an icon sprite.Drawing points at
postcompose
time, with the immediate API or with feature overlays, is also supported. Thedynamic-data
anddraw-features
examples can now be loaded with?renderer=webgl
in the URL to see this working.What this PR does not include: Hit Detection. This is our next (big) step! We will start working on it as soon as this PR is merged.
A final note about the commit history:
This PR includes a lot of commits, including merge commits. But we decided not to rewrite the commit for this PR. It is a large work, so it makes sense not too loose the history. And we have merge commits that point to PRs on http://github.com/camptocamp/ol3, with valuable information on the code.
Please review.