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

[WIP] Fixed Timestep Interpolation (3D) - version four #53137

Closed
wants to merge 1 commit into from

Conversation

lawnjelly
Copy link
Member

Adds fixed timestep interpolation to the visual server.
Loosely based around godotengine/godot-proposals#2753

This is based on the latest suggestions from reduz. His latest suggestion is behaviour dependent on the node type, hopefully I have this correct from our last discussions:

Physics nodes

Interpolate by default (teleport when you call set_transform, I'm assuming)

Cameras

Interpolate by default, presumably do not teleport when you call set_transform (as that would be needed to move the camera). No mechanism to call teleport yet.

Other Spatials

Do not interpolate.

New node type

reduz suggested rather than allowing Spatials to turn off interpolation, we should add a new node type with the sole purpose of turning on and off interpolation. (?) Not yet implemented.

Notes

  • This isn't a full implementation, I don't think it is worth me doing any further work until a decision is made on which approach to take.
  • I would personally use Fixed Timestep Interpolation (3D) (3.x) #52846, and imo is far better than any of the other suggestions, it is just the simple standard implementation of fixed timestep, each node can turn on and off interpolation, and call teleport().
  • The implementation is this PR is highly convoluted and hard to follow, with the user having to understand that for some reason we have different behaviour for different node types. This makes no sense to me, both from a user and engine perspective.

Adds fixed timestep interpolation to the visual server.
@lawnjelly lawnjelly requested review from a team as code owners September 27, 2021 16:42
@lawnjelly lawnjelly changed the title Fixed Timestep Interpolation (3D) - version four [WIP] Fixed Timestep Interpolation (3D) - version four Sep 27, 2021
@Calinou Calinou added this to the 3.4 milestone Sep 27, 2021
Copy link
Contributor

@jordo jordo left a comment

Choose a reason for hiding this comment

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

Thanks for your work @lawnjelly, i know this one is a difficult one. I'll try and spend a little time on the other PRs as well.

if (instance->transform == p_transform) {
return; //must be checked to avoid worst evil
}
if (!Engine::get_singleton()->is_physics_interpolation_enabled() || !instance->interpolated) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I commented the same in rocket chat, but would love to see any settings for interpolation not be engine/global. As reduz suggested perhaps a property on scene tree, and perhaps something gets assigned when a node is entered in that tree.

}

void VisualServerScene::InterpolationData::notify_free_instance(RID p_rid) {
if (!Engine::get_singleton()->is_physics_interpolation_enabled()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, prev comment regarding engine setting global

@@ -4059,6 +4307,7 @@ bool VisualServerScene::free(RID p_rid) {

instance_owner.free(p_rid);
memdelete(instance);
_interpolation_data.notify_free_instance(p_rid);
Copy link
Contributor

Choose a reason for hiding this comment

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

these looks safe to notify after freeing the instance, but what if some one changes notify_free_instance in some unexpected fashion later on? Should not memdelete(instance); be the last operation here?

@lawnjelly
Copy link
Member Author

lawnjelly commented Sep 29, 2021

I'm just having a look at whether we can simply store the interpolation lists in the Scenario. The scenario seems to be created from the scene tree so this may be a solution to this global nature for multiple scene trees.

The only tricky bit is that the instances and cameras need quick access to the interpolation lists, for the set_transform commands and also the free commands. The instances store a Scenario pointer but the Cameras do not appear to, but that could be potentially be added (I don't know if there are any situations a Camera is used without a Scenario?).

Then you could turn on and off the global interpolation setting per scene tree (or scenario, if you weren't using a scene tree), which would hopefully deal with these use cases.

EDIT: This is implemented in #52846

@lawnjelly
Copy link
Member Author

Closing (at least temporarily) in favour of #52846.

@lawnjelly lawnjelly closed this Sep 30, 2021
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