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

Implement MSAA for 2D #63003

Merged
merged 1 commit into from
Aug 30, 2022
Merged

Implement MSAA for 2D #63003

merged 1 commit into from
Aug 30, 2022

Conversation

Geometror
Copy link
Member

@Geometror Geometror commented Jul 14, 2022

This PR implements MSAA for the 2D/Canvas renderer. (Implements godotengine/godot-proposals#1297) In addition, this might improve the situation for godotengine/godot-proposals#1126.

Detailed changes:

  • Implement 2D MSAA support for the Canvas renderer, which can be controlled separately from 3D MSAA for maximum flexibility (2X, 4X and 8X are supported, same as for 3D)
    • implementation should be efficient since we can use automatic resolving without the need of VkCmdResolveImage, which has a negative impact on bandwidth
    • can be set for each viewport separately
    • currently not implemented for OpenGL implementation (but should be easy once the OpenGL renderer is done)
  • Rename project settings, viewport properties and methods from msaa to msaa_2/3d for clarity
  • Minor code restructuring and comment style fixes

Screenshot 2022-07-29 011539

Without MSAA:
godot windows tools 64_ev8S4G9ANk
MSAA 2x:
HTVAzzwNeh
MSAA 4x:
P7b5duDhEK
MSAA 8x:
DY33rv7W7a

Bugsquad edit: This partially addresses godotengine/godot-proposals#4354. This closes #56233 by adding MSAA sample checks to both 2D and 3D.

@Geometror
Copy link
Member Author

Geometror commented Jul 16, 2022

NOTE: No longer included!
Cleaned it up and added the implementation for the OpenGL renderer:
grafik

@Geometror
Copy link
Member Author

Rebased and added checks for the maximum supported sample count for both 2D and 3D MSAA. This should fix any crashes on Mali when MSAA is above 4x.

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Great work! I am very excited about this PR and I know many users will be too.

I have left a few comments on implementation, mostly just stuff that needs to be fixed up in the GLES3 implementation.

doc/classes/ProjectSettings.xml Outdated Show resolved Hide resolved
doc/classes/Viewport.xml Outdated Show resolved Hide resolved

glTexImage2D(GL_TEXTURE_2D, 0, rt->color_internal_format, rt->size.x, rt->size.y, 0, rt->color_format, rt->color_type, nullptr);
if (rt->msaa != RS::VIEWPORT_MSAA_DISABLED) {
#ifndef JAVASCRIPT_ENABLED
Copy link
Member

Choose a reason for hiding this comment

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

WebGL2 uses Renderbuffers to support Multisampled Framebuffers. So you can still use it even though GL_TEXTURE_2D_MULTISAMPLE isn't defined

@@ -247,6 +247,8 @@ void RasterizerCanvasGLES3::canvas_render_items(RID p_to_render_target, Item *p_
Rect2i group_rect = ci->canvas_group_owner->global_rect_cache;

if (ci->canvas_group_owner->canvas_group->mode == RS::CANVAS_GROUP_MODE_OPAQUE) {
// Resolve the multisampled framebuffer before copying to the back buffer.
_resolve_multisampled_fbo(p_to_render_target);
Copy link
Member

Choose a reason for hiding this comment

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

We need to minimize the number of FBO changes and full screen copies we do in order to keep performance manageable on mobile devices. Accordingly, I would put the resolve inside of render_target_copy_to_back_buffer(). If copying the full rect, you can get away with resolving the Multisampled buffer directly into the backbuffer. If copying a partial rect, you'll have to resolve and then copy still.


glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST);
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
glTexImage2DMultisample(GL_TEXTURE_2D_MULTISAMPLE, texture_samples[rt->msaa], rt->color_internal_format, rt->size.x, rt->size.y, GL_TRUE);
Copy link
Member

Choose a reason for hiding this comment

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

Right now this will break 3D rendering as the same color texture is used in both 2D and 3D. Since the Depth texture does not use Multisample the Framebuffer will fail on creation.

drivers/gles3/storage/texture_storage.cpp Show resolved Hide resolved
drivers/gles3/storage/texture_storage.cpp Show resolved Hide resolved
drivers/gles3/storage/texture_storage.cpp Show resolved Hide resolved
doc/classes/ProjectSettings.xml Outdated Show resolved Hide resolved
Copy link
Member Author

@Geometror Geometror left a comment

Choose a reason for hiding this comment

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

Currently combined 2D and 3D rendering is broken since the resolve attachment's store op is VK_ATTACHMENT_STORE_OP_DONT_CARE while it should be VK_ATTACHMENT_STORE_OP_STORE. (this took way too long to figure out :))
grafik
Just brute forcing all store ops to VK_ATTACHMENT_STORE_OP_STORE resolves the issue:
grafik
Now that I know what's wrong I can work on a proper fix, will probably push one tomorrow after I addressed clayjohns review comments. (I also rewrote a few bits to make things cleaner)

@Geometror Geometror force-pushed the msaa-2d branch 2 times, most recently from de56470 to b945698 Compare July 28, 2022 23:55
@Geometror
Copy link
Member Author

The Vulkan side is should be done now, OpenGL still needs some work, but because of #63600 I can't test my changes - so that might take some time. If it's ok, I would prefer to only have the critical parts in the OpenGL implementation fixed and then implement 2D MSAA properly later (e.g. with webgl2 support) when the OpenGL renderer has matured. This PR was already pretty time-consuming and unfortunately I have not much time currently.
Screenshot 2022-07-29 011539

@Geometror
Copy link
Member Author

Rebased. Regarding the GLES3 part: I just looked at the code and noticed that 3D MSAA is also not yet implemented. I would rather strip the implementation completely from this PR and add both 2D and 3D MSAA together at a later point in time (and properly, so that you can set both independently from each other like in the Vulkan renderer).

@clayjohn
Copy link
Member

clayjohn commented Aug 1, 2022

Rebased. Regarding the GLES3 part: I just looked at the code and noticed that 3D MSAA is also not yet implemented. I would rather strip the implementation completely from this PR and add both 2D and 3D MSAA together at a later point in time (and properly, so that you can set both independently from each other like in the Vulkan renderer).

Fair enough. I am okay with that if you are planning on adding it back at a later time to avoid this getting too complex.

@Geometror
Copy link
Member Author

Ok, I removed the GLES3 implementation (a warning is printed when the user tries to enable it). With that I think this PR is complete :)

@Geometror Geometror requested a review from clayjohn August 4, 2022 19:44
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Just the one change before it is ready to go

drivers/vulkan/rendering_device_vulkan.cpp Outdated Show resolved Hide resolved
@Geometror Geometror force-pushed the msaa-2d branch 4 times, most recently from 65ef7c7 to 6039b23 Compare August 10, 2022 01:44
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

I'm happy with this PR. Should be ready to merge.

CC @BastiaanOlij I think this may conflict with your Framebuffer reorganization PR, but I think we should merge it now as Geometror won't be available for awhile after the end of this week.

@reduz
Copy link
Member

reduz commented Aug 16, 2022

Does this work properly with backbuffer or clip effects? I think it would be good to test this before merging.

@Geometror
Copy link
Member Author

Geometror commented Aug 17, 2022

@reduz Yes, this seems to work fine with those effects(MSAA 8x):

Polygon2D with clip children enabled
image

Backbuffer-reading post processing shader
image

@clayjohn clayjohn requested a review from reduz August 18, 2022 22:26
@akien-mga
Copy link
Member

@reduz says it's OK so let's merge 👍

@akien-mga akien-mga merged commit 02d510b into godotengine:master Aug 30, 2022
@akien-mga
Copy link
Member

Thanks!

@insomniacUNDERSCORElemon
Copy link

insomniacUNDERSCORElemon commented Sep 17, 2022

Small note: this doesn't smooth the clipped items themselves, only the parent. Note the zoomed square (that part lacking AA is a polygon):

ArcoLinux_2022-09-17_04-38-17

(also seen w/this screenshot is the backbuffer ghosting which is not specific to MSAA, also unwanted color blending)

@Geometror
Copy link
Member Author

@insomniacUNDERSCORElemon Could you open an issue and provide an reproduction project? :)

@Calinou
Copy link
Member

Calinou commented Sep 17, 2022

@insomniacUNDERSCORElemon Could you open an issue and provide an reproduction project? :)

An issue was opened: #66005

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

Successfully merging this pull request may close these issues.

Vulkan: Selecting 8× MSAA on Apple M1 GPU crashes the editor
7 participants