Skip to content
This repository has been archived by the owner on Feb 1, 2024. It is now read-only.

GLES2 MSAA settings not working #54

Closed
NeoSpark314 opened this issue Oct 6, 2019 · 17 comments · Fixed by godotengine/godot#33291
Closed

GLES2 MSAA settings not working #54

NeoSpark314 opened this issue Oct 6, 2019 · 17 comments · Fixed by godotengine/godot#33291
Milestone

Comments

@NeoSpark314
Copy link
Collaborator

NeoSpark314 commented Oct 6, 2019

The MSAA settings (Under Project Settings->Rendering->Quality->Msaa seem to have no effect on the actual rendering inside the Oculus Quest in GLES2.

Here is a comparison shot with MSAA off (left) vs MSAA 4x (right):
mass_comparison_off_4x

I tried to investigate briefly; My first suspicion is that in void RasterizerStorageGLES2::render_target_set_external_texture(RID p_render_target, unsigned int p_texture_id): https://github.com/godotengine/godot/blob/1d9233c3882afe888b9396f7f2aac917d4dcac4d/drivers/gles2/rasterizer_storage_gles2.cpp#L5063
no check if msaa is enabled (and no call to glRenderbufferStorageMultisample or glFramebufferTexture2DMultisample is performed) when the external eye target texture is attached to the framebuffer for rendering.

@creikey
Copy link
Contributor

creikey commented Oct 6, 2019

Relevant PR: godotengine/godot#28518

@NeoSpark314
Copy link
Collaborator Author

I used some printf debugging and the check added in the PR godotengine/godot#28518 you referenced seems to work on the oculus quest and it is correctly reported that MSAA is supported.
What I mentioned above is that I think the special function render_target_set_external_texture used to render to the (external) eye textures is missing perhaps some multisample setup for the fbo.

@m4gr3d m4gr3d added this to the 1.0.0 milestone Oct 29, 2019
@m4gr3d
Copy link
Collaborator

m4gr3d commented Oct 29, 2019

cc @BastiaanOlij

@BastiaanOlij
Copy link
Member

As far as I'm aware the MSAA implementation in Godot was a GLES3 only feature. I vaguely recall a discussion on IRC about creating an alternative for the GLES2 driver.

That said, @NeoSpark314 could be correct in his remark.

@m4gr3d
Copy link
Collaborator

m4gr3d commented Oct 30, 2019

@NeoSpark314 Can you open an issue on the Godot engine project. It'd be ideal if we can resolve this issue before 3.2 is final.

@NeoSpark314
Copy link
Collaborator Author

Yes I'll create one; I just tested it again and the problem is still present. From what I have seen I would assume that the same issue exists also with desktop VR headsets on the GLES2 renderer but I have no hardware to verify this.

@NeoSpark314
Copy link
Collaborator Author

Issue godotengine/godot#33188 is created.

@NeoSpark314
Copy link
Collaborator Author

I managed to get an old Oculus DK2 and tested the proposed fix on Windows. As @clayjohn already suspected in a comment it does not work on desktop that way.

My current understanding of this is, that for desktop it would require the external texture to be created with MSAA via glRenderbufferStorageMultisample (as it is done here https://github.com/godotengine/godot/blob/500863859ca34fec245cdf9ff4bc236a308904c4/drivers/gles2/rasterizer_storage_gles2.cpp#L4725); or is there another option for the swap chain textures?
But this would need to be done in the Oculus Plugin (as well as the OpenXR plugin) and can't be done within the core (@BastiaanOlij do you know if this is correct or if there are other ways to render multisampled to the external textures provided by the plugins?) .

Independent of this would it be an option to have the android only fix first? It would resolve the issue for Oculus Quest and would not introduce a regression compared to the current state.
If my reasoning is correct, followup issues for plugins that use texture swap chains on desktop would need to be created.

@andy-d1969
Copy link

@NeoSpark314 I compiled godot master with your pull request #33291 applied, but MSAA is still not working for me. I tried importing your Godot Oculus Quest Toolkit project and set MSAA to 4x in the quality settings. When I run it in the editor MSAA is working, but not on my Quest.

@NeoSpark314
Copy link
Collaborator Author

@andy-d1969 thanks a lot for also looking into this. I will try it again in a moment.
How do you check if MSAA is working? Keep in mind that all the text and ui elements will not be affected/improved with MSAA as they come from a texture. You will only notice MSAA at the borders/silhouettes of geometry.

@andy-d1969
Copy link

I made a screenshot:
mpv-shot0004

@NeoSpark314
Copy link
Collaborator Author

Indeed no MSAA; hmm
I also just made two screenshots: 4xMSAA and MSAA off:
quest_msaa4x
quest_msaaOff

For me it works at the moment.

@NeoSpark314
Copy link
Collaborator Author

Maybe sth. went wrong while applying the PR or the build? If you want you could try the version I just built: https://www.dropbox.com/s/a1wgke4adufzdjd/android_debug.apk?dl=0

@andy-d1969
Copy link

I cannot install your apk:
$ adb install android_debug.apk
adb: failed to install android_debug.apk: Failure [INSTALL_PARSE_FAILED_NO_CERTIFICATES: Package /data/app/vmdl535797989.tmp/base.apk has no certificates at entry AndroidManifest.xml]`

The PR seems to be applied correctly:
$ git log
commit 8a452b3c4ba6e0ff6942fed1f6c6fee64c7fbead (HEAD -> master)
Merge: c663d65ff 418b035dd
Author: andy you@example.com
Date: Thu Nov 7 12:30:36 2019 +0100

Merge commit 'refs/pull/33291/head' of https://github.com/godotengine/godot

commit c663d65ffc4f4a6b9c6dd26ebe091c09cc6b8d6d (origin/master, origin/HEAD)
Author: Rémi Verschelde rverschelde@gmail.com
Date: Thu Nov 7 10:22:04 2019 +0100

New contributors added to AUTHORS:
@DavidSichma, @ptrojahn

New Platinum sponsor, added to splash screen:
Interblock

New Gold sponsor:
Image Campus

commit 706552404cd000fc011181a77189662ed9b6c97e
Merge: 99beb9afe 9fd416abc
Author: Rémi Verschelde rverschelde@gmail.com
Date: Wed Nov 6 22:34:27 2019 +0100

Merge pull request #33362 from qarmin/regression_control

Fix dragging spinner without control key                                                                                                                                                            

commit 99beb9afe618d648ad2283dd8cc60adb49d36492
Merge: 74c4543c4 134255166
Author: Rémi Verschelde rverschelde@gmail.com
Date: Wed Nov 6 22:33:13 2019 +0100

Merge pull request #33380 from SaracenOne/localise_path_fix                                                                                                                                         
                                                                                                                                                                                                    
Fix localise_path method

What could went wrong in the build? The binary is working.

@NeoSpark314
Copy link
Collaborator Author

NeoSpark314 commented Nov 7, 2019

Ah; sorry; I did not mention it; the .apk is just the godot android export template you would need to use instead of the one you build from godot.
But I just saw the godot PR with my fix is number #33188: godotengine/godot#33188; you mentioned a different number above

@NeoSpark314
Copy link
Collaborator Author

Oh; ups; please ignore; that was the issue number; PR is indeed godotengine/godot#33291

@andy-d1969
Copy link

andy-d1969 commented Nov 7, 2019

With your apk MSAA is working.
So something must be wrong with my Godot binary? I try to build again.

Edit: Ok, sorry, my fault. I did only rebuild the linux binary of the editor with your patch applied, not the android templates.

victordomiciano pushed a commit to JavaryGames/godot that referenced this issue Dec 3, 2019
When rendering to an external texture and MSAA was active (as happened
in the Oculus Mobile ARVR plugin) no MSAA was rendered as the correct
depth buffer and multisample texture target was not used.
This also fixes GodotVR/godot_oculus_mobile#54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants