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

Relax motion vector updates to allow skipped frames for skeletons #95705

Merged
merged 1 commit into from
Aug 19, 2024

Conversation

zeux
Copy link
Contributor

@zeux zeux commented Aug 17, 2024

Before this change, a skeleton that was not updated every frame would result in a difference of 2+ between last_change and frame index every frame, which would disable the buffer rotation and set motion vectors to zero. This results in significant visual artifacts for FSR2 that are especially prominent on the characters that move together with the view such as the main character in third person mode.

This is a significant problem for high refresh rate displays: at 120 Hz, we are effectively guaranteed to skip skeleton updates every other frame with skeleton update happening during physics processing, and the lack of physics interpolation for skeletons. This happens by default in TPS demo when FSR2 is enabled and resolution is below native:

image

In other places where motion vectors are disabled, such as multi-mesh and mesh rendering (where previous transform is updated), the logic effectively allows for a single-frame gap in updates, because it compares the frame where the update happened (which is the current frame if updates are consistent) with the current frame, so the latency of 0 means "update just happened", but both multi-mesh and mesh transform updates permit a latency of 1 as well:

if (inst->prev_transform_dirty && frame > inst->prev_transform_change_frame + 1 && inst->prev_transform_change_frame) {
inst->prev_transform = inst->transform;
inst->prev_transform_dirty = false;
}

bool MeshStorage::_multimesh_uses_motion_vectors(MultiMesh *multimesh) {
return (RSG::rasterizer->get_frame_number() - multimesh->motion_vectors_last_change) < 2;
}

Here, however, last_change is updated after the frame processing has concluded, so a zero-latency update has a distance of 1. Allowing a distance of 2 (latency 1) reduces the severity of the problem and aligns the logic with transform updates.

With this change at 120 Hz (and even 144 Hz) the problem is not visible:

image

Note that the problem will still happen when refresh rate is noticeably higher than physics rate times 2. For example, it still happens at 240 Hz. However, a longer latency allowance is inconsistent with other transforms and could lead to issues, so ideally long term physics interpolation of skeleton transforms would completely solve this.

@zeux zeux requested a review from a team as a code owner August 17, 2024 17:29
@Calinou
Copy link
Member

Calinou commented Aug 17, 2024

This makes sense to support for stepped updates for distant animations (which is common for NPCs in open world games), although I still maintain the main character really ought to have physics interpolation affect its skeleton.

@Calinou Calinou added bug topic:rendering topic:3d cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Aug 17, 2024
@Calinou Calinou added this to the 4.4 milestone Aug 17, 2024
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, it works as expected.

FSR2 Ultra Performance on 1080p:

120 FPS

Before

fsr2_ultra_perf_before_120fps.mp4

After

fsr2_ultra_perf_after_120fps.mp4

60 FPS

Before

fsr2_ultra_perf_before_60fps.mp4

After

fsr2_ultra_perf_after_60fps.mp4

TAA also benefits from similar improvements, although they are less drastic since it does not perform upscaling.

Before this change, a skeleton that was not updated every frame would
result in a difference of 2+ between last_change and frame index every
frame, which would disable the buffer rotation and set motion vectors to
zero. This results in significant visual artifacts for FSR2 that are
especially prominent on the characters that move together with the view
such as the main character in third person mode.

This is a significant problem for high refresh rate displays: at 120 Hz,
we are effectively guaranteed to skip skeleton updates every other frame
with skeleton update happening during physics processing, and the lack
of physics interpolation for skeletons. This happens by default in TPS
demo when FSR2 is enabled.

In other places where motion vectors are disabled, such as multi-mesh
and mesh rendering (where previous transform is updated), the logic
effectively allows for a single-frame gap in updates, because it
compares the frame where the update happened (which is the current frame
if updates are consistent) with the current frame, so the latency of 0
means "update just happened", but both multi-mesh and mesh transform
updates permit a latency of 1 as well.

Here, however, last_change is updated *after* the frame processing has
concluded, so a zero-latency update has a distance of 1. Allowing a
distance of 2 (latency 1) reduces the severity of the problem and aligns
the logic with transform updates.

Note that the problem will still happen when refresh rate is noticeably
higher than physics rate times 2. For example, it still happens at 240
Hz. However, a longer latency allowance is inconsistent with other
transforms and could lead to issues, so ideally long term physical
interpolation of skeleton transforms would completely solve this.
@akien-mga akien-mga merged commit 10b91ee into godotengine:master Aug 19, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

@zeux zeux deleted the fsr-skin-relax branch August 21, 2024 20:27
@akien-mga
Copy link
Member

Cherry-picked for 4.3.1.

@akien-mga akien-mga removed the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Sep 16, 2024
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