Skip to content

Implement MSAA for 2D#63003

Merged
akien-mga merged 1 commit into
godotengine:masterfrom
Geometror:msaa-2d
Aug 30, 2022
Merged

Implement MSAA for 2D#63003
akien-mga merged 1 commit into
godotengine:masterfrom
Geometror:msaa-2d

Conversation

@Geometror

@Geometror Geometror commented Jul 14, 2022

Copy link
Copy Markdown
Member

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.

Comment thread drivers/gles3/storage/texture_storage.cpp Outdated
Comment thread editor/editor_node.cpp Outdated
Comment thread editor/plugins/material_editor_plugin.cpp Outdated
Comment thread servers/rendering/renderer_rd/renderer_canvas_render_rd.cpp Outdated
Comment thread servers/rendering/renderer_rd/renderer_canvas_render_rd.cpp Outdated
Comment thread servers/rendering/renderer_rd/storage_rd/texture_storage.cpp Outdated
Comment thread servers/rendering/renderer_rd/storage_rd/texture_storage.cpp Outdated
Comment thread drivers/gles3/storage/texture_storage.cpp Outdated
@Geometror

Geometror commented Jul 16, 2022

Copy link
Copy Markdown
Member Author

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

@Geometror Geometror force-pushed the msaa-2d branch 2 times, most recently from 4e9feee to ce1f1bb Compare July 16, 2022 17:31
@Geometror

Copy link
Copy Markdown
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.

@clayjohn clayjohn left a comment

Copy link
Copy Markdown
Member

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.

Comment thread doc/classes/ProjectSettings.xml Outdated
Comment thread doc/classes/Viewport.xml Outdated

Copy link
Copy Markdown
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

Copy link
Copy Markdown
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.

Copy link
Copy Markdown
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.

Comment thread drivers/gles3/storage/texture_storage.cpp Outdated
Comment thread drivers/gles3/storage/texture_storage.cpp Outdated
Comment thread drivers/gles3/storage/texture_storage.cpp Outdated
Comment thread servers/rendering/renderer_rd/storage_rd/texture_storage.cpp Outdated
Comment thread doc/classes/ProjectSettings.xml Outdated

@Geometror Geometror left a comment

Copy link
Copy Markdown
Member Author

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
Copy Markdown
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
Copy Markdown
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

clayjohn commented Aug 1, 2022

Copy link
Copy Markdown
Member

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
Copy Markdown
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

@clayjohn clayjohn left a comment

Copy link
Copy Markdown
Member

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

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

@clayjohn clayjohn left a comment

Copy link
Copy Markdown
Member

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

reduz commented Aug 16, 2022

Copy link
Copy Markdown
Member

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

@Geometror

Geometror commented Aug 17, 2022

Copy link
Copy Markdown
Member Author

@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
Copy Markdown
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
Copy Markdown
Member

Thanks!

@insomniacUNDERSCORElemon

insomniacUNDERSCORElemon commented Sep 17, 2022

Copy link
Copy Markdown

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
Copy Markdown
Member Author

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

@Calinou

Calinou commented Sep 17, 2022

Copy link
Copy Markdown
Member

@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