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

Add motion vector support for animated surfaces #80618

Merged
merged 1 commit into from
Aug 17, 2023

Conversation

DarioSamo
Copy link
Contributor

Extends mesh instances that required custom vertex buffers to create two alternating buffers that are written to and binds them to use them as the previous vertex buffer when generating motion vectors.

Looking for some input on some of the implementation decisions.

@clayjohn
Copy link
Member

Just a general comment from testing, before I look at the code. I can confirm that this makes the ghosting from using skeletons go away, so great work so far!

I did notice an issue that we may want to investigate further. If an animation is stopped (either because the user paused it or it reached the end and isn't set to loop) then the current implementation continues to report motion vectors from the last frame which leads to continuous jitter on that mesh.

To debug this I multipied the motion vectors by 50 in copy_to_fb.glsl

Animation stopped (no MVs visible, TAA resolves nicely):
Screenshot from 2023-08-15 15-11-37

Animation reaches end (MVs frozen in place, TAA not resolving:
Screenshot from 2023-08-15 15-12-04

I guess we need a way to flag for the renderer that the skeleton hasn't updated in this frame, in which case it doesn't even need to write out motion vectors.

@DarioSamo
Copy link
Contributor Author

DarioSamo commented Aug 15, 2023

There's a few possible solutions to that.

Currently the one swapping the buffers is update_mesh_instances. If pausing the animation here makes that no longer update, then we could choose to validate the vertex array chosen based on the current frame number and the 'last change' stored in the surface itself (mesh_instance_surface_get_vertex_arrays_and_format most likely). The current implementation should handle that pretty gracefully if that's the only change we need to do.

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Left a couple comments from code review

@DarioSamo DarioSamo force-pushed the skeleton-motion-vectors branch 4 times, most recently from 716b1c5 to 1bbad6d Compare August 16, 2023 13:53
@DarioSamo DarioSamo marked this pull request as ready for review August 16, 2023 13:59
@DarioSamo DarioSamo requested a review from a team as a code owner August 16, 2023 13:59
@DarioSamo
Copy link
Contributor Author

Submitting for review, we might want some more extensive testing to spot any other issues if they arise.

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Just the one nitpicky comment. Otherwise this looks great! Tested locally and it works well

servers/rendering/renderer_rd/storage_rd/mesh_storage.h Outdated Show resolved Hide resolved
…nd shapes.

Extends mesh instances that required custom vertex buffers to create two alternating buffers that are written to and binds them to use them as the previous vertex buffer when generating motion vectors.
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Looks great! I tested locally and everything works.

While I don't anticipate any issues, I think the scope of changes is large enough that we shouldn't cherrypick this into 4.1

@clayjohn clayjohn modified the milestones: 4.x, 4.2 Aug 17, 2023
@clayjohn clayjohn added bug and removed enhancement labels Aug 17, 2023
@akien-mga akien-mga merged commit 0430677 into godotengine:master Aug 17, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

5 participants