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

[3.x] Physics Interpolation - Move 3D FTI to SceneTree #103685

Merged
merged 1 commit into from
Mar 30, 2025

Conversation

lawnjelly
Copy link
Member

@lawnjelly lawnjelly commented Mar 6, 2025

Moves 3D interpolation from VisualServer to the client code (SceneTree).
Complete rework of 3D physics interpolation, but using the same user API.

  • API compatible / backward compatible with old system
  • Solves a large number of bugs with complex cases
  • Much easier user experience with physics_interpolation_mode OFF branches
  • Accurately renders scene tree pivot relationships

Fixes #103683 (for 3.x)
Fixes #104198 (for 3.x)
Fixes #103724 (for 3.x)
Fixes #103025 (for 3.x)
May help address #101823 (for 3.x)

Adds a SceneTreeFTI class with the intention of being used by third parties that write their own custom SceneTree, so
even custom versions of Godot can easily integrate FTI.

Fixes IK with root motion, Wheels, Pivot relationships, OFF branches

2025-03-10.14-32-37.mp4

Notes

Interpolating custom properties

Note: This section is aimed at maintainers rather than users.

The new system allows fairly simply interpolating a new property.
Let's say for example we want to interpolate a light color:

  • Define a current and previous value in your code:
    e.g.
Color light_col_prev;
Color light_col_curr;
  • Set your current value when the user changes this, and call fti_notify_node_changed()
  • Implement fti_pump(), this will handle pumping and resets automatically:
void MyClass::fti_pump() {
light_col_prev = light_col_curr;
Spatial::fti_pump(); // Call the parent class to ensure all data is pumped.
}
  • Implement fti_update_servers():
void MyClass::fti_update_servers() {
Color col = lerp(light_col_prev, light_col_curr, Engine::get_singleton()->get_physics_interpolation_fraction());
VisualServer::get_singleton()->light_update_color(instance, col);
VisualInstance::fti_update_servers(); // Call the parent class to ensure all data is sent to servers. 
}

Two passes

There is a chicken egg problem with get_global_transform_interpolated(). Ideally we want to concatenate scene tree xforms to calculate interpolated versions once after idle and immediately prior to rendering. However that would mean getting the interpolated xform during idle would be at best one frame out of date.

The solution to that is some clever management. First of all we do a pass before idle. This ensures that any initial calls to get_global_transform_interpolated() are up to date. When we change node xforms during idle, as part of the normal propagate we add a DIRTY_GLOBAL_INTERPOLATED flag to those altered nodes.

Then during the second pass, we refresh only those nodes that have this flag set. Thus we minimize any recalculation of moved nodes.

Strictly speaking we also can make subsequent calls to get_global_transform_interpolated() correct as to previous calls which may have set transforms during idle. This can be done by checking the dirty flag, and recalculating any dirty chains. I'm not sure yet whether this will be necessary.

The scene tree traversals in each pass are currently quite expensive. This is due to a number of factors, but primarily there is usually no need to traverse the whole tree, only altered branches, so this can be significantly optimized in future. Scene tree traversal itself is expensive due to cast_to and likely jumping around in memory and cache misses due to the nodes being stored in random memory locations.

Discussion

I'll refer to physics interpolation here by the more accurate name, Fixed Timestep Interpolation (abbreviated to FTI).

Although 3D FTI has been present in 3.x for 3 years, it hasn't had a lot of testing, and we are finally getting some decent testing in 4.x which has led me to propose changing the architecture to fix a number of areas / bug potential.

FTI in the VisualServer

The original decision for having the FTI take place in VisualServer was made by @reduz after I presented a number of alternative approaches. Note that 2D works completely differently from 3D, we only discuss 3D here.

There are some proposed advantages to having it take place in the server:

  • In the situation where FPS is higher than the physics tick rate, the traffic over the client-server command queue is reduced, there is the potential (in theory) to use less CPU.
  • FTI calculations can be done on the render thread instead of the main thread.
  • There is at least the potential that users of the VisualServer interface directly could take advantage of the existing machinery in the server.

In return there are a number of disadvantages:

  • The major disadvantage is that interpolated xforms are not available to scene side code. Retrieving them from the server is not an option due to synchronization and stalls.
  • Parent child relationships are not sent to VisualServer and only the global xform is sent. As a result PIVOT relationships in the scene tree cannot be accurately represented, and must be approximated, which results in a lot of visual anomalies. These can be reduced (but not eliminated) by running at high tick rate.

I was not convinced this approach was the best at the time, and tried my best to voice concern, but was ultimately only allowed to implement it in the server.

Alternatives

The obvious alternatives were:

  1. Perform FTI scene tree side (this is what my smoothing addon does)
  2. Send parent child relationships to the server, along with local xforms, and either calculate global xforms on the server or send both from the client

(2) is the approach used by 2D already (it calculates the global xforms during the render pass in the server), but there was a fear that changing the 3D to do the same would require significant rework.

Problems and special cases

Now some time has passed and we are getting a clearer idea of the problems, the biggest problem we face is that certain scene tree nodes require access to interpolated xforms in order to work, and they also need to operate with physics_interpolation_mode OFF, i.e. on the frame instead of on the tick.

Some examples:

  • Mouse controlled cameras
  • IK
  • Ragdoll
  • XR
  • Skeleton attachments
  • Following nodes
  • Skeletons in general

Workarounds

To this end I started adding workarounds to this problem, including the ability to try and duplicate the interpolation calculation for specific nodes in the scene tree using the get_global_transform_interpolated() machinery.

This works to an extend but there is a large bug surface particularly for when the flow of xforms is interrupted.

In order to sensibly work with branches that set their physics_interpolation_mode to OFF we want them to inherit the interpolated xform from their parent, but with the old system they inherit the physics xform which only moves each physics tick. This means we need to either have to burden the user with writing gdscript to do this, or manage the engine so that these nodes can automatically do client side FTI for their parent nodes, which is inefficient, error prone, and a potential user experience nightmare.

We are now facing a number of bugs in these above systems which other maintainers have to look at, and it is difficult to get their heads around the whole client / server separation of interpolation data, which makes me question, there must be a better way of addressing this, rather than adding more workarounds, bug surface, and future maintenance burden.

Revisiting the scene side approach

Earlier this week a user trying to convert their project from smoothing addon asked me the very valid question:

Why is the smoothing addon easier to use than core interpolation?

I started with the excuses about being constrained etc ... but realised that ultimately, for Godot, the user experience should trump everything else.

If USERS find it more difficult to use the current approach, then this alone should provide sufficient reason for us to explore alternative implementations, according to the Godot philosophy.

So this week we have come around to the bold idea that the simplest and most practical longterm solution for the engine may be to move the 3D FTI to be done scene side, and remove it from the server (with some small exceptions like multimesh for performance reasons).

I wrote a testbed this week, and it took just a day to implement the basic system that was far more robust and reliable than the server side system, and contained a lot less code. I then discussed with various other maintainers and we agreed that it likely made sense to proceed with this new method, initially in 3.x with a view to also changing in 4.x if the tests were successful.

@Zireael07
Copy link
Contributor

but was ultimately only allowed to implement it in the server.

WDYM?

@lawnjelly
Copy link
Member Author

WDYM?

As far as I remember as I said Juan wanted to try it in the server, which was a judgement call as he is the most senior Godot member. It's his baby, and he rightly has prime rights on architectural decisions (although these days most is delegated to area maintainers).

All maintainers have to make such judgement calls regularly. They don't always pan out, we just do the best we can. And it is fine to change stuff if we find a better way (although backward compat is always an issue). 🙂

@KeyboardDanni
Copy link
Contributor

Assuming the interpolation remains purely visual (which seems to be the case from the code changes), I'm all for this. Will we also make these changes for 2D?

@lawnjelly
Copy link
Member Author

lawnjelly commented Mar 10, 2025

Ok this is getting there, so I've marked ready for review.

I've just been fixing up some bugs in Camera and ClippedCamera today, and making some of the code more generic so it's easy to interpolate more values. I did get tempted to fix up interpolating zoom etc for Camera (I think @Calinou did a proposal on this), but I think that's better for a separate PR, it should be super easy now everything is done client side.

Tested in:

  • 3D Platformer
  • Kinematic Demo
  • IK Demo
  • TPS Demo
  • Wrought Flesh
  • Trucktown
  • Numerous FTI tests

Fixed (versus current 3.6):

  • Skeleton attachments
  • Wheels
  • IK

For XR I'll have to work with the XR team to get the best solution, in 4.x, but I'm hoping it will be a lot easier with this new approach.

The more testing the better so I would be grateful if anyone with any 3D 3.x projects could test. I'm expecting a few bugs / things I've forgotten to change, so shout out if you spot things.

There's still a lot of low hanging fruit for optimizing the scene tree traversal, but in the interests of making something as clear as possible I'd prefer to leave that to a later PR.

@Asaduji
Copy link
Contributor

Asaduji commented Mar 12, 2025

I'm not a reviewer or member of the XR team but since I'm who made the PR for the XR workaround (#103233) I can try this and give some feedback if you want

@lawnjelly
Copy link
Member Author

I'm not a reviewer or member of the XR team but since I'm who made the PR for the XR workaround (#103233) I can try this and give some feedback if you want

The more testing the better! 😁

@Asaduji
Copy link
Contributor

Asaduji commented Mar 12, 2025

Good job with this PR! It works exactly as I wanted it to work when I had to do the workaround in 4.x, I still have to do some changes to the XR nodes here because it really isn't designed to work with interpolation, those changes are minor and nothing to do with working around the interpolation, it works as intended!
Here is godot 3.x branch with the xr camera fix applied, completely unusable:

1662194_storage_emulated_0_Movies_ScreenRecording_ScreenRecording_2025.03.12-16.32.29.mov

Here is this PR, working perfectly out of the box (xr camera fix still has to be applied)

2528292_storage_emulated_0_Movies_ScreenRecording_ScreenRecording_2025.03.12-16.33.19.mov

The red box is interpolated and following the hand, everything running at 10 physics FPS to make it easier to see the behavior.

The changes required for XR are: ARVRServer not setting the world origin based on the interpolated position and ARVRCamera changing its position on every NOTIFICATION_INTERNAL_PROCESS while being interpolated.

For the first issue it would be changing arvr_server->set_world_origin(get_global_transform()); to arvr_server->set_world_origin(get_global_transform_interpolated());
For the second issue we have to either do tracked_camera->set_transform(t); on physics frame if the camera is interpolated or just set the camera interpolation to disabled by default

You can apply those changes here or I can make a new PR when this gets merged, up to you 🫡

@lawnjelly
Copy link
Member Author

That's great, I thought it would make the XR stuff quite a bit easier.

You can apply those changes here or I can make a new PR when this gets merged, up to you 🫡

I've done the first but I'll leave the second to you in a follow up as I'm not 100% sure where needed (and you'll get author credit).

@lawnjelly lawnjelly force-pushed the fti_scene_tree branch 2 times, most recently from 12731bd to 29844a4 Compare March 12, 2025 18:44
@lawnjelly lawnjelly requested a review from clayjohn March 14, 2025 07:30
@lawnjelly
Copy link
Member Author

lawnjelly commented Mar 14, 2025

I've marked @rburing and @clayjohn as these are most likely but anyone who can review that would be great as it would be super helpful if we can get this merged and tested asap and then potentially into 4.x too.

This will likely have to be YOLO merged anyway in 3.x (aside from obvious stuff), and then any regressions sorted, so I wouldn't worry about it being perfect straight off, as this kind of thing needs testing in the wild.

If there's delay here there is a risk we will be doing pointless PRs in 4.x to fix bugs in the old system, that may need to be reverted (there have already been several such PRs).

I'll try and have a look at a 4.x port while I'm waiting as sometimes that can expedite things.

Moves 3D interpolation from `VisualServer` to the client code (`SceneTree`).
Complete rework of 3D physics interpolation, but using the same user API.
@rburing
Copy link
Member

rburing commented Mar 19, 2025

Sorry for the late comment, I have been busy with work and other things. Could you elaborate on why you didn't go with alternative 2 (send parent child relationships and local xforms to the server)?

If you wanted to do interpolation for things that only exist on the server, then I guess with this PR you would create a Node and use the new virtual methods to make calls to the server (using the interpolation fraction). But it seems these virtual methods are not exposed to scripting. Even if the virtual methods are exposed, doing the interpolation would still be something to implement manually again, instead of a boolean toggle as it is now. And if the thing to interpolate is not a scalar like color but a Transform or a Basis, then you might want to use the TransformInterpolator functions, which are also not exposed to scripting. Admittedly this is a more advanced use case (which could justify having to write more code manually), but still it seems to be a feature that would be taken away, compared to the status quo.

If alternative 2 could allow both keeping the existing features and fixing the linked issues, then I think it would be better, even if it would be a bit more complex. Of course it could be that I am missing some other subtlety, so please let me know if there are any other disadvantages to alternative 2 or if I missed something else.

@lawnjelly
Copy link
Member Author

Could you elaborate on why you didn't go with alternative 2 (send parent child relationships and local xforms to the server)?

I did spend some time considering option 2 and discussed it briefly with Juan, and it was a hard no from him, and I understand why.

It would likely entail either:

  1. Re-engineering Godot engine 3D to work in a way more similiar to 2D (i.e. upload local xforms, and concatenate global xforms on the server).

This is something fundamental to how Godot engine works.

This is e.g. Godot 5 territory, and even then it would be so compat breaking it would likely not be possible to port existing projects from Godot 4. Realistically it's a non-starter I suspect.

Everytime you call get_global_transform() in scene code it relies on all this machinery to calculate global xforms client side, so effectively you would have to duplicate scene tree xform concatenation both client and server side, something that Godot has been architected to prevent.

  1. Put in a special case in physics interpolated mode to upload local xforms in addition to global xforms to the server, as well as parent child relationships, and duplicate all the transform concatenation on server as well as client.

This would likely be pointless, because we need access to interpolated xforms in the client, so we'd be doubling the work, and doubling the client-server traffic for no benefit. This also would add considerable complexity to server code, having to support essentially two separate pipelines.

Problems with server model

The key point that breaks the server model is we need access to interpolated xforms on the scene side, for IK, for XR, for Cameras, for Ragdoll, for follow targets, etc etc. We need some branches to be non-interpolated, and these need to inherit interpolated xforms from their parent node, otherwise these branches will judder on each physics tick (witness the issue reports with 4.4).

This is something the original proposal didn't envisage, and is not possible with the server model, except with workarounds to duplicate client side interpolation (as we do currently with get_global_transform_interpolated().

But that approach simply does not scale. Now it is clear this data is needed for every non-interpolated node (hence all the bug reports coming in now 4.4 is released). At a certain point it becomes cheaper to calculate all interpolated global xforms client side, and this is the point we have reached, and at this point there is no longer any need to calculate interpolated xforms server side, as it is simpler and cheaper to upload interpolated xforms instead of non-interpolated xforms. This is before we even get to the problems caused by the need to prime data for client side interpolation. Users won't have a hope.

Additionally it is clear that users don't understand the issues involved here in order to successfully use this model. From a search on discord, it's clear users barely understand the basic concepts of physics interpolation, let alone being able to deal with this client - server abstraction, whereas all this should be transparent to them.

Essentially at some point you have to recognise when you are using the wrong tool for the job. We are currently trying to use a car to cross the sea, adding various patches to try and make it waterproof, it's taking on water and we are frantically bailing to keep it afloat, and at a certain point you have to say "this is ridiculous, why don't we use a sailboat instead".

But it seems these virtual methods are not exposed to scripting.

Assuming I'm understanding you here - in the example, the "interpolating custom properties" section was aimed at maintainers (adding new node types / fixing bugs), not at users, I'll try and edit it to make this more clear.

It means in a few lines of code we can now easily make Cameras interpolate zoom, have colors interpolated, have animation systems interpolate blending values etc etc.

You are absolutely correct though that in the future this same system could potentially be used to help users write their own interpolated properties, but that is not the aim at this stage, that's a proposal all of its own.

Admittedly this is a more advanced use case (which could justify having to write more code manually), but still it seems to be a feature that would be taken away, compared to the status quo.

I'm not sure I understand, there is no such feature currently? Maybe I'm misunderstanding.

@rburing
Copy link
Member

rburing commented Mar 19, 2025

Thanks for the explanation! I'm starting to understand. I'll try to do a code review this weekend.

Sorry, I should have been more clear about the boolean toggle, I meant that this PR removes instance_set_interpolated, so it removes the feature of automagically interpolating the transforms of instances created on the server in user code.

@lawnjelly
Copy link
Member Author

lawnjelly commented Mar 19, 2025

Sorry, I should have been more clear about the boolean toggle, I meant that this PR removes instance_set_interpolated, so it removes the feature of automagically interpolating the transforms of instances created on the server in user code.

Ah yes, this is something I discussed briefly with @clayjohn . The intention is that SceneTreeFTI can be used from inherited SceneTrees for custom Godot versions.

You are correct if users are creating their own instances from script without using nodes, they are responsible for their own interpolation (if they want to do this).

It's possible we could add some helpers to make this easier (like an interpolated instance proxy), but probably for a separate PR. I don't think it would be a big deal.

UPDATE:
I've done a draft this afternoon for a small helper proxy to cater for people using instances directly from the server, which will do the global interpolation for them as before. I'll do a bit more on it after the weekend and will link the PR. There's a few alternative ways of doing this but can discuss on that PR.

Copy link
Member

@rburing rburing left a comment

Choose a reason for hiding this comment

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

The draft PR linked just above addresses my previous concern. I've had a look at the code in this PR and it looks good to me, simpler in a way. Also the fact that it fixes so many issues says a lot by itself. My only remaining concern is the performance of iterating over (branches of) the scene tree. Can we get some FPS (or other timing) numbers from nontrivial projects like some of the 3D demos? I understand wanting to leave optimization for a follow-up but it would be good to know the current state.

It would also be worth re-testing some of the MRPs of closed physics interpolation issues, to ensure the issues stay fixed.

The decision of whether to merge or not would benefit from input from @reduz (if he is available) since he was involved in the original discussions that led to the other approach being implemented first.

@lawnjelly
Copy link
Member Author

lawnjelly commented Mar 27, 2025

Although I mention more optimization above, that's because I'm an optimization guy (and I'm confident it can be made 10x faster at least than it is currently). In practice I don't think users will see any significant change to frame rates even from this un-optimized version, mainly because in most circumstances the GPU is the bottleneck.

Anyway for full info here is some figures:

3D platformer demo

Old: 395-398 fps
This PR: 395-398 fps

3D platformer demo, 16x16 window

Old: 2700-4000 fps
This PR: 2800-3800 fps
No interpolation: 2700-4000 fps

TPS demo (small window)

Old: 119-121 fps
This PR: 122-123 fps

Wrought Flesh

Old: 138-139 fps
This PR: 138-140 fps

As you can see in most cases things are within margin of error (or new PR is faster!).

Bear in mind that although the new method has a cost, so does the old method. This is using the debug/show_fps project setting, really it would be more stable if measurement was taken over a longer period, but this is kind of indicative. Also in practice the fps for small windows is probably limited by (rendering API) queue stuffing, hence there is a lot of variation.

The only one where I could coax a difference was in the 3D platformer with 16x16 window (to remove fill rate), where the max frame rate was possibly slightly slower.

I also include a figure for that demo with "no interpolation", where there is no measurable difference in FPS, which shows how cheap FTI really is.

It would also be worth re-testing some of the MRPs of closed physics interpolation issues, to ensure the issues stay fixed.

Yes, have already done this during development as best I could. Bear in mind there were a lot of issues, so I wouldn't be surprised if there was the odd one to fix. Also note that most of the historical bugs we faced were caused by physics interpolation taking place on the server, and this new approach fixes a lot of them automagically.

@lawnjelly
Copy link
Member Author

The decision of whether to merge or not would benefit from input from @reduz (if he is available) since he was involved in the original discussions that led to the other approach being implemented first.

I can leave this open a few days before merging (I pinged him in original post weeks ago), but bear in mind Juan is tending to be very hands off these days. He has afaik delegated day to day management to teams (and that was always the intention).

@clayjohn is now probably doing most management now for rendering, and we discussed this in private prior to the PRs, he's happy for us to go ahead (especially regarding testing in 3.x, then trying in 4.x), and really is leaving the decisions on physics interpolation up to us.

@KeyboardDanni
Copy link
Contributor

KeyboardDanni commented Mar 27, 2025

The 3D Platformer demo is a pretty basic scene. Wouldn't it be better to test by adding a couple hundred enemies to the scene so we have more moving objects to interpolate? Or maybe some extra coins and have them move around...

@lawnjelly lawnjelly merged commit 5155fe5 into godotengine:3.x Mar 30, 2025
14 checks passed
@lawnjelly lawnjelly deleted the fti_scene_tree branch March 30, 2025 14:46
@lawnjelly
Copy link
Member Author

Thanks!

@lawnjelly
Copy link
Member Author

@Asaduji reminder to you, if you are able to PR the second change you mentioned above, as this PR is now merged for 3.x, and we can get XR working.

I can review when you are ready.

@Asaduji
Copy link
Contributor

Asaduji commented Apr 1, 2025

@Asaduji reminder to you, if you are able to PR the second change you mentioned above, as this PR is now merged for 3.x, and we can get XR working.

I can review when you are ready.

I totally forgot 😅
Will get into it

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.

6 participants