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

Improvements to VRS/Foveated rendering #89894

Merged

Conversation

BastiaanOlij
Copy link
Contributor

@BastiaanOlij BastiaanOlij commented Mar 26, 2024

This PR extracts the improvements to the fragment rate shading improvements from #85824, I wanted to extract these as I'm still having issues with the density map extension and will likely lift that to 4.4 while I'd like these changes to still make it into 4.3.

This also applies a few fixes I missed, makes the map we provide with VRS_XR match the format for VRS_TEXTURE, and adds controls to adjust the VRS map creates for VRS_XR by default.

Note as already is the case, VRS only works on the Vulkan renderer though I think we're close to adding support for DirectX and Metal. With DirectX its interesting that in multiview, we can't have separate shading rate maps per eye. We'll need to make our vrs.glsl copy function capable of combining the views and output a unified map. Currently no plans exist to add support for OpenGL/WebGL

Our default fallback implementation in XRInterface has now been moved into a helper object and is only implemented for MobileVRInterface and OpenXRInterface. It has been greatly improved over the older version.

Note we found out that Quest 3 now supports the fragment shading rate extension and this feature can be used and can be tested on Quest 3. Older Quests only support the fragment density map extension and require #85824 to be completed.
The improvements here should show a serious performance improvement on Quest 3, we already tested this without the improvements in control.

I've created an example Godot project that allows you to play with this without requiring XR hardware:
image

TestStereoVRS.zip

@BastiaanOlij BastiaanOlij marked this pull request as ready for review March 26, 2024 02:15
@BastiaanOlij BastiaanOlij requested review from a team as code owners March 26, 2024 02:15
@BastiaanOlij BastiaanOlij added this to the 4.3 milestone Mar 26, 2024
@BastiaanOlij
Copy link
Contributor Author

@DarioSamo this is giving me a:

ERROR: VALIDATION - Message Id Number: 1303270965 | Message Id Name: UNASSIGNED-CoreValidation-DrawState-InvalidImageLayout
        Validation Error: [ UNASSIGNED-CoreValidation-DrawState-InvalidImageLayout ] Object 0: handle = 0x1ff941e8660, type = VK_OBJECT_TYPE_COMMAND_BUFFER; | MessageID = 0x4dae5635 | vkQueueSubmit(): pSubmits[0].pCommandBuffers[0] command buffer VkCommandBuffer 0x1ff941e8660[] expects VkImage 0x6969c50000000b79[RenderBuffer VRS/texture] (subresource: aspectMask 0x1 array layer 1, mip level 0) to be in layout VK_IMAGE_LAYOUT_COLOR_ATTACHMENT_OPTIMAL--instead, current layout is VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL.     
        Objects - 1
                Object[0] - VK_OBJECT_TYPE_COMMAND_BUFFER, Handle 2197213316704
        Command Buffer Labels - 20
                Label[0] -  (L-1){ 0, 0, 0, 1 }
                Label[1] - Command graph (L10) (Draw){ 1, 1, 1, 1 }
                Label[2] - Command graph (L9) (Draw){ 1, 1, 1, 1 }
                Label[3] - Command graph (L8) (Draw){ 1, 1, 1, 1 }
                Label[4] - Shadow Render (L7) (Draw){ 1, 1, 1, 1 }
                Label[5] - Shadow Render (L6) (Draw){ 1, 1, 1, 1 }
                Label[6] - Render Opaque (L5) (Copy){ 1, 1, 1, 1 }
                Label[7] - Shadow Render (L5) (Draw){ 1, 1, 1, 1 }
                Label[8] - Shadow Render (L4) (Draw){ 1, 1, 1, 1 }
                Label[9] - Shadow Setup (L3) (Copy){ 1, 1, 1, 1 }
                Label[10] - Shadow Setup (L2) (Copy){ 1, 1, 1, 1 }
                Label[11] - Shadow Setup (L1) (Copy){ 1, 1, 1, 1 }
                Label[12] - Command graph (L0) (Draw){ 1, 1, 1, 1 }
                Label[13] - VRS Setup (L0) (Draw){ 1, 1, 1, 1 }
                Label[14] - Command graph (L0) (Copy){ 1, 1, 1, 1 }
                Label[15] - Shadow Setup (L0) (Copy){ 1, 1, 1, 1 }
                Label[16] - Setup Sky (L0) (Copy){ 1, 1, 1, 1 }
                Label[17] - Render Setup (L0) (Copy){ 1, 1, 1, 1 }
                Label[18] - Command graph (L0) (Copy){ 1, 1, 1, 1 }
                Label[19] - Command graph (L-1){ 1, 1, 1, 1 }

Any chance you can have a look at that? May be an existing validation error for VRS but just noticed it working on this. I think this may be caused by the ARG not recognising the VRS texture being used as a VRS attachment and not marking it as being read from in that pass?

@dsnopek
Copy link
Contributor

dsnopek commented Apr 2, 2024

I'm still not crazy about storing the VRS data on XRInterface and the default implementation in get_vrs_texture(). It would make more sense to me to put that on XRServer or some other central place, and then if VRS is enabled and get_vrs_texture() returns an invalid RID, call XRServer::get_default_vrs_texture() or something like that. To my mind, XRInterface should be only empty virtual methods that are filled in by the XR platform. Having it store data and provide default functionality feels wrong to me.

That said, it's not a hill I'm willing to die on. :-)

@dsnopek
Copy link
Contributor

dsnopek commented Apr 2, 2024

I tested this on Quest 3 using the demo project linked from PR #85824. I'm still seeing the same frame time gains that I saw when testing that issue, but I'm getting some weird flickering (even with VRS turned off) that I didn't get when I tested that PR a month or so ago. I tried switching to master and I'm seeing the same flickering, so I don't think it's caused by this PR, it seems like something unrelated.

So, in summary, testing seems good :-)

@BastiaanOlij
Copy link
Contributor Author

That said, it's not a hill I'm willing to die on. :-)

I get what you're saying, and maybe that is a nicer way to do it. What irks me is that this will likely always be the code path. It's kind of a weird one because it doesn't really belong to the XR implementation, it's a graphics implementation related to XR.

@BastiaanOlij
Copy link
Contributor Author

I tested this on Quest 3 using the demo project linked from PR #85824. I'm still seeing the same frame time gains that I saw when testing that issue, but I'm getting some weird flickering (even with VRS turned off) that I didn't get when I tested that PR a month or so ago. I tried switching to master and I'm seeing the same flickering, so I don't think it's caused by this PR, it seems like something unrelated.

This is a bug that is unique to the Quest 3 and that has been plaguing us since the Q3 came out. Its sporadic but I think our recent improvements to the rendering engine has made it more noticeable.

@BastiaanOlij
Copy link
Contributor Author

@dsnopek mind giving this another look? I've moved the logic into a helper object which solves your issue. I've also fixed a few things so it works well now.

@Malcolmnixon if you have time for it as well, that would be great, this solves how strong the default VRS is currently.

I've also added support for adjusting the VRS based on focus if the eye gaze interaction extension is available. Sadly we can't test this on Quest Pro yet as it requires the work in #85824 to be finished.

VRS does work nicely on Quest 3. You can use the following project to test:
https://github.com/BastiaanOlij/godot_openxr_demo/tree/add_more_testing
Note that I have changed the defaults in that project to play around with things so you may want to disable that code in its start_vr.gd script to see what things look like by default.

Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dsnopek mind giving this another look? I've moved the logic into a helper object which solves your issue.

This looks great to me! It solves all my concerns around storing data on XRInterface and having a Vulkan-specific default implementation.

VRS does work nicely on Quest 3. You can use the following project to test:
https://github.com/BastiaanOlij/godot_openxr_demo/tree/add_more_testing

I also tested on Quest 3, but using the same project I tested with last time (which is originally based on your test_vrs_mobile branch but updated for the latest API), rather than the project linked above.

It works great! I'm still seeing the same frame time gains as previously.

I only have a couple of nitpicks.

modules/openxr/extensions/openxr_eye_gaze_interaction.cpp Outdated Show resolved Hide resolved
modules/openxr/extensions/openxr_eye_gaze_interaction.cpp Outdated Show resolved Hide resolved
modules/openxr/openxr_api.h Outdated Show resolved Hide resolved
modules/openxr/openxr_interface.cpp Outdated Show resolved Hide resolved
servers/rendering_server.cpp Outdated Show resolved Hide resolved
modules/openxr/extensions/openxr_eye_gaze_interaction.cpp Outdated Show resolved Hide resolved
scene/main/viewport.cpp Outdated Show resolved Hide resolved
servers/xr/xr_vrs.cpp Outdated Show resolved Hide resolved
servers/xr/xr_vrs.cpp Outdated Show resolved Hide resolved
servers/xr/xr_vrs.cpp Outdated Show resolved Hide resolved
servers/xr/xr_vrs.h Outdated Show resolved Hide resolved
modules/openxr/openxr_api.cpp Outdated Show resolved Hide resolved
modules/openxr/openxr_api.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't re-tested, but with the latest changes, the code looks great to me!

@akien-mga akien-mga merged commit a2fc5e2 into godotengine:master May 4, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@BastiaanOlij BastiaanOlij deleted the improve_foveated_rendering branch May 7, 2024 00:49
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.

4 participants