Add MSAA mode for Quest #33518

BastiaanOlij · 2019-11-10T10:06:17Z

Due to performance issues we want to do a single pass render on the Quest. The Quest has a wonderful feature where we can directly render into the textures supplied by the Oculus SDK and have MSAA applied as long as we set our frame buffer up correctly.

There was a previous hacked version that attempted to do the same but broke the Rift plugin and potentially also the OpenXR drivers.

This PR introduces two new MSAA modes specific to the Quest and only work with the GLES2 driver (the GLES3 drivers effects setup make this approach impossible as far as I can tell so they just behave as the normal MSAA 2x and 4x modes).

When these modes are chosen Godot does not build its MSAA buffers instead setting up the FBO for the external texture correctly.
If these new modes are used on anything but Quest hardware MSAA remains turned off.

As we're already rendering to the external texture it now simply skips all the extra logic in post and the Quest does its thing.

BastiaanOlij · 2019-11-10T10:34:12Z

scene/main/viewport.cpp

@@ -3186,7 +3186,7 @@ void Viewport::_bind_methods() {
 	ADD_PROPERTY(PropertyInfo(Variant::BOOL, "transparent_bg"), "set_transparent_background", "has_transparent_background");
 	ADD_PROPERTY(PropertyInfo(Variant::BOOL, "handle_input_locally"), "set_handle_input_locally", "is_handling_input_locally");
 	ADD_GROUP("Rendering", "");
-	ADD_PROPERTY(PropertyInfo(Variant::INT, "msaa", PROPERTY_HINT_ENUM, "Disabled,2x,4x,8x,16x"), "set_msaa", "get_msaa");
+	ADD_PROPERTY(PropertyInfo(Variant::INT, "msaa", PROPERTY_HINT_ENUM, "Disabled,2x,4x,8x,16x,External 2x,External 4x"), "set_msaa", "get_msaa");


Should we just call these Quest 2x and Quest 4x?

Added another comment above about naming.

BastiaanOlij · 2019-11-10T11:22:18Z

@akien-mga you'll need to make a call on the fact we're adding a new feature after the feature freeze, I think this one is worth it :)

drivers/gles2/rasterizer_storage_gles2.cpp

m4gr3d · 2019-11-10T21:35:59Z

drivers/gles2/rasterizer_storage_gles2.h

@@ -1182,10 +1182,13 @@ class RasterizerStorageGLES2 : public RasterizerStorage {
 		struct External {


Unrelated to this PR, but the use of the term External might introduce confusion with GL_TEXTURE_EXTERNAL_OES textures.
Might be worth removing the confusing by renaming the struct in another PR.

Well I got there first ! :P

J/k but it is interesting, I never really got why that extension is called TEXTURE_EXTERNAL, from what I understand it simply allows the texture to be shared amongst processes so one process could be writing say camera data into it (ARCore) while another process uses it to render. What the underlying magic is that makes it special I'm not sure.

But the external texture feature in ARVR is a different thing and was added a while back, here we refer to a render texture supplied externally by a plugin. I'm all for a better name but we should apply it across the board, not just this struct but also the method call in the ARVR plugin should be renamed (seeing we use a struct of function pointers changing the name of a pointer won't have that much impact). I suggest though that we do this in Godot 4, I've been wanting to rename all the ARVR objects to XR, might aswell do a bunch of improvements in the naming of things.

m4gr3d · 2019-11-10T21:41:01Z

drivers/gles2/rasterizer_storage_gles2.cpp

+			// This code only applies to the Oculus Go and Oculus Quest. Due to the the tiled nature
+			// of the GPU we can do a single render pass by rendering directly into our texture chains
+			// texture and apply MSAA as we render.
+
+			// On any other hardware these two modes are ignored and we do not have any MSAA,
+			// the normal MSAA modes need to be used to enable our two pass approach


@BastiaanOlij There's no actual check that prevent this from actually being used on other Android devices, correct? By this I mean if an hardware manufacturer decides to follow Oculus's approach, there's nothing preventing a plugin for that hardware to leverage this logic.
So maybe we should leave the naming and description generic enough for Android devices.

Nope, but it does only work on Android atm. I tried using it on desktop but it turns out there is a specific extention on Quest that allows us to do this.
I'm not sure what would be the best name, we shouldn't make it too broad or else people are going to try and use it on normal Android devices while this only works with the VR plugins.

m4gr3d · 2019-11-10T21:41:38Z

scene/main/viewport.cpp

@@ -3186,7 +3186,7 @@ void Viewport::_bind_methods() {
 	ADD_PROPERTY(PropertyInfo(Variant::BOOL, "transparent_bg"), "set_transparent_background", "has_transparent_background");
 	ADD_PROPERTY(PropertyInfo(Variant::BOOL, "handle_input_locally"), "set_handle_input_locally", "is_handling_input_locally");
 	ADD_GROUP("Rendering", "");
-	ADD_PROPERTY(PropertyInfo(Variant::INT, "msaa", PROPERTY_HINT_ENUM, "Disabled,2x,4x,8x,16x"), "set_msaa", "get_msaa");
+	ADD_PROPERTY(PropertyInfo(Variant::INT, "msaa", PROPERTY_HINT_ENUM, "Disabled,2x,4x,8x,16x,External 2x,External 4x"), "set_msaa", "get_msaa");


Added another comment above about naming.

…imisation

akien-mga · 2019-11-11T10:46:53Z

Thanks!

@BastiaanOlij

A new external MSAA setting was introduced in godotengine#33518 that fixed issues on GLES2 and Oculus Mobile VR. To avoid misunderstanding it was suggested by @BastiaanOlij and discussed on discord to rename it to AndroidVR.

@BastiaanOlij

A new external MSAA setting was introduced in godotengine#33518 that fixed issues on GLES2 and Oculus Mobile VR. To avoid misunderstanding it was suggested by @BastiaanOlij and discussed on discord to rename it to AndroidVR.

BastiaanOlij requested review from reduz and a team as code owners November 10, 2019 10:06

BastiaanOlij force-pushed the msaa_ext_modes branch from 313f4ed to 6b2885c Compare November 10, 2019 10:30

BastiaanOlij changed the title ~~WIP: Add MSAA mode for Quest~~ Add MSAA mode for Quest Nov 10, 2019

BastiaanOlij commented Nov 10, 2019

View reviewed changes

BastiaanOlij force-pushed the msaa_ext_modes branch from 6b2885c to d357cbd Compare November 10, 2019 10:41

BastiaanOlij mentioned this pull request Nov 10, 2019

MSAA in GLES2 slow and breaks Fixed Foveated Rendering GodotVR/godot_oculus_mobile#70

Closed

clayjohn self-requested a review November 10, 2019 16:31

clayjohn reviewed Nov 10, 2019

View reviewed changes

drivers/gles2/rasterizer_storage_gles2.cpp Outdated Show resolved Hide resolved

m4gr3d reviewed Nov 10, 2019

View reviewed changes

akien-mga added enhancement platform:android topic:rendering labels Nov 11, 2019

akien-mga added this to the 3.2 milestone Nov 11, 2019

m4gr3d approved these changes Nov 11, 2019

View reviewed changes

Add special external MSAA modes for GLES2 Rift S/Quest and OpenXR opt…

4e23431

…imisation

BastiaanOlij force-pushed the msaa_ext_modes branch from d357cbd to 4e23431 Compare November 11, 2019 10:02

akien-mga approved these changes Nov 11, 2019

View reviewed changes

akien-mga merged commit 4b8feff into godotengine:master Nov 11, 2019

NeoSpark314 mentioned this pull request Nov 16, 2019

Rename External MSAA to AndroidVR MSAA #33658

Merged

BastiaanOlij deleted the msaa_ext_modes branch February 27, 2020 11:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MSAA mode for Quest #33518

Add MSAA mode for Quest #33518

BastiaanOlij commented Nov 10, 2019 •

edited

BastiaanOlij Nov 10, 2019

m4gr3d Nov 10, 2019

BastiaanOlij commented Nov 10, 2019

m4gr3d Nov 10, 2019

BastiaanOlij Nov 11, 2019

m4gr3d Nov 10, 2019

BastiaanOlij Nov 11, 2019

m4gr3d Nov 10, 2019

akien-mga commented Nov 11, 2019

		@@ -1182,10 +1182,13 @@ class RasterizerStorageGLES2 : public RasterizerStorage {
		struct External {

Add MSAA mode for Quest #33518

Add MSAA mode for Quest #33518

Conversation

BastiaanOlij commented Nov 10, 2019 • edited

BastiaanOlij Nov 10, 2019

Choose a reason for hiding this comment

m4gr3d Nov 10, 2019

Choose a reason for hiding this comment

BastiaanOlij commented Nov 10, 2019

m4gr3d Nov 10, 2019

Choose a reason for hiding this comment

BastiaanOlij Nov 11, 2019

Choose a reason for hiding this comment

m4gr3d Nov 10, 2019

Choose a reason for hiding this comment

BastiaanOlij Nov 11, 2019

Choose a reason for hiding this comment

m4gr3d Nov 10, 2019

Choose a reason for hiding this comment

akien-mga commented Nov 11, 2019

BastiaanOlij commented Nov 10, 2019 •

edited