Use a spec constant to control whether the MultiMesh branch is used in the vertex shader. #94289
Conversation
…n the vertex shader. This works around a bug on the Quest3 and slightly improves performance on all mobile devices at the cost of increased pipeline count.
That's something I was thinking about if this was indeed necessary. It does sound a bit random that we have to guess what particular branch will fail to work in this particular hardware until the issue is resolved. One workaround could be we just introduce a particular code path that if a mesh uses multimesh, we demand for the specialization to be compiled if it's encountered while rendering but only on the hardware where we know the bug exists. We'd add a stutter, but it'd only be for multimesh and perhaps until the issue is resolved that's enough. |
| @@ -178,8 +182,6 @@ void main() { | |||
| color_interp = color_attrib; | |||
| #endif | |||
|
|
|||
| bool is_multimesh = bool(instances.data[draw_call.instance_index].flags & INSTANCE_FLAGS_MULTIMESH); | |||
|
|
|||
There was a problem hiding this comment.
This is changing the way is_multimesh is detected in the scene_forward_movine.glsl shader, but non mobile shaders will continue using the old way. Is there a reason to not unify criterias in different shaders?
There was a problem hiding this comment.
Yes, we will be changing how multimesh are handled in both shaders in 4.4. But before we do that, we need to merge #90400 otherwise we will make the shader compilation stutter much worse.
This PR is a band aid fix for 4.3. It only targets Mobile since the Quest3 is the only device with this driver bug and the cost of adding a new spec_constant in the Forward+ shader is much higher. The way the Mobile renderer works right now, this change won't increase shader compilation stutter, so it is safe to make. In the Forward+ renderer this change could increase shader compilation stutter (its already a much bigger problem there) and since the Forward+ renderer can't be used on the Quest3, it doesn't really provide a benefit.
There was a problem hiding this comment.
This has been tested with The Mirror and resolves the flickering problem with the Quest 3.
I tested this using shaders, the mobile renderer and a large scene with a large terrain.
I was present when the fix was being developed and this was the best way forward for 4.3.
My performance felt good in the standalone headset, which is great.
|
Thanks! |
|
Thanks!!! |
* Android Support for The Mirror for Meta Quest 2/3 What does this do: - Major engine upgrade to a much newer version of Godot Engine, with the fixes we helped with on the meta quest 3. (https://github.com/the-mirror-gdp/godot/tree/gordon/rebase-rc-2 to be merged) - Adds performance advantages for quest2/quest3 and low end hardware. - In theory fixes the intel crashing issues thanks to us removing our blur shader for the UI. (it was a huge performance hit) - Allows the Game UI to be used from within VR, renders all existing UI. (Doesn't allow the build menu but this was out of scope for this work) - Ensures the player is not visible for themselves on the local client, this will ensure no weird clipping happens. - The mirror can run in standalone mode on the quest 2 and quest 3. - Android doesn't flicker anymore. (Thanks to the Godot team for their amazing help ❤️) Use a spec constant to control whether the MultiMesh branch is used in the vertex shader. godotengine/godot#94289 - GameUI had to be moved to GameUI.instance as GameUI.instance is set based on if you are using VR or if you are using the Desktop UI, they are very different. GameUI.instance is useful because it ensures that you point at the correct UI and that you don't need to delete/readd the game UI, this was a major refactor and hugely painful to get working, there may be issues we do not know about without wider testing. - Forces glow and various graphical settings off when using VR, this is because it must be disabled officially any post processing except MSAA should be disabled for VR, baring foveation and some settings under OpenXR. - Removes the GameUI autoload and replaces it with the GameUI singleton using GameUI.instance. This was a combination of the GodotEngine team's rendering fixes. Special thanks to them.
|
can this changes be cherry-picked for 4.2.3? @akien-mga |
|
I'll check with Clay what he thinks. |
…pec-constant Use a spec constant to control whether the MultiMesh branch is used in the vertex shader.
|
Yes, this can be cherrypicked for 4.2. I think it should be a clean cherrypick |
Fixes: #89985
This works around a driver bug on the Quest3 and slightly improves performance on all mobile devices at the cost of increased pipeline count.
After extensive debugging we narrowed this issue down specifically to the
is_multimeshbranch. We are 99% certain that there is a driver bug on the Quest3 hardware. For whatever reason, the presence of this branch causes the flickering. My current theory is that the hardware is running the branch and masking the result (which shouldn't happen with a uniform branch) and it is failing to mask the result properly so data from that branch is leaking out. Since there is no transform buffer with regular meshes, we end up with an invalid model matrix which causes the meshes to jump around. Also note, that MultiMeshes don't exhibit the flickering.We found that manually removing the branch fixed the issue.
We have opted to use specialization constants to fix the issue for 4.3 as they have the same effect as removing the branch entirely. Luckily, this change improves performance on all platforms and shouldn't have a significant impact on pipeline compilation. Ironically, we already planned on moving this branch to a specialization constant, but we were waiting for the ubershader PR to be merged first. But since it isn't coming until 4.4, we need to do this separately.
One concern, the Ubershader uses a uniform buffer instead of specialization constants. If we are not careful, it may reintroduce this bug, but only for the Ubershader variants (which would make the flickering happen, but only rarely). CC @DarioSamo I think we will need to ensure that non multimesh specialization is compiled at load time. The multimesh specialization can fall back on the Ubershader.
This PR was a joint effort between Bastiaan, David, Dario, Gordo, and I. Together we brainstormed and arrived at this change by process of elimimination. For posterity, here is an (incomplete) list of other problems we checked before arriving at this workaround: