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) (3.x) #52771

Closed

Conversation

lawnjelly
Copy link
Member

@lawnjelly lawnjelly commented Sep 17, 2021

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

Loosely based around godotengine/godot-proposals#2753

Notes

  • The no_draw function is historical and may not be needed, it will probably get taken out.
  • Still have to put in some warning for users if they call the interpolated functions outside of the physics tick
  • Interpolation method will automatically switch from basis lerp to slerp where possible / necessary
  • InterpolatedCamera TODO

More Info

Physics objects automatically call set_global_transform_interpolated, which calls the interpolated functions in the visual server (these fallback to normal funcs if interpolation is switched off in settings). For user code, users should call set_transform_interpolated, set_global_transform_interpolated during the physics tick. This sets a propagated 'interpolated' flag in the node and children.

Implicit set_transform problem

A complication with the proposal has turned out to be that Spatialcontains a large number of functions that internally call set_transform. This will break interpolation if it currently active, as it will cause a teleport. We've been evaluating various methods to get around this problem.

The huge potential for future bugs due to this problem (both in the engine and in user games) leads me to believe we should take the simpler alternative approach provided in #52846, where instead of providing a new API, we simply add an physics_interpolated property to each node. Then we add a teleport function for the rare case when we want to break interpolation for a single tick. This is how fixed timestep interpolation is normally implemented - on by default, with a special case for teleports. This has the advantage of being a tried and tested approach, and it means in most cases games can be converted to and from fixed timestep interpolation simply by switching the feature off and on.

In this PR I've been trying various approaches suggested by reduz to try and solve this inherent problem in the proposal, but all of them have been problematic in some way in practice, leading to time spent debugging gdscript calls, which I fear may make this too difficult to use for users, or put them off using it.

The obvious extension to the explicit approach in the proposal is to change the entire set of functions available on Spatial which implicitly call set_transform, and add an interpolated argument to each:

	void set_translation(const Vector3 &p_translation);
	void set_rotation(const Vector3 &p_euler_rad);
	void set_rotation_degrees(const Vector3 &p_euler_deg);
	void set_scale(const Vector3 &p_scale);

	void rotate(const Vector3 &p_axis, float p_angle, bool p_interpolated);
	void rotate_x(float p_angle, bool p_interpolated);
	void rotate_y(float p_angle, bool p_interpolated);
	void rotate_z(float p_angle, bool p_interpolated);
	void translate(const Vector3 &p_offset, bool p_interpolated);
	void scale(const Vector3 &p_ratio, bool p_interpolated);

	void rotate_object_local(const Vector3 &p_axis, float p_angle, bool p_interpolated);
	void scale_object_local(const Vector3 &p_scale, bool p_interpolated);
	void translate_object_local(const Vector3 &p_offset, bool p_interpolated);

	void global_rotate(const Vector3 &p_axis, float p_angle, bool p_interpolated);
	void global_scale(const Vector3 &p_scale, bool p_interpolated);
	void global_translate(const Vector3 &p_offset, bool p_interpolated);

	void look_at(const Vector3 &p_target, const Vector3 &p_up, bool p_interpolated);
	void look_at_from_position(const Vector3 &p_pos, const Vector3 &p_target, const Vector3 &p_up, bool p_interpolated);

	Vector3 to_local(Vector3 p_global, bool p_interpolated) const;
	Vector3 to_global(Vector3 p_local, bool p_interpolated) const;

	void orthonormalize(bool p_interpolated);
	void set_identity(bool p_interpolated);

In addition this does not look possible for set_translation -> set_scale because these are accessors for the Transform properties and this is incompatible with the property panel.

Imo this is needlessly pushing the complexity onto the user, there is no good reason for a user to have to take care in each of these functions when at the end of the day the only thing being set is a single bool per node.

Adding this bool parameter to each function also means in practice searching through game code to find each instance of these functions and rewriting them, for no apparent reason.

Compromise

As a compromise here I've added an internal function _set_transform_auto, which is used internally in these above functions, and calls set_translate or set_translate_interpolated based on the current flag of the node, thereby shielding the user from this complexity.

This means that effectively:

  • calling set_transform() is a combination of set_interpolated(false); and set_transform()
  • calling set_transform_interpolated() is a combination of set_interpolated(true) and set_transform()

This does mean that if the user isn't intending to call either of these functions (e.g. in a Camera where they use a look_at function) they should remember to call either set_transform or set_transform_interpolated at startup to set the interpolated flag of the node. But again this seems more difficult to explain to a user who is scratching their head than to just say 'just set the interpolated property'.

Essentially the pros/cons of the two approaches are as follows:

Explicit calls

  • Changing a lot of game gdscript
  • Dealing with game / engine bugs where interpolation is broken, tracking down the offending calls
  • User confusion
  • This effectively means all engine programmers will need to be conscious of these differences, and call set_transform_interpolated when adding new game functionality - we will constantly get changes causing game breaking regressions.

Per node setting

  • Little to no changes to game code.
  • Adding a teleport to parts of the game where objects are moved long distances. (Note that this can be done automatically for objects entering the scene tree, so it is quite a rare occurrence in practice.)
  • Probably sensible to turn off interpolation globally when is_editor_hint is set, as most editor operations don't want / require interpolation.
  • There may be occasions in Engine code where we might want to add a teleport call. This should be rare, and if we miss one, the worst regression will be an object being interpolated for a frame when it shouldn't, which isn't that visually jarring, rather than game breaking bugs.

Other game changes necessary to get fixed timestep working

Typically for most games some small adjustment for the Camera is all that is required. The easiest is to process the camera in _physics_process and call the set_transform_interpolated or set_global_transform_interpolated funcs, or just inherit the position from an interpolated parent.

If processing a camera in _process you will currently need to do the target interpolation yourself, by recording the target previous and current positions etc. I had a little look into automating this, but on discussion with reduz we decided this might be best to leave to the user to start with.

e.g. This is 3d platformer demo running at 20 ticks per second with interpolation:
https://user-images.githubusercontent.com/21999379/133752437-4a749906-b574-429c-ac6f-b6f2ea4c21cd.mp4

Adds fixed timestep interpolation to the visual server.
Switchable on and off with project setting.
@lawnjelly
Copy link
Member Author

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

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