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

Fix get_global_transform_interpolated() with multiple ticks per frame #58381

Merged
merged 1 commit into from
Feb 28, 2022

Conversation

lawnjelly
Copy link
Member

@lawnjelly lawnjelly commented Feb 21, 2022

The previous and current transforms in the interpolation data were not being correctly updated in cases where two or more physics ticks occurred on a frame. This PR introduces a simple mechanism to ensure updates on interpolated spatials.

On suggestion from reduz this now uses a SelfList to store the list of Spatials to update, and this list is stored in the SceneTree. Members of this update list have a timeout value in order to reduce processing - if get_global_transform_interpolated() has not been called in a certain number of physics ticks, it is removed from the update list until the next time get_global_transform_interpolated() is called.

Note that the update list ONLY copies the current transform to the previous transform and updates the global transform. It does not perform any interpolation, that is performed on request within the get_global_transform_interpolated() function.

Explanation

Previously, if there were 2 or more physics ticks in a frame (for instance setting physics tick rate to 90tps, at a frame rate of 60fps), then the previous transform would only be set once, and would end up being a stale previous transform, which would result in the the interpolated value being further back in time than it should be, giving a visual effect of jitter.

Notes

  • This bug only occurred when using client interpolation using the get_global_transform_interpolated() function (e.g. for a Camera).
  • It only occurs when the tick rate is high enough that 2 or more physics ticks occur per frame (which is why I didn't notice it earlier).
  • The exact same bug also came up in the smoothing addon (!) 😁

@lawnjelly lawnjelly force-pushed the fti_fix_double_ticks branch 3 times, most recently from 9c3637b to 770f93a Compare February 21, 2022 08:44
@lawnjelly lawnjelly marked this pull request as ready for review February 21, 2022 08:58
@lawnjelly lawnjelly requested review from a team as code owners February 21, 2022 08:58
@lawnjelly lawnjelly added this to the 3.5 milestone Feb 21, 2022
@lawnjelly lawnjelly marked this pull request as draft February 21, 2022 12:43
@lawnjelly lawnjelly force-pushed the fti_fix_double_ticks branch from 770f93a to fd5067a Compare February 21, 2022 18:48
@lawnjelly lawnjelly marked this pull request as ready for review February 21, 2022 18:52
@lawnjelly lawnjelly force-pushed the fti_fix_double_ticks branch from fd5067a to e20e0e0 Compare February 21, 2022 19:06
@lawnjelly lawnjelly marked this pull request as draft February 21, 2022 20:11
@lawnjelly lawnjelly marked this pull request as ready for review February 22, 2022 16:00
@lawnjelly lawnjelly force-pushed the fti_fix_double_ticks branch from e20e0e0 to 1907325 Compare February 22, 2022 19:56
@lawnjelly lawnjelly requested a review from a team as a code owner February 22, 2022 19:56
@lawnjelly lawnjelly force-pushed the fti_fix_double_ticks branch 2 times, most recently from 889c887 to d8a95f0 Compare February 23, 2022 08:38
@lawnjelly lawnjelly marked this pull request as draft February 23, 2022 08:42
@lawnjelly lawnjelly force-pushed the fti_fix_double_ticks branch 3 times, most recently from 14fc02c to 180cf04 Compare February 23, 2022 12:45
@lawnjelly lawnjelly marked this pull request as ready for review February 23, 2022 12:45
@lawnjelly lawnjelly force-pushed the fti_fix_double_ticks branch 2 times, most recently from 72160b5 to d4dc702 Compare February 23, 2022 18:57
The previous and current transforms in the interpolation data were not being correctly updated in cases where two or more physics ticks occurred on a frame. This PR introduces a simple mechanism to ensure updates on interpolated spatials.
@lawnjelly lawnjelly force-pushed the fti_fix_double_ticks branch from d4dc702 to 688dc53 Compare February 25, 2022 11:28
reduz
reduz previously requested changes Feb 28, 2022
@@ -2240,6 +2240,7 @@ bool Main::iteration() {

if (OS::get_singleton()->get_main_loop()->iteration(frame_slice * time_scale)) {
exit = true;
Engine::get_singleton()->_in_physics = false;
Copy link
Member

@reduz reduz Feb 28, 2022

Choose a reason for hiding this comment

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

I think since this is a SceneTree related variable, it may be better to just put it SceneTree rather than here or in engine. Nodes have access to it via get_tree()

Copy link
Member Author

@lawnjelly lawnjelly Feb 28, 2022

Choose a reason for hiding this comment

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

Ok I'll take a look at this, could be better in a second PR maybe? (as is only side related to the interpolation, just used as a fallback for if get_global_transform_interpolated() was called from e.g. _process())

And it is a compatibility breaking change, so would benefit from some separate scrutiny.
EDIT - Yes it looks a little more involved than it may appear, because the Input depends on checking this, and due to timing of Input it may not be a good idea just to set it on and off at the beginning and end of SceneTree::iteration.

This line was a simple fix for an existing bug incidentally - the situation when exiting the app, the in_physics flag was never set to false (I picked this up when debug outputting error messages if get_global_transform_interpolated() was called from inside the physics.

@reduz
Copy link
Member

reduz commented Feb 28, 2022

I think this looks good, it remains to be seen how well it works when users test it, but I am positive.

@akien-mga akien-mga dismissed reduz’s stale review February 28, 2022 20:13

Changes for a follow up PR.

@akien-mga akien-mga merged commit 706d282 into godotengine:3.x Feb 28, 2022
@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.

3 participants