Fix changing physics interpolation in SceneTree at runtime

[KeyboardDanni]

Fixes #101192
Fixes #101195

If physics interpolation is disabled, newly created and updated objects don't update their interpolation data, which can cause severe problems if the setting is enabled globally via SceneTree::set_physics_interpolation_enabled() at runtime. This PR adds a notification, NOTIFICATION_TREE_PHYSICS_INTERPOLATION_CHANGED, which is used to perform a full interpolation reset on the nodes in the tree regardless of their visibility. Since SceneTree::set_physics_interpolation_enabled() is likely to only be called when changing in-game graphics settings, the performance cost of updating everything shouldn't be an issue. The existing physics interpolation reset functionality is unaffected.

This PR also fixes the camera no longer updating after SceneTree::set_physics_interpolation_enabled() is called.

[lawnjelly]

I want to have a proper look at this, I'm not sure this will be most appropriate fix yet.

[lawnjelly]

I had a look and although the gist is correct as we discussed, there a number of problems here that would be nice to avoid, so I would prefer to fix as in #101218 .

NOTIFICATION

Adding an extra NOTIFICATION for this rare event doesn't seem ideal, if we can avoid it. Essentially at the moment we just want to update the process mode on Cameras (which is cheap), and everything else is the same as a regular reset, so for now it seems more sensible to just use regular reset, rather than duplicating code and adding an extra code path.

Updating hidden items

I'm not keen on the approach of trying to update the VisualServer on hidden items. This goes against the Godot paradigm. Hidden objects are not updated, and there is no guarantee that they are up to date. There is considerable risk (future) of sending stale data to VisualServer, due to lazy updating. Or worse, making future contributors feel they need to keep hidden items up to date.

Instead what we tend to use is updating VisualServer on un-hiding items, and that is the preferred paradigm for physics interpolation. We probably don't deal with all cases yet (this is done on purpose as part of the roll out), but this will have to be done in the long term anyway, making updates of hidden items as done in this PR redundant.

[KeyboardDanni]

I had a look and although the gist is correct as we discussed, there a number of problems here that would be nice to avoid, so I would prefer to fix as in #101218 .

NOTIFICATION

Adding an extra NOTIFICATION for this rare event doesn't seem ideal, if we can avoid it. Essentially at the moment we just want to update the process mode on Cameras (which is cheap), and everything else is the same as a regular reset, so for now it seems more sensible to just use regular reset, rather than duplicating code and adding an extra code path.

Does having an extra NOTIFICATION enum hurt anything here? We already have NOTIFICATION_RESET_PHYSICS_INTERPOLATION and NOTIFICATION_TEXT_SERVER_CHANGED. The reason I added an extra notification is because a regular reset isn't enough to fix the bug, and I didn't want to break the existing logic.

Updating hidden items

I'm not keen on the approach of trying to update the VisualServer on hidden items. This goes against the Godot paradigm. Hidden objects are not updated, and there is no guarantee that they are up to date. There is considerable risk (future) of sending stale data to VisualServer, due to lazy updating. Or worse, making future contributors feel they need to keep hidden items up to date.

The update is only performed when the setting is switched, not on every frame. Again, the reason I went with a separate notification is so that regular resets can still ignore invisible items for performance. However, updates need to be sent to the VisualServer for the invisible items on a full reset, because the existing data in the server is already stale (and not only stale, but causing the object to vibrate erratically, like it's really bad).

Instead what we tend to use is updating VisualServer on un-hiding items, and that is the preferred paradigm for physics interpolation. We probably don't deal with all cases yet (this is done on purpose as part of the roll out), but this will have to be done in the long term anyway, making updates of hidden items as done in this PR redundant.

This will be an end-user nightmare. We don't ask users to remember and manually reset every individual object in the game every time someone toggles V-Sync or anti-aliasing. We shouldn't ask them to do it for the global physics interpolation setting either.

[rburing]

I would also prefer to keep things simple and not add another notification when it isn't really needed, it just clutters the API. Not updating invisible items was an intentional optimization, so the solution to stale data should be to do a reset when unhiding. I don't have time to look into things in detail now, but as far as I can tell your issue can be solved by @lawnjelly's PR plus a reset when unhiding.

[KeyboardDanni]

@rburing This misses the point as far as why I'm introducing a new notification, why the bug occurs, and why the PR solves the bug this way. Also worth mentioning that #101218 does not completely fix the issue.

Please see the original bug here: #101192

See also this comment where I mention that the workaround (and thus the proposed alternate fix) does not address the issue if objects start out invisible: #101192 (comment)

Essentially, we have a bug that is occurring because of said optimization. We don't initialize or update the interpolation information if physics interpolation is disabled, or the object is invisible. This is a reasonable assumption to make if the user always has physics interpolation enabled or disabled, because you don't pay for what you don't use.

However, this assumption falls apart if you change the physics interpolation setting in SceneTree at runtime. This is a thing that you can do, for example, in modern Doom and Quake source ports. This is also a thing Godot lets you do. So it seemed to me that this was a reasonable use of the API. The fact that physics interpolation breaks and objects shake violently when the API is used like this signifies that this is a bug.

Basically, here's what's going on:

• Scene is initialized with physics interpolation off in SceneTree.
• All 2D nodes in scene have uninitialized lerp data (including previous transform and lerp timestamp).
• Physics interpolation is enabled in SceneTree.
• 2D nodes that haven't moved will vibrate erratically, because they're repeatedly looping through an interpolation between world origin and their starting position with a bad lerp timestamp, because this data was never initialized, and wasn't initialized when switching on physics interpolation.

This can be partially worked around using reset_physics_interpolation(). The reason why this isn't a full solution is because of this scenario:

• Scene is initialized with physics interpolation off in SceneTree.
• All 2D nodes in scene have uninitialized lerp data (including previous transform and lerp timestamp).
• There is an invisible 2D node with congratulatory text that will be made visible when the player clears the level.
• Physics interpolation is enabled in SceneTree.
• All 2D nodes have their physics interpolation reset, except for invisible ones.
• The player completes the level, and the big "Congrats! You win!" text appears, but is shaking violently because it wasn't handled by the physics interpolation reset.

Not updating invisible items was an intentional optimization, so the solution to stale data should be to do a reset when unhiding.

Yes, you can work around this by calling reset_physics_interpolation() after the node is made visible. However, this is not a reasonable expectation to have of the end user, because the purpose of calling this function is to prevent interpolation after moving a node. If the node never moves, there's no expectation that this call is necessary. However, the user will have to call this anyway because the interpolation data is uninitialized.

And the user will have to do this for every single node in their game that could potentially be invisible when the physics interpolation setting is changed in SceneTree. This is what I mean when I say that this is an end-user nightmare. Users shouldn't be expected to have to do this in order to work around a bug that could potentially trigger epilepsy if one particular instance isn't caught.

Also, there really should be some way to force a physics interpolation reset even if the node is invisible. I can understand wanting to optimize for the general case, but users need some form of escape hatch to force a reset for all nodes. I don't like that I can't force a full reset even if I want to.

I would also prefer to keep things simple and not add another notification when it isn't really needed, it just clutters the API.

While implementing this fix, I didn't see a better way. There needs to be some way to reset invisible nodes, but I didn't want to break the existing logic. Ideally, the NOTIFICATION_RESET_PHYSICS_INTERPOLATION notification would have a flag to indicate whether to force reset for invisible nodes, but notifications can't have parameters, at least not in current Godot. A lot of nodes have their own physics interpolation reset logic, and I wanted the force reset logic to be nearby. The next best option was to introduce a second notification for this scenario.

I can think of two other ways about this, though. One is to introduce a second notification like before, but have it going the other direction. That is, the second notification is NOTIFICATION_RESET_PHYSICS_INTERPOLATION_PARTIAL and the existing reset_physics_interpolation() is moved over to this notification. Then NOTIFICATION_RESET_PHYSICS_INTERPOLATION is repurposed as a full reset. This would avoid the problem of physics interpolation resets not happening if custom code doesn't use the new notification, but it does change how these notifications are fired, which might go against existing expectations.

The other idea is to handle this without notifications, by introducing a function call that itself propagates to child nodes. My concern with this however, is that the logic will be too separate from the existing reset logic (for example, 3D needs some special code to perform the reset, and it's better to have all that stuff in one place, because DRY principle).

[lawnjelly]

We really appreciate your investment in this issue, but it would be best to close this particular proposal now.

As the physics interpolation maintainers, @rburing and myself have decided to go with the other PR to fix the issue, as it works better for overall plan for physics interpolation rollout.

Thank you for your time nonetheless, and we hope to see you contribute again in the future!

See: #102652 (comment)

Just to reiterate, we are aware that auto-resets on unhiding some nodes are not yet performed, that decision has been purposefully deferred so we can look at them on a case by case basis.

It may well be we can address these soon, so any MRPs for unhiding nodes with moving starts would be welcome.