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

Added MSAA to GLES2 backend #28518

Merged
merged 1 commit into from
May 13, 2019
Merged

Conversation

clayjohn
Copy link
Member

As discussed with @reduz, this PR checks to see if the multisample FBO extension is available on the hardware, if so it renders to a multisample renderbuffer and blits back to the normal framebuffer at the end of frame or when the screen is being read from.

Fixes #25843

@clayjohn clayjohn requested a review from reduz as a code owner April 29, 2019 18:54
@akien-mga akien-mga added this to the 3.2 milestone Apr 29, 2019
@akien-mga akien-mga changed the title Added MSAA to GLES backend Added MSAA to GLES2 backend Apr 29, 2019
@akien-mga
Copy link
Member

Note for release managers: if/when merged, let it simmer in the master pot for a while for potential fixups, but otherwise it should be a nice candidate for cherry-picking in 3.1.2.

@clayjohn
Copy link
Member Author

clayjohn commented Apr 30, 2019

Changes to fix the travis ci

edit: Okay attempt number two, I reordered some stuff and scoped off a section with {} just like in GLES3

@clayjohn
Copy link
Member Author

clayjohn commented Apr 30, 2019

@akien-mga when you get a chance I could use a hand with the travis-ci errors. I have resolved most, the last one is only for android and iphone. Neither one properly recognizes the OpenGL-specific constants. They are used the same as in GLES3, so they should be defined somewhere, but I can't figure out what travis needs in order to recognize that.

I suspect the solution is going to be to have a defines section at the top of the files like we currently have for the texture API, but wrapped in an GLES_OVER_GL ifdef statement.

#define _EXT_COMPRESSED_RGBA_S3TC_DXT1_EXT 0x83F1
#define _EXT_COMPRESSED_RGBA_S3TC_DXT3_EXT 0x83F2
#define _EXT_COMPRESSED_RGBA_S3TC_DXT5_EXT 0x83F3
#define _EXT_COMPRESSED_RED_RGTC1_EXT 0x8DBB
#define _EXT_COMPRESSED_RED_RGTC1 0x8DBB
#define _EXT_COMPRESSED_SIGNED_RED_RGTC1 0x8DBC
#define _EXT_COMPRESSED_RG_RGTC2 0x8DBD
#define _EXT_COMPRESSED_SIGNED_RG_RGTC2 0x8DBE
#define _EXT_COMPRESSED_SIGNED_RED_RGTC1_EXT 0x8DBC
#define _EXT_COMPRESSED_RED_GREEN_RGTC2_EXT 0x8DBD
#define _EXT_COMPRESSED_SIGNED_RED_GREEN_RGTC2_EXT 0x8DBE
#define _EXT_ETC1_RGB8_OES 0x8D64
#define _EXT_COMPRESSED_RGB_PVRTC_4BPPV1_IMG 0x8C00
#define _EXT_COMPRESSED_RGB_PVRTC_2BPPV1_IMG 0x8C01
#define _EXT_COMPRESSED_RGBA_PVRTC_4BPPV1_IMG 0x8C02
#define _EXT_COMPRESSED_RGBA_PVRTC_2BPPV1_IMG 0x8C03
#define _EXT_COMPRESSED_SRGB_PVRTC_2BPPV1_EXT 0x8A54
#define _EXT_COMPRESSED_SRGB_PVRTC_4BPPV1_EXT 0x8A55
#define _EXT_COMPRESSED_SRGB_ALPHA_PVRTC_2BPPV1_EXT 0x8A56
#define _EXT_COMPRESSED_SRGB_ALPHA_PVRTC_4BPPV1_EXT 0x8A57
#define _EXT_COMPRESSED_RGBA_BPTC_UNORM 0x8E8C
#define _EXT_COMPRESSED_SRGB_ALPHA_BPTC_UNORM 0x8E8D
#define _EXT_COMPRESSED_RGB_BPTC_SIGNED_FLOAT 0x8E8E
#define _EXT_COMPRESSED_RGB_BPTC_UNSIGNED_FLOAT 0x8E8F

@akien-mga
Copy link
Member

@clayjohn The problem is that those methods and constants are not defined in OpenGL ES 2.0, but they are in OpenGL 2.1. From a quick look, there doesn't seem to be widely supported extensions to bring those to OpenGL ES 2.0.

So this code should be enclosed in #ifdef GLES_OVER_GL to be OpenGL-only, or an alternative implementation will be needed to be compatible with the OpenGL ES 2.0 spec.

@akien-mga
Copy link
Member

So this code should be enclosed in #ifdef GLES_OVER_GL to be OpenGL-only, or an alternative implementation will be needed to be compatible with the OpenGL ES 2.0 spec.

I guess we need to make sure to limit the API calls to things enabled by https://www.khronos.org/registry/OpenGL/extensions/EXT/EXT_framebuffer_multisample.txt (for OpenGL 1.5+ so GLES_OVER_GL for us) and https://www.khronos.org/registry/OpenGL/extensions/EXT/EXT_multisampled_render_to_texture.txt (for OpenGL ES 2.0).

BTW, useful reference to find matching extensions: https://chromium.googlesource.com/angle/angle/+/master/src/libANGLE/renderer/gl/FeatureSupportGL.md

@akien-mga
Copy link
Member

Also:

If OpenGL ES 3.0 or later is not supported, ignore all references
to DRAW_FRAMEBUFFER and READ_FRAMEBUFFER.

in https://www.khronos.org/registry/OpenGL/extensions/EXT/EXT_multisampled_render_to_texture.txt

@akien-mga
Copy link
Member

So there are several things to do for OpenGL ES 2.0 support:

  • Identify and replace the API calls which can't be supported, like glReadBuffer, glBlitFramebuffer, GL_READ_FRAMEBUFFER and GL_DRAW_FRAMEBUFFER
  • Then some of the remaining calls likely need EXT appended as they're neither in the OpenGL 2.1 nor OpenGL ES 2.0 specifications, but rely on the two extensions mentioned above and checked against in this patch.
  • Then I guess some magic will be needed so that we can make those EXT calls conditionally point to the right GLAPIENTRY or to void, based on whether the matching extension is defined.

@clayjohn clayjohn force-pushed the GLES2-MSAA branch 2 times, most recently from 8a420fe to 3ec88b8 Compare April 30, 2019 23:59
@clayjohn clayjohn force-pushed the GLES2-MSAA branch 3 times, most recently from 1a7f485 to 7acd7dd Compare May 1, 2019 00:16
@clayjohn
Copy link
Member Author

clayjohn commented May 1, 2019

@akien-mga Im gonna need some help figuring out the GLAPIENTRY stuff. I thought I could get it to work by including <GLES2/glext.h> because that is where the definitions of the functions are stored, but it didn't work. reduz suggested just defining the functions in the file. That may be our best option. But I'm not sure how that would work while still keeping things nice and clean.

I added stuff to the glad file for 2.1, this should make things work without the EXT call. (I'm just mirroring how it is already done in rasterizer_gles2.cpp)

@akien-mga
Copy link
Member

@clayjohn The errors are:

drivers/gles2/rasterizer_storage_gles2.cpp:4689:3: error: use of undeclared
      identifier 'glRenderbufferStorageMultisample'
                glRenderbufferStorageMultisample(GL_RENDERBUFFER, msaa, ...
                ^
drivers/gles2/rasterizer_storage_gles2.cpp:4711:3: error: use of undeclared
      identifier 'glFramebufferTexture2DMultisample'
                glFramebufferTexture2DMultisample(GL_FRAMEBUFFER, GL_COL...
                ^

But as I mentioned above, and as per the extension, you should use glRenderbufferStorageMultisampleEXT and glFramebufferTexture2DMultisampleEXT, both of which are properly defined in GLES2/gl2ext.h (at least in the Android NDK).

@akien-mga
Copy link
Member

I tried this fixup locally, and that fixes the build for Android, but then there's a link error for those EXT symbols: https://hastebin.com/nazajegiti

@clayjohn
Copy link
Member Author

clayjohn commented May 1, 2019

@akien-mga I added es2 to glad.h and then included it for iphone and android. It builds successfully for android now, but I am unable to test iphone. I had to change an SCsub file in order to get scons to compile glad.h when using android or iphone.

@clayjohn clayjohn force-pushed the GLES2-MSAA branch 2 times, most recently from c130f0c to e9258fe Compare May 1, 2019 16:47
@clayjohn clayjohn force-pushed the GLES2-MSAA branch 2 times, most recently from 8725b65 to 7a87aae Compare May 2, 2019 06:13
@clayjohn
Copy link
Member Author

clayjohn commented May 2, 2019

@akien-mga Just pushed another version. Now im loading the needed functions for ios and android manually. Turns out ios also has a different way of handling multisampled buffers.

Also, I can't build for ios so I'm pretty much just relying on travis-ci to let me know if I made a mistake here...

@clayjohn clayjohn force-pushed the GLES2-MSAA branch 5 times, most recently from 74bfda6 to a3d5aec Compare May 2, 2019 21:23
@clayjohn
Copy link
Member Author

clayjohn commented May 2, 2019

@godotengine/ios Can I get someone from the ios team to test this on an ios device?

@akien-mga This is finally passing travis-ci!!

@reduz Can you review this again as I made a few changes to get it working with ios and android?

@akien-mga
Copy link
Member

As discussed on IRC, there are some concerns about how this will work on real GLES2-only devices (was only tested on GLES3-capable devices) and especially on iOS.

Since we don't have a battery of devices nor QA testers available to check it before merging, we'll go with our usual workflow: test in production. Please give the feature a try in the master branch and report any issue you encounter.

To release maintainers: this means of course that it shouldn't be cherry-picked for the 3.1 branch right away. We should wait until we're confident that tests in the master branch confirm that the feature works well everywhere.

@akien-mga akien-mga merged commit e0517a1 into godotengine:master May 13, 2019
@akien-mga
Copy link
Member

Thanks a lot!

@akien-mga
Copy link
Member

Note to release maintainers: if we decide to cherry-pick this, there are quite a few follow-up PRs to include (search "MSAA" in merged PRs). Overall probably not worth the trouble given that the 3.2 release is imminent and includes a lot of GLES2 fixes.

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.

Anti aliasing in 3.1 GLES2 Renderer
3 participants