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

Implement motion vectors in mobile renderer #100283

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

devloglogan
Copy link
Contributor

This is the mobile renderer implementation of motion vectors, built on top of the renderer-agnostic #100282. A fair amount of discussion related to motion vectors has taken place in the compatibility renderer version of this PR: #97151.

If you'd like to test this PR out, build the OpenXR vendor repo with the PR adding support for XR_FB_space_warp: GodotVR/godot_openxr_vendors#222

Technical details

Since the forward+ renderer already implements motion vectors, the additions needed to get them working in the mobile renderer were pretty minimal. The biggest change is updating the vertex shader code of scene_forward_mobile.glsl, abstracting it into the _unpack_vertex_attributes()/vertex_shader() functions, similar to the forward+ renderer.

Motion vectors are rendered in a new, separate render pass. Here are the reasons for this (copied from #97151):

  • Since motion vectors can be rendered at a much smaller resolution this extra pass should be quite fast (e.g. the Quest 3 recommends a resolution of around 420x420).
  • This will make it easier to allow developers to include or exclude specific objects from motion vectors, which can be helpful in minimizing the artifacts from space warp. (Note: this isn’t part of the current PR, but something planned for the future.)
  • Combining motion vectors with the color pass would hinder future compatibility between Application Space Warp and the XR_META_recommended_layer_resolution OpenXR extension, since all attachments would then have to be of the same resolution.

Issues that need to be addressed

  • In abstracting the vertex shader code into the vertex_shader() function, the new function seems unable to take an argument of type SceneData. Whenever trying to do this, the shader breaks and I receive VK_ERROR_UNKNOWN. To work around this, I am instead passing all of the needed, individual fields from the SceneData structs. This makes for a very long function signature and I'd like to change it if possible.

  • In scene_forward_mobile.glsl, the y values of prev_screen_position/screen_position need to be flipped, but they need to be flipped after they are passed to gl_Position in the vertex shader. Without flipping them, the color output of the motion vectors texture is incorrect. But if y is flipped earlier, the position is incorrect. Currently this is solved by flipping the y at the end of the fragment shader when calculating ndc coordinates, but this feels a bit hacky to me. Does anyone have ideas on how else this could be accomplished?

This PR will remain as a draft until the other PR it builds upon (#100282) is merged.

@devloglogan devloglogan added this to the 4.x milestone Dec 11, 2024
@devloglogan devloglogan changed the title Implement motion vectors in mobile renderer [DRAFT] Implement motion vectors in mobile renderer Dec 11, 2024
@Saul2022
Copy link

Nice job, does this mean we will be able to use fsr 2.0 and taa? (now that enabling it improves shadow quality a lot).

@devloglogan
Copy link
Contributor Author

Nice job, does this mean we will be able to use fsr 2.0 and taa? (now that enabling it improves shadow quality a lot).

This PR doesn't enable those. I'm not a rendering guy, so I couldn't even say if these changes make those techniques possible in the mobile renderer!

@devloglogan
Copy link
Contributor Author

#100282 has been merged! 🎉 So I'm opening this PR. :)

@devloglogan devloglogan marked this pull request as ready for review December 13, 2024 01:03
@devloglogan devloglogan requested a review from a team as a code owner December 13, 2024 01:03
@devloglogan devloglogan changed the title [DRAFT] Implement motion vectors in mobile renderer Implement motion vectors in mobile renderer Dec 13, 2024
@dsnopek
Copy link
Contributor

dsnopek commented Dec 13, 2024

I tried testing this on the Quest 3 together with GodotVR/godot_openxr_vendors#222 and the spacewarp sample that it includes, but unfortunately, it's not working for me.

Visually, it doesn't look like spacewarp is working (it just gets choppy when spacewarp is enabled, and none of the expect artifacts), and I don't see any colors on the motion vectors overlay (enabled using the adb comments from GodotVR/godot_openxr_vendors#222).

I get these two errors spammed in the console (regardless of whether spacewarp is enabled or not):

Selection_251

@devloglogan devloglogan requested a review from a team as a code owner December 13, 2024 16:50
@devloglogan
Copy link
Contributor Author

@dsnopek I accidentally dropped support for VK_FORMAT_R16G16B16A16_SFLOAT in openxr_vulkan_extension.cpp somewhere along the way in all the rebasing. It should be fixed now. :)

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.

Thanks!

It's working for me as expected now :-)

Approving from the perspective of XR. I'm not super knowledgeable about the "Mobile" renderer, but skimming the changes, the code looks OK to me. In fact, I'm pleasantly surprised at how small the changes were! But someone from the rendering team should give it an in-depth review.

Comment on lines 20 to +24
vec2 uv_offset;
uint instance_index;
uint pad;
uint multimesh_motion_vectors_current_offset;
uint multimesh_motion_vectors_previous_offset;
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be packed like the Forward+ version since we shouldn't use more than 16 bytes on Mobile (except when using the ubershader)

uint instance_index;
uint uv_offset;
uint multimesh_motion_vectors_current_offset;
uint multimesh_motion_vectors_previous_offset;

@@ -1050,6 +1066,25 @@ void RenderForwardMobile::_render_scene(RenderDataRD *p_render_data, const Color
breadcrumb = RDD::BreadcrumbMarker::REFLECTION_PROBES;
}

if (rb_data.is_valid()) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (rb_data.is_valid()) {
if (rb_data.is_valid() && p_render_data->scene_data->calculate_motion_vectors) {


#ifdef MODE_RENDER_MOTION_VECTORS
vec3 ndc = screen_position.xyz / screen_position.w;
ndc.y = -ndc.y;
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be necessary, have you validated the output against the output from the Forward+ Renderer? My guess is that this backwards

@clayjohn
Copy link
Member

Overall this is looking really good!

  • In abstracting the vertex shader code into the vertex_shader() function, the new function seems unable to take an argument of type SceneData. Whenever trying to do this, the shader breaks and I receive VK_ERROR_UNKNOWN. To work around this, I am instead passing all of the needed, individual fields from the SceneData structs. This makes for a very long function signature and I'd like to change it if possible.

  • In scene_forward_mobile.glsl, the y values of prev_screen_position/screen_position need to be flipped, but they need to be flipped after they are passed to gl_Position in the vertex shader. Without flipping them, the color output of the motion vectors texture is incorrect. But if y is flipped earlier, the position is incorrect. Currently this is solved by flipping the y at the end of the fragment shader when calculating ndc coordinates, but this feels a bit hacky to me. Does anyone have ideas on how else this could be accomplished?

Both of these sound very fishy. For the first one my guess is its an Adreno driver bug :( Did you get the same error when running on desktop?

For the second one, I suspect that the extension is requesting MVs in a forward backwards from our own. FSR2 has an option to tell the plugin what range the MVs are in, does Application space warp have something similar? Ideally we would render MVs with the same format that we do in the rest of the engine and then trust the plugin to scale them as needed.

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