Add 2D MSAA to GLES3 backend#111531
Conversation
There was a problem hiding this comment.
Tested locally on Windows + NVIDIA, it works as expected.
We should test this PR on mobile hardware to check its performance impact, and check that it works correctly on the web platform on various browsers too.
As an (unrelated) tangent, it is possible to force a MSAA level in the NVIDIA Control Panel on a Godot OpenGL game, but only if you use Enhance application setting instead of Override application setting. This affects either 2D, 3D or both depending on whether MSAA is enabled in Godot (if it's disabled, Enhance application setting can't enable MSAA). Note that "enhance" is a misnomer here, as you can set it to Off and it will turn off MSAA, even if it's enabled in Godot.
clayjohn
left a comment
There was a problem hiding this comment.
Great job so far!
I think you need to rethink the logic for how this works. Currently you are bouncing the texture back and forth between MSAA and non-MSAA several times per frame which should never happen. When MSAA is being used everything should render in MSAA. There is no reason to even allocate the non-MSAA texture when MSAA is being used. There certainly isn't a good reason to copy the MSAA texture into the non-MSAA texture and back (that will lose the MSAA sample information by the way and defeat the purpose of having an MSAA texture in the first place)
| if (render_target->msaa_2d.mode != RS::VIEWPORT_MSAA_DISABLED) { | ||
| glBindFramebuffer(GL_READ_FRAMEBUFFER, render_target->fbo); | ||
| glBindFramebuffer(GL_DRAW_FRAMEBUFFER, render_target->msaa_2d.fbo); | ||
|
|
||
| glBlitFramebuffer(0, 0, render_target->size.x, render_target->size.y, | ||
| 0, 0, render_target->size.x, render_target->size.y, | ||
| GL_COLOR_BUFFER_BIT, GL_NEAREST); | ||
| glBindFramebuffer(GL_FRAMEBUFFER, render_target->msaa_2d.fbo); | ||
| } |
There was a problem hiding this comment.
You should never need to blit from the non-MSAA framebuffer into the MSAA framebuffer. When MSAA is enabled, all rendering should use the MSAA framebuffer
There was a problem hiding this comment.
We need to get the 3D rendering result into the MSAA buffer though. After a bit of research I found that it is rather common to render the buffer with the rendered 3D scene onto a quad which then can be used as background.
| if (render_target->msaa_2d.mode != RS::VIEWPORT_MSAA_DISABLED && render_target->msaa_2d.needs_resolve) { | ||
| glBindFramebuffer(GL_READ_FRAMEBUFFER, render_target->msaa_2d.fbo); | ||
| glBindFramebuffer(GL_DRAW_FRAMEBUFFER, render_target->fbo); | ||
|
|
||
| glBlitFramebuffer(0, 0, render_target->size.x, render_target->size.y, | ||
| 0, 0, render_target->size.x, render_target->size.y, | ||
| GL_COLOR_BUFFER_BIT, GL_NEAREST); | ||
| glBindFramebuffer(GL_FRAMEBUFFER, render_target->fbo); | ||
| } |
There was a problem hiding this comment.
This shouldn't be happening. This function can be called multiple times per frame.
There was a problem hiding this comment.
Yeah, I see the problem. We have to define a clear endpoint then. In "servers/rendering/renderer_viewport.cpp" (in _draw_viewport, somewhere after the for loop in which render_canvas gets called) I would add a call to a new function in the texture storage like RSG::texture_storage->finalize_viewport_render_target(p_viewport->render_target) in which we then could put the resolve. Since this would be only used by the GLES backend (for now) we could also surround this call with an if statement that checks the used backend.
| if (!backbuffer_cleared) { | ||
| texture_storage->render_target_clear_back_buffer(p_to_render_target, Rect2i(), Color(0, 0, 0, 0)); | ||
| backbuffer_cleared = true; | ||
| } |
There was a problem hiding this comment.
Why is this being cleared? The backbuffer should only be cleared when needed. It definitely shouldn't be cleared when not used
There was a problem hiding this comment.
Misunderstood the architecture here, will be removed!
| if (p_to_backbuffer && render_target->msaa_2d.mode != RS::VIEWPORT_MSAA_DISABLED && render_target->msaa_2d.backbuffer_fbo != 0 && render_target->msaa_2d.needs_resolve) { | ||
| glBindFramebuffer(GL_READ_FRAMEBUFFER, render_target->msaa_2d.backbuffer_fbo); | ||
| glBindFramebuffer(GL_DRAW_FRAMEBUFFER, render_target->backbuffer_fbo); | ||
|
|
||
| glBlitFramebuffer(0, 0, render_target->size.x, render_target->size.y, | ||
| 0, 0, render_target->size.x, render_target->size.y, | ||
| GL_COLOR_BUFFER_BIT, GL_NEAREST); | ||
| glBindFramebuffer(GL_FRAMEBUFFER, render_target->msaa_2d.backbuffer_fbo); | ||
| } |
There was a problem hiding this comment.
This can happen several times for each call to canvas_render_items(). Which itself can be called several times per frame. This shouldn't be happening so frequently.
There was a problem hiding this comment.
Haven't found a better solution for this yet, but the question is whether we want to support MSAA for the backbuffer, since it is not supported for the other backends as well.
|
hello, is this still being worked on? i might be able to help if you want. |
|
Thanks for the comments and sorry for responding so late. I have gone through the code again and left some comments on it. @HAROCODE-MEWO that would be awesome. As mentioned I have commented my new insights to the code but I still have no clue how I could render the 3D image on a quad as background (or whether my new approach is even better than the old one :D ) |
187796a to
ff87ba7
Compare
|
I finally found some time to implement my new approach. I adressed the blitting/performance issues and I found out how to render the already rendered scene as a background quad. I put the details in the PR description :) |
|
Looks good in broad strokes, my two cents here are:
|
|
@BastiaanOlij While I would need to spend more time on that, I think it would be possible to implement one MSAA buffer for 2D and 3D. However, this would mean that we could only have one MSAA setting for both, 2D and 3D, instead of separate ones and the post processing of the 3D scene would be needed to be applied after the final resolve which then would affect the 2D scene as well which I am not sure if we would want this. (Maybe it would be also possible to solve these issues but, as you mentioned, this would be probably much more involved). If you are refering to |
This PR implements the 2D MSAA in the OpenGL3 backend. It is based on the discussions in #83976 and the already existing code from #84688
Current base commit: 6d6e822 (4.7 Beta 1)
New version (v2)
I overhauled the previous implementation to address the amount of blitting used in my first approach. You can find the implementation details of the older version below. Images are provided after.
I will squash the commits if this approach is approved. For now I'd keep both to have the comparison to the first version available.
Technical details
We create a multisampled framebuffer when turning 2D MSAA on and delete it again when turned off again.
I introduced two new functions in the TextureStorage class:
render_target_prepare_canvas_msaain which we copy the scene that is rendered up to this point onto the MSAA framebuffer using the CopyEffects class.render_target_finalize_canvas_msaain which we simply callrender_target_do_msaa_resolvewhich in itself blits the final 2D render onto the normal framebuffer.These functions are only implemented in the OpenGL backend since these operations are already implied in the other backends.
render_target_prepare_canvas_msaais called after a potential 3D scene is rendered, before we render the 2D scene.render_target_finalize_canvas_msaais called after the 2D scene is done rendering. Since Godot supports the rendering of the 3D scene in between the 2D scene which in turn relies on the non-multisampled framebuffer, we need to resolve our MSAA framebuffer before rendering the 3D scene. This means that in this case we need two full resolves in one render cycle.Furthermore, if the backbuffer is used we might need to copy parts of the frontbuffer onto the backbuffer. For this I introduced a partial resolve in
TextureStorage::render_target_copy_to_back_buffer.For Mobile devices we can still use the
GL_EXT_multisampled_render_to_textureextension so we don't need the resolve steps.Discussion
For this approach I decided to not support MSAA on the backbuffer, apart from the mentioned partial resolve when copying from the frontbuffer to the backbuffer.
I also decided that the partial resolve is a tradeoff worth making, since otherwise we would need to turn off MSAA completely when encountering a copy from the frontbuffer to the backbuffer.
Old version (v1)
Technical details
We create a multisampled framebuffer when turning 2D MSAA on, as well as a multisampled backbuffer if it is needed.When rendering without the backbuffer we simply have to blit the already rendered 3D scene (if available) to the multisampled framebuffer, render the complete scene and finally resolve the MSAA buffer into the normal framebuffer. I put the final resolve now into the end ofRasterizerCanvasGLES3::canvas_render_items, since everything should be already rendererd at this point (but please correct if I am wrong).When using the backbuffer we have to consider the following things:We need to resolve the backbuffer everytime we render onto it, since we sample from it later onWhen copying parts of or the complete render_target we first need to resolve the frontbuffer, so we can sample from it, and after copying we need to blit the backbuffer back to the multisampled backbufferWhen clearing the backbuffer we need to clear the normal backbuffer as well as the multisampled backbufferFor Mobile devices we want to use theGL_EXT_multisampled_render_to_textureextension so we don't need the resolve step.Discussion
While this approach works, we introduce quite a lot of blitting operations when using the backbuffer. If this is not wanted, another approach could be to copy or adjust the relevant shaders (like the copy shader or the default_clip_children_shader) to support multisampled textures.While making the images below, I noticed that the vulkan backend does not include MSAA on the backbuffer. That would of course also be an option for the OpenGL backend.Images
closes #69462 and #84688