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

Fixed Timestep Interpolation (3D) (3.x) #52846

Merged
merged 1 commit into from
Feb 16, 2022

Conversation

lawnjelly
Copy link
Member

@lawnjelly lawnjelly commented Sep 19, 2021

Adds fixed timestep interpolation to the visual server.
Switchable on and off with project setting.

On request from reduz I have changed the teleport function name to reset_physics_interpolation.

(While some of the features will be shared with 2D, they are both fairly separable so I will be addressing 2D in a later PR. Once we are happy with the approach in 3.x, I will do similar for 4.x, perhaps after we get some initial testing.)

Details

  • Interpolation lists in the VisualServer are now stored in the Scenario (i.e. unique per scene tree).
  • There is an overall on / off bool setting per scene tree which can be changed by calling set_scene_tree_physics_interpolation_enabled() on the SceneTree, which defaults to the project setting physics/common/physics_interpolation (and is always off when is_editor_hint is set).
  • All nodes have a physics_interpolated flag, which can be read and set for individual control with set_physics_interpolated().
  • There is now a mechanism for retrieving interpolated transforms on frames : The command get_global_transform_interpolated().
  • CPUParticles and MultiMeshInstances now work via a modified MultiMesh that can handle interpolation.

Based around proposal : godotengine/godot-proposals#2753

Notes

  • This version often requires very little changes in terms of game code, all that is required is making sure that movement of objects and camera take place in _physics_process. Where changes are necessary these are most likely to occur in custom Camera code.
  • I moved the 'global' on / off setting, from Engine to SceneTree in response to comments from reduz and jordo. The interpolation lists are also moved to be per Scenario (i.e. SceneTree). This enables the whole system to be flexible for users who are not using the standard SceneTree.
  • As a consequence of moving the on / off setting to the Scenario, the Camera needs quick access to its scenario when calling camera_set_transform. As a result I've added storing the scenario in the Camera, in a similar manner to how it is stored in Instances.
  • I've moved the interpolation function for the Transform into a separate class TransformInterpolator. This was getting repeated in several places in the code (for instances, cameras, and potentially in the scene tree code) and it probably makes sense to put it in one place.
  • Having the interpolation code in one place makes it easier to adjust it, if we want to add e.g. Hermite in future.
  • The interpolation code automatically switches to a slerp where possible (this is dependent on the Basis).
  • We may want to eventually address audio as well, although I'm not sure it needs to be done in the same PR.
  • I removed changes to InterpolatedCamera. These can be done in a separate PR once we decide on an approach.
  • There is a checksum optimization for checking for noop set_transforms. This isn't strictly speaking necessary, so try not to get confused by this (it's cheaper than comparing 2 transforms each time, which could be important with a bunch of non-moving objects).

get_global_transform_interpolated()

It turns out that in a practical sense, especially for Cameras, it is often necessary to turn off the automatic fixed timestep interpolation and interpolate manually. See below for the problems with @Calinou 's project for an example - mouse looks better without fixed timestep interpolation, either with no interpolation, or using non-fixed timestep based interpolation. This means in _process users will want to be able to focus on the displayed (i.e. interpolated) position of an object, rather than the position at the last physics tick.
The same issue of needing to know the transform on a frame rather than a physics tick occurs when you need to make something emit at _process rather than _physics_process.
For these reasons, the PR includes an implementation for selectively duplicating the interpolation client side, i.e. within the scene tree. The reason for this requirement is that interpolation usually takes place in the VisualServer, and the queue / threading system means that synchronization is difficult to retrieve information from the VisualServer - it may cause a stall.
The solution used involves storing extra optional InterpolationData on the Spatial. This mechanism only is activated the first time the user calls get_global_transform_interpolated() on a node. This means there is no cost for the vast majority of nodes, both in terms of memory and processing.

MultiMeshes

It turned out that a large hurdle was handling MultiMeshes in a reasonable manner. As well as for MultiMeshInstances, these are used by CPUParticles under the hood. MultiMeshes use instancing in GLES3.

As a multimesh is a list of instances, in theory the main work is to maintain a list of previous and current data (transforms, colors and custom data), and interpolate between this. In practice however there are two snags:

  1. From CPUParticles, the order of the instances (particles) is not coherent from frame to frame. They may be drawn in e.g. z draw order etc. This means we cannot use a simple round robin list of the previous and current data for each instance.

The solution I have used here is the simplest conceptually, but it does impose some overheads. Basically for each bulk update of the particle instances (on a physics tick), instead of sending just the current data, it sends the current AND previous data every tick. This is wasteful, but it ensures that the data of the current and previous is always in sync.

This is achieved via a new alternative function:

virtual void multimesh_set_as_bulk_array_interpolated(RID p_multimesh, const PoolVector<float> &p_array, const PoolVector<float> &p_array_prev);
  1. In normal use by users it is expected the instances are more likely to remain in sync and the usual:
multimesh_instance_set_transform
multimesh_instance_set_color
multimesh_instance_set_custom_data

functions can be used as usual. There is also one other new function of interest:

virtual void multimesh_instance_reset_physics_interpolation(RID p_multimesh, int p_index);

which can be used to teleport individual instances.

How MultiMesh works internally

MultiMesh was previously implemented purely as classes within each backend, which was a problem, because ideally we want to share interpolation functionality between the two.
The solution was two create a set of wrapper functions in RasterizerStorage, which handle interpolation, and in turn call a similar set of underscored functions in the actual backends which are identical to the previous code.
In order to store interpolation specific data, the backends are responsible for creating a MMInterpolator (multimesh interpolator) object as part of their multimesh, and returning it when the following function is called:

virtual MMInterpolator *_multimesh_get_interpolator(RID p_multimesh) const = 0;

From here the Rasterizer storage can now handle storing previous and current data for each MultiMesh, and interpolating before a frame is drawn in a similar way to the Instances and Cameras.

One further gotcha is that MultiMesh does not have a Scenario associated with it, and could potentially be used like a resource in several MultiMeshInstances, potentially in different Scenarios (although unlikely). As the global interpolation flag is now a property of the SceneTree / Scenario, this makes the situation not quite so ideal, but a flag for interpolated is available on each MultiMesh and will be set by the corresponding instance that "claims" it.

This isn't perfect but it's a bodge around the problem that a MultiMesh isn't owned by a Scenario. In the vast majority of cases anyway the global interpolation will either be on or off, and providing a MultiMesh isn't used in two Scenarios with different SceneTree interpolation flags this shouldn't be a problem.

CPUParticles (in global (non-local) coordinate mode)

Along with the multimesh changes, there were additional changes to CPU particles to get them to behave correctly with interpolation.

Particle global coordinate space

CPUParticles were previously implemented in a slightly curious way in global mode. Although the particle positions are calculated in global (world) space, instead of specifying the particles in global coordinates, the instance in the VisualServer still had the transform inherited from the parent node. In order to compensate for this the particles are given an extra step to back transform them from global space to local space, by xforming by the affine inverse of the node transform.
This was seen as potentially problematic in terms of interpolation, and also was requiring more CPU than necessary. An obvious simpler solution is to set the instance to an identity transform, and just specify the particles directly in global space.
I have done this in the PR, and it did require some small adjustments but seems to be working correctly. It is possible that there may be more small compensations I have missed, but this seems well worth doing.

Camera code

The most likely area to require changes for existing games is Camera code. While cameras can be used as before except updating only in _physics_process and allowing fixed timestep interpolation to interpolate them, best results often involve turning automatic interpolation off and doing this manually.
When doing manual interpolation, as with the smoothing addon, it is recommended that in the SceneTree you separate the Camera from the object it is targetting, i.e. it is easier if the Camera is specified in global space and is not a child / grandchild of the object being targetted.
For custom interpolation, I recommend moving the Camera in _process, and calling get_global_transform_interpolated() on the target node, and using this interpolated position to focus on. The other calculations can be done as would normally be done for a camera.

Testing

Tested so far in:

  • Truck town demo
  • 3d platformer demo
  • Blubits demo
  • Test framework
  • SuperTuxParty
  • Veraball (some changes to Camera gdscript, see later in this thread)
  • Wrought Flesh

Works great in all so far at e.g. 20 ticks per second. In most cases no changes were needed to the games, just switching on interpolation.

@lawnjelly lawnjelly force-pushed the fixed_timestep_simple branch 3 times, most recently from cb65f28 to c1e7fb7 Compare September 19, 2021 14:50
@lawnjelly lawnjelly added this to the 3.4 milestone Sep 19, 2021
@lawnjelly lawnjelly force-pushed the fixed_timestep_simple branch 3 times, most recently from 6579493 to 0eedc19 Compare September 21, 2021 13:10
@lawnjelly
Copy link
Member Author

lawnjelly commented Sep 21, 2021

This should be getting pretty reasonable now, I could do with some people testing this out. The interpolation defaults to on (project_settings/physics/common/physics_interpolation), although it will likely default to off later for backward compatibility.

Instructions

Let me know any bugs / strange behaviour. Strictly speaking you'll also want to call reset_physics_interpolation on Nodes in some situations, but that's more for making releases.

@lawnjelly lawnjelly force-pushed the fixed_timestep_simple branch 2 times, most recently from 119d2e4 to 1664ebf Compare September 27, 2021 16:46
@Calinou Calinou added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Sep 27, 2021
@lawnjelly lawnjelly force-pushed the fixed_timestep_simple branch 4 times, most recently from da836ed to ae67e51 Compare September 30, 2021 14:59
@lawnjelly lawnjelly marked this pull request as ready for review September 30, 2021 17:24
@lawnjelly lawnjelly requested review from a team as code owners September 30, 2021 17:24
@lawnjelly lawnjelly changed the title [WIP] Fixed Timestep Interpolation (3D) (3.x) Alternative version Fixed Timestep Interpolation (3D) (3.x) Alternative version Sep 30, 2021
doc/classes/MultiMesh.xml Outdated Show resolved Hide resolved
doc/classes/MultiMesh.xml Outdated Show resolved Hide resolved
doc/classes/Spatial.xml Outdated Show resolved Hide resolved
core/math/transform_interpolator.cpp Outdated Show resolved Hide resolved
core/math/transform_interpolator.cpp Outdated Show resolved Hide resolved
doc/classes/MultiMesh.xml Outdated Show resolved Hide resolved
Comment on lines 654 to 661
<method name="set_physics_interpolated">
<return type="void" />
<argument index="0" name="p_interpolated" type="bool" />
<description>
Enables or disables physics interpolation per node, offering a finer grain of control than turning physics interpolation on and off globally.
[b]Note:[/b] This can be especially useful for [Camera]s, where custom interpolation can sometimes give superior results.
</description>
</method>
Copy link
Member

Choose a reason for hiding this comment

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

This and is_physics_interpolated could be exposed as a property.
Edit: I see you have it commented out, any specific reason?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did in fact expose it in earlier versions - when we discussed this @reduz preferred to have this hidden (I think with the aim of not overwhelming users with new concepts, or exposing the minimum possible).

On the other hand, if we get feedback expressing an interest to expose this I think we can easily re-add it.

doc/classes/Node.xml Outdated Show resolved Hide resolved
scene/main/node.cpp Outdated Show resolved Hide resolved
scene/main/node.cpp Outdated Show resolved Hide resolved
scene/main/node.h Show resolved Hide resolved
scene/main/node.cpp Outdated Show resolved Hide resolved
servers/visual/visual_server_raster.cpp Outdated Show resolved Hide resolved
@lawnjelly lawnjelly force-pushed the fixed_timestep_simple branch 2 times, most recently from 4b291dc to e22c225 Compare February 15, 2022 18:33
Adds fixed timestep interpolation to the visual server.
Switchable on and off with project setting.

This version does not add new API for set_transform etc, when nodes have the interpolated flag set they will always use interpolation.
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@reduz wanted a thorough review by @clayjohn, which could still be done post-merge - let's get this included in the next 3.5 beta to get broader testing.

@lawnjelly
Copy link
Member Author

@reduz wanted a thorough review by @clayjohn, which could still be done post-merge - let's get this included in the next 3.5 beta to get broader testing.

Great. BTW I've also done preliminary docs for the interpolation and occluder polys, which might be good to link in the beta notes:

https://github.com/lawnjelly/Misc/blob/master/FTIDocs/FTI.md
https://github.com/lawnjelly/Misc/blob/master/OccluderDocs/OccluderShapePolygon.md

I'll be improving these and doing some screenshots etc as I get the chance.

@akien-mga akien-mga merged commit 7974ea4 into godotengine:3.x Feb 16, 2022
@akien-mga
Copy link
Member

Thanks!

@lawnjelly lawnjelly deleted the fixed_timestep_simple branch February 16, 2022 12:40
@clayjohn
Copy link
Member

Looks good to me too! My apologies for taking so long to review the full thing

@mackatap
Copy link

I'm not sure if this should be submitted as a separate issue. Enabling physics interpolation seems to double the speed scale on the second and subsequent emissions of CPUParticles. Halving the speed scale to .5 makes the particles act as they should. The first emission seems to work as intended. I could upload a minimal project if needed, but it is just any CPUParticles.

@Diddykonga
Copy link
Contributor

Diddykonga commented Apr 18, 2022

https://github.com/godotengine/godot/issues
Any issues with already merged features should be posted there.
Please make sure to provide a minimal reproduction project, as it makes it significantly easier to find the issue. Thank you!

@lawnjelly
Copy link
Member Author

lawnjelly commented Apr 18, 2022

There are some significant changes to CPUParticles in the PR. This would be good posted as an issue with an MRP, meanwhile I will try to replicate it. It could be related to something being done / advanced twice, as we have to deal with both previous and current frames when using interpolation.

EDIT: Is ok, have found it, was quite simple, see PR below. 👍

@marcinn
Copy link
Contributor

marcinn commented Apr 19, 2022

Hi.

v3.5.beta.custom_build (from origin/3.x branch). I'm noticing issues with interpolation of collision shapes. It looks like they doesn't react to NOTIFICATION_RESET_PHYSICS_INTERPOLATION, which should be received "by the node and all children recursively" (as docs says).

https://www.youtube.com/watch?v=hVUG-7baPw4

@lawnjelly
Copy link
Member Author

lawnjelly commented Apr 19, 2022

v3.5.beta.custom_build (from origin/3.x branch). I'm noticing issues with interpolation of collision shapes. It looks like they doesn't react to NOTIFICATION_RESET_PHYSICS_INTERPOLATION, which should be received "by the node and all children recursively" (as docs says).

This is actually expected as far as I can see, nothing is technically going wrong. Collision shapes themselves are not interpolated (they expressly should not be), they are not derived from VisualInstance, and outside the editor they are not even visible. So they do not receive the reset physics interpolation signal.

The wireframe in the editor for the debug collision shapes is purely for debugging, I'm not sure offhand whether it is drawn in worldspace or simply attached to these non-interpolating collision shapes. The wireframe does seem to currently be interpolated (which is probably just the default).

It does look a little strange when teleporting I will admit. I can have a look into whether it can be made to match the graphical sphere (if you can create an issue with an MRP), or maybe we can simply turn off the interpolation flag for the wireframe, that might be better for visualization. But it is not a nasty "bug" per se, just an interesting side effect (that's my excuse lol 😁 ).

It is also possible I guess that people might hang some other VisualInstance from the CollisionShape I guess. I could also have a look whether the logic propagates past non-visual instances such as CollisionShape.

@marcinn
Copy link
Contributor

marcinn commented Apr 19, 2022

I have issues with collision detection when phys interp is on. I have an area containing (0,0,0), and when I'm adding a player to the scene and immediately "teleporting" it to the desired position (outside the area), the body_entered signal is emitting despite the player is moved (and drawn) outside the area.

Maybe I'm doing something wrong, so I would ask how to add a node to the scene into the desired place? I cannot set it's position before adding to the tree - Godot complains about this.

@lawnjelly
Copy link
Member Author

lawnjelly commented Apr 19, 2022

I have issues with collision detection when phys interp is on. I have an area containing (0,0,0), and when I'm adding a player to the scene and immediately "teleporting" it to the desired position (outside the area), the body_entered signal is emitting despite the player is moved (and drawn) outside the area.

Maybe I'm doing something wrong, so I would ask how to add a node to the scene into the desired place? I cannot set it's position before adding to the tree - Godot complains about this.

This needs an issue and a minimum reproduction project, and I can look into it. 👍
(We nearly always suggest creating fresh issues BTW, rather than posting in a PR for already merged work)

The MRP is necessary by the way because we have had several problems with this kind of thing in the past in the physics (way before interpolation was introduced), @pouleyKetchoupp did some workarounds I believe. This sounds less likely to be directly caused by the interpolation (ideally it should only affect the rendering layer, and not the physics / collision detection), but there may be some interaction going on, and debugging a reproduction project will show what is happening.

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.

10 participants