Add MSAA support for WebXR #84686
Add MSAA support for WebXR #84686
Conversation
dsnopek
force-pushed
the
webxr-msaa
branch
3 times, most recently
from
63dd028
to
ee9aed3
Compare
dsnopek
force-pushed
the
webxr-msaa
branch
3 times, most recently
from
9f1dd94
to
1e37913
Compare
|
I just rebased now that the main OpenGL MSAA PR (#83976) has been merged. This should be ready for review! |
modules/webxr/webxr_interface_js.cpp
Outdated
| @@ -461,21 +461,10 @@ bool WebXRInterfaceJS::pre_draw_viewport(RID p_render_target) { | |||
| // See: https://immersive-web.github.io/layers/#xropaquetextures | |||
| // | |||
| // This is why we're doing this sort of silly check: if the color and depth | |||
| // textures are the same this frame as last frame, we need to attach them | |||
| // textures are the same this frame as last frame, we need to reattach them | |||
There was a problem hiding this comment.
Did you ever figure out why this is needed? Sounds more like we're forgetting to update the framebuffer somewhere or that it gets updated by code that forgets about the override logic. Seems weird this would only be needed for webXR
There was a problem hiding this comment.
Did you ever figure out why this is needed?
Yep, it's what I'm attempting to explain in the comment, but apparently not very well. :-)
The short version:
Every frame, we need to re-attach the textures we get from WebXR to the framebuffer even if (or, actually, "especially if") they have the same OpenGL id.
The long version:
The WebXR spec invents this concept that doesn't otherwise exist in WebGL called "opaque textures", which is what the link in my comment is about: https://immersive-web.github.io/layers/#xropaquetextures
The goal of opaque textures is to hide the swapchain from the developer. Personally, I think this is a bad idea, because we're really doing a lot of work to hide something that's not really that complex. But it is what is. :-)
To attempt to explain briefly: every frame, we ask WebXR for the texture to render to, and every frame it passes us the exact same texture object. Note: these are objects (not ids) in WebGL. Underneath, each WebGL texture object actually refers to an OpenGL texture ID from the real OpenGL driver, and what's happening is that it's changing that underlying texture id to the next texture in the swapchain, but this is completely hidden from the WebGL-side, where all we see is the exact same texture object. However, since it's actually a different texture underneath, we need to reattach it to the FBO, because otherwise we'd be trying to render to the previous texture in the swapchain.
Where it gets properly is confusing is Emscripten mapping from WebGL to the OpenGL API. Emscripten assigns its own id numbers to the texture objects (which are not related at all to the ids from the real OpenGL driver underneath). It does this by simply setting an id property on the texture object (ex texture.id = 123) because you can just assign arbitrary properties on Javascript objects.
So, from C++, every frame, we're asking WebXR for the texture to render to, and it's just giving us the same id number every frame (because it's the exact same WebGL texture object), but we need to reattach that texture to the FBO anyway, because underneath-underneath it's actually referring to the next texture in the swapchain. This looks silly from C++, because it's the same texture id number every frame, but we need to do it anyway.
(Unfortunately, this also conflicts with any caching that we're doing in Godot, because we make the assumption that each texture id is a unique, individual texture, so that if we get the same texture id, we can just reuse a previously created FBO. However, in this case, because the texture id could really be a different texture underneath, we need to re-attach it anyway.)
Hopefully, that makes sense? I can try and update the comment to explain the situation a little better.
There was a problem hiding this comment.
Hm. I spent some time thinking about the comment in the code, and I'm actually not sure I can make it much clearer without making it 3x as long or drawing a picture. :-)
I am going to simplify the logic by just always re-attaching the texture, rather than the sort of confusing check where I'm attempting to account for "what if the browser did actually give us a different object for every texture in the swapchain" which no browsers actually do. That will maybe make the code less confusing overall.
But please let me know if you have any suggestions to make the explanation clearer.
There was a problem hiding this comment.
Nah all good, it's just so alien and dumb that the penny didn't drop when I read it. It goes against the grain of what a framebuffer object is meant to do (maintain a fixed state you can enable), you're not supposed to update it every frame and on some drivers that would even be fairly costly.
This is why in Vulkan we cache framebuffer objects when go loop through a swapchain like this.
But if that is the direction they went with in WebXR, that even when you have the same ID for an object, underlying you have a changing ID, and thus you need to update your frame buffer, good on them....... ;)
There was a problem hiding this comment.
Nah all good, it's just so alien and dumb that the penny didn't drop when I read it. It goes against the grain of what a framebuffer object is meant to do (maintain a fixed state you can enable), you're not supposed to update it every frame and on some drivers that would even be fairly costly.
Yep, I agree, it's pretty dumb.
I'm hoping that one day the standard is amended to not do this. It would actually be pretty easy to do in a backwards compatible way: since we're already required to re-attach the textures every frame, if WebXR started giving us a different texture object for each texture in the swapchain (rather than the same one over and over), existing code wouldn't need to change at all. And if we could detect this new behavior (maybe a flag could be set somewhere?), then we could start caching FBO's and avoid re-attaching the textures every frame, probably for a performance gain.
But we'll see - web standards change very slowly :-)
|
@BastiaanOlij Thanks for the review! I've addressed your comments in my latest push |
|
Thanks! |
This builds on top of PR #83976
It works in my testing on the Meta Quest 2.
Due to one of the weirder parts of WebXR, we need to re-attach the color and depth texture to any FBO using them on every frame. Previously, this was being done via
WebXRInterfaceJS::pre_draw_viewport()by updating the render target's FBO. However, that won't work with MSAA because it's not using the render target's FBO, but instead one created byRenderSceneBuffersGLES3, so I had to add a special flag to render targets tellingRenderSceneBuffersGLES3to reattach the textures. This is a little hacky, but I can't think of a better way to do it.Keeping this as a draft until PR #83976 has been mergedUPDATE: It has been merged! This is no longer a draft :-)
Production edit: closes godotengine/godot-roadmap#72