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

Physics interpolation (2D) #88424

Merged
1 commit merged into from
Mar 24, 2024
Merged

Physics interpolation (2D) #88424

1 commit merged into from
Mar 24, 2024

Conversation

rburing
Copy link
Member

@rburing rburing commented Feb 17, 2024

This is a port of @lawnjelly's

to the master branch. It also takes bits

TODO:

  • Resolve questions marked by TODO in the code
  • Check snapping code
  • TileMap
  • Documentation (and mention that this only works for 2D for now)

Left for a follow-up PR (if it is acceptable):

  • CPUParticles2D

Thanks to @lawnjelly for the original PR.

Testing welcome! Make sure to enable the project setting physics/common/physics_interpolation and set physics/common/physics_jitter_fix to 0.0 when testing.

This PR is sponsored by My Spare Time™

scene/main/node.h Outdated Show resolved Hide resolved
Transform2D parent_xform = p_parent_xform;

if (snapping_2d_transforms_to_pixel) {
xform.columns[2] = xform.columns[2].round();
final_xform.columns[2] = final_xform.columns[2].round();
parent_xform.columns[2] = parent_xform.columns[2].round();
Copy link
Member

Choose a reason for hiding this comment

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

Note: If you are doing snapping here, you may want the snapping to apply to the local object, but NOT to propagate to the children, so they can do their snapping independently. This may entail creating an extra intermediate e.g. snapped_xform and use this for rendering, while passing the unchanged final_xform to the children.

This certainly needs consideration because snapping is done differently in 3.x.

Copy link
Member

@adamscott adamscott Feb 18, 2024

Choose a reason for hiding this comment

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

I tried multiple times snap the children independently (ask @markdibarry) but it always resulted with snapping issues. I think it's best to snap the parent as is and pass the parent to the children.

@lawnjelly
Copy link
Member

lawnjelly commented Feb 17, 2024

Great work @rburing . This is something I was not looking forward to 😁 as I'm not so familiar with master and there will inevitably be a lot of 4.x specific bugs / ongoing maintenance work.

Particles in particular have the potential to be tricky because (at least in 3.x) they were previously not specified in world space. They (and MultiMesh which CPUParticles used under the hood) were the most time consuming feature in 3.x, though more so for 3D.

Also we should pay particularly attention at whether CPUParticles are planned to be deprecated in 4.x (in which case you will have to get them working with GPUParticles). Also I noted that, at least in 3D, particles in 4.x already use some kind of interpolation so you will need to get both systems to play nice with each other.

Also as I noted above there may end up being some rejigging for snapping purposes. This is something I'm anticipating may need adjustment in 3.x based on feedback, as we haven't yet released 3.6. So in some ways this code may get more testing in 4.x (also bearing in mind there are now many more users on 4.x than 3.x). The paradigm is intended to be that transforms are interpolated and passed through the tree without snapping, and snapping is applied on a per object basis. Consider if a child node has a scaling applied, then trying to interpolate from snapped parent transforms would look incorrect otherwise.

BTW an option (if other reviewers are agreeable to this) that I used in 3.x is to leave particles to a followup PR. They will probably have a lot more bikeshedding / bugs, and it makes it easier to review both sections in depth. This does mean that realistically users will have to wait for particle PR to make finished games, but it does mean they can start testing etc more rapidly.

@Calinou
Copy link
Member

Calinou commented Feb 17, 2024

Also I noted that, at least in 3D, particles in 4.x already use some kind of interpolation so you will need to get both systems to play nice with each other.

Only GPU particles have built-in interpolation in 4.x, and it's not complete yet.

@rburing
Copy link
Member Author

rburing commented Feb 17, 2024

Thanks for the feedback @lawnjelly ! I think it makes sense to leave particles for a follow-up PR, if reviewers can agree.

I'll have to look at the snapping (propagation) more carefully, and I'll ask for some more eyes on this on RocketChat.

The latest push also fixes this issue with TileMaps:

physics-interpolation-streaking.mp4

@rburing rburing marked this pull request as ready for review February 17, 2024 22:06
@rburing rburing requested review from a team as code owners February 17, 2024 22:06
@rburing
Copy link
Member Author

rburing commented Feb 18, 2024

The test from the project PixelPerfectTest.zip from #84380 seems to pass with a physics tick rate of 30 (also lower ones) plus the interpolation from this PR, after commenting out the _process function in Camera.gd (it was setting the camera position in both _process and _physics_process):

physics-interpolation-pixel-test.mp4

This was recorded using Peek because movie writer mode is bugged when using the settings of the test project.

The potential issue #88424 (comment) with propagating snapped transforms to children should be present in master as well, so I'd prefer not to change the behavior in this PR. What would an MRP look like @lawnjelly? It can be reported separately.

doc/classes/SceneTree.xml Outdated Show resolved Hide resolved
doc/classes/ProjectSettings.xml Outdated Show resolved Hide resolved
doc/classes/ProjectSettings.xml Outdated Show resolved Hide resolved
@rburing rburing force-pushed the fti_2d branch 2 times, most recently from 336b826 to 406b409 Compare February 18, 2024 23:26
@Chaosus Chaosus added this to the 4.x milestone Feb 20, 2024
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally (rebased on top of master b8f106a), it works as expected. Amazing work!

Here's the 2D platformer demo with Physics Ticks per Second set to 10:

platformer.mp4

I noticed an issue when resizing the viewport when using the viewport stretch mode and a Camera2D. If the aspect ratio changes, the camera motion will be interpolated, even though this doesn't occur in discrete steps when physics interpolation is disabled. The Camera2D in the project has its Process Callback set to Idle (the default).

This is hardly noticeable with tickrates greater than 30 Hz, so it's not a huge issue, but it might be worth looking into nonetheless.

See these videos with Physics Ticks per Second set to 5:

Physics interpolation disabled

simplescreenrecorder-2024-02-20_18.15.04.mp4

Physics interpolation enabled

simplescreenrecorder-2024-02-20_18.15.37.mp4

In the bullet shower demo with Physics Ticks per Second set to 5, something strange happens with the sprite movement. It's interpolated even though we're moving it every frame in _input() based on mouse movement, so it can move faster than the physics tick rate.

The individual white sprites (that are not nodes but created via low-level servers) are also not interpolated:

Physics interpolation disabled

simplescreenrecorder-2024-02-20_18.26.11.mp4

Physics interpolation enabled

simplescreenrecorder-2024-02-20_18.25.52.mp4

Parallax backgrounds look strange when wrapping over with low physics tick rate (10 here), and they don't interpolate as smoothly as they could. They seem to interpolate in visible "steps", as if the interpolation was only performed for half of the physics tick length:

simplescreenrecorder-2024-02-20_18.37.21.mp4

Testing project: physics_platformer_modified.zip


Also, I strongly suggest enabling physics interpolation by default. We'll need to stick to a default value once this is merged, and with high refresh rate displays slowly becoming the norm among players, it's expected that games should adapt automatically without requiring extra work from the developer's side. While this will require updating some tutorials, I believe it's worth the effort.

@rburing
Copy link
Member Author

rburing commented Feb 22, 2024

@Calinou Thanks a lot for your testing!

I've rebased the PR and fixed the issue with parallax (it should not be interpolated, as it is already updated every frame).

I'm not sure what to do about the player sprite in the bullet shower demo. The bullets, with their transforms set manually every physics tick, are rendered using _draw with the position from the last physics tick, so it is expected that they are not interpolated automagically. They can be interpolated by modifying the _draw function e.g. as follows:

func _draw():
    var offset = -bullet_image.get_size() * 0.5
    for bullet in bullets:
        var interpolated_position = lerp(bullet.position + Vector2(bullet.speed / Engine.physics_ticks_per_second, 0), bullet.position, Engine.get_physics_interpolation_fraction())
        draw_texture(bullet_image, interpolated_position + offset)

(It could be improved a bit further by storing the current and previous position, thereby avoiding the hack to compute delta, and setting both the current and previous position when wrapping around, effectively resetting the interpolation.)

I haven't yet looked into the Camera2D interpolation when resizing the viewport with stretch mode set to viewport, but it seems a low priority issue.

It's easy to make physics interpolation the default (should it also be a basic setting then?), but currently this would also override the Engine.physics_jitter_fix to 0.0, which would be good for 2D with interpolation but maybe bad for 3D which is currently lacking interpolation.

@rburing rburing requested a review from a team February 22, 2024 20:52
@Calinou
Copy link
Member

Calinou commented Feb 23, 2024

It's easy to make physics interpolation the default (should it also be a basic setting then?)

I'm not sure if it should be made a basic setting, considering we want to encourage leaving physics interpolation enabled as much as possible. In the long run, physics interpolation is something you should only disable for troubleshooting, or for games intended to run at a fixed framerate (which is generally a bad idea, but can minimize input lag in this particular situation).

@lawnjelly
Copy link
Member

Consider if a child node has a scaling applied, then trying to interpolate from snapped parent transforms would look incorrect otherwise.

I've finally had a chance to test this (sorry was away from home when writing originally so was working from memory), and it doesn't seem to cause the problem that I envisaged, because the scaling turns out to be applied after the translation. (The problem I was thinking of was e.g. moving parent node by 0.9 pixels, then child being scaled 10x and this movement being lost, when it should have moved 9 pixels.) But this was based on the incorrect assumption of scaling being applied first, so it seems fine so far in some tests. 👍

@lawnjelly
Copy link
Member

lawnjelly commented Mar 9, 2024

reset_physics_interpolation() doesn't appear to be working for PointLight2D.
Not sure whether that is also the case in 3.x (it may be my mistake 😁 ).

EDIT: It's the same in 3.x, I'll try and work out why.

It's an order of operations bug, the transform notification is coming in after the reset, even if the reset is called after setting the transform in the script. I'll see if I can fix it in 3.x, and we can carry fix over.

Ok I have it:

The Light2D when calling set_transform() it updates the transform of the canvas_item immediately, however the light doesn't use this, instead uses a canvas_light RID and this only updates the canvas_light_set_transform() when a notification is received some time later.

This won't be too involved to fix but I can't guarantee I'll be able to do before going off on holiday tomorrow for a week, but I don't think this should be a showstopper for this PR (after all it is already merged in 3.x with this bug 😄 ).

This same bug could be affecting some of the other non-canvas items, but with any luck they can all be fixed in similar way.

UPDATE: Fixed in 3.x by #89577 . Should be fixed in this PR soon.

Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

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

What a great port. I tested it summarily (and it seems to work amazingly), but I think others tested it thoroughly.

The code seems OK! Didn't find anything requiring major changes, but this is a hefty PR, so I suggest that we merge for dev6 then we fix issues afterwards.

main/main.cpp Outdated Show resolved Hide resolved
scene/2d/camera_2d.cpp Show resolved Hide resolved
scene/2d/camera_2d.cpp Outdated Show resolved Hide resolved
@akien-mga akien-mga requested a review from reduz March 15, 2024 09:38
servers/rendering_server.h Outdated Show resolved Hide resolved
servers/rendering/rendering_server_default.h Outdated Show resolved Hide resolved
servers/rendering/renderer_canvas_cull.h Outdated Show resolved Hide resolved
servers/rendering/renderer_canvas_cull.cpp Outdated Show resolved Hide resolved
servers/rendering/renderer_canvas_cull.cpp Outdated Show resolved Hide resolved
servers/rendering/renderer_canvas_cull.cpp Outdated Show resolved Hide resolved
servers/rendering/renderer_canvas_cull.cpp Outdated Show resolved Hide resolved
scene/main/node.h Outdated Show resolved Hide resolved
Copy link
Member

@reduz reduz left a comment

Choose a reason for hiding this comment

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

Looks fantastic, really good work!

@markdibarry
Copy link
Contributor

markdibarry commented Mar 22, 2024

@rburing From your comment earlier, you probably want to add your new method to Parallax2D as well, since (like ParallaxLayer) it also relies on immediately jumping between two points for the infinite scroll effect:

void Parallax2D::_update_repeat() {
	if (!is_inside_tree()) {
		return;
	}

	Point2 repeat_scale = repeat_size * get_scale();
	RenderingServer::get_singleton()->canvas_set_item_repeat(get_canvas_item(), repeat_scale, repeat_times);
	RenderingServer::get_singleton()->canvas_item_set_interpolated(get_canvas_item(), false);
}

@rburing rburing force-pushed the fti_2d branch 2 times, most recently from 136338c to e07c2f9 Compare March 23, 2024 10:30
@rburing
Copy link
Member Author

rburing commented Mar 23, 2024

Thanks for the reviews! And thanks @markdibarry, it's done. This should be ready to merge now.

Adds fixed timestep interpolation to the rendering server (2D only).
Switchable on and off with a project setting (default is off).

Co-authored-by: lawnjelly <lawnjelly@gmail.com>
akien-mga added a commit that referenced this pull request Mar 24, 2024
Physics interpolation (2D)
@akien-mga akien-mga closed this pull request by merging all changes into godotengine:master in 06abc86 Mar 24, 2024
@akien-mga
Copy link
Member

Thanks! Amazing work!

@rburing rburing deleted the fti_2d branch March 24, 2024 07:01
Comment on lines 3968 to +3973
#endif // _3D_DISABLED

// Prepare the fixed timestep interpolated nodes BEFORE they are updated
// by the physics server, otherwise the current and previous transforms
// may be the same, and no interpolation takes place.
OS::get_singleton()->get_main_loop()->iteration_prepare();
Copy link
Member

@aaronfranke aaronfranke Mar 27, 2024

Choose a reason for hiding this comment

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

If the goal is to do this before physics, why isn't it done before all physics (including 3D physics?)

It would make sense to me to open this up to 3D in the future, so either this should be moved before 3D, or the comment needs to be adjusted explaining why things are the way they are.

As-is, this code also leaves me with the question: If I want to disable 2D physics, should I keep this line enabled? I don't know.

Copy link
Member

@lawnjelly lawnjelly Mar 27, 2024

Choose a reason for hiding this comment

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

@aaronfranke You are also absolutely right, I didn't spot this in the PR, the prepare should probably be before the 3D here (although it shouldn't make any difference until the 3D is done, so we can fix in 3D PR).

(This is actually something I need to double check in 3.x actually, because there was some rejigging here when I added 2D and it may have affected 3D, so thanks for pointing to this! 👍 3.x has a flush_queries() here but no sync().)

To address the confusion a little - "physics interpolation" (somewhat confusingly) is nothing to do with physics engine. The more correct name is fixed timestep interpolation.

The only relation is to the physics ticks, rather than the physics itself. It can work with or without physics engine. Juan preferred the name "physics interpolation", because even though incorrect, he thought it would be easier for users to understand (and admittedly in many cases their eyes would glaze over with "fixed timestep interpolation" 😁 ).

This is something that we as developers will be expected to learn the difference, but it will inevitably cause confusion in some way (if we e.g. called it "fixed timestep interpolation" in the c++, and "physics interpolation" for user facing code, this could be confusing, as many c++ funcs are bound etc, so it is called "physics interpolation" throughout).

As it is not related to the physics engines, it should not be disabled when disabling physics engines (as _physics_process() still occurs).

Just to try and explain what it is doing .. the VisualServer items have to be prepared before each iteration (tick) before any changes have been made to them (server side at least).

If you follow the code it calls update_interpolation_tick().

This does a bunch of things:

  • Detect any objects that are no longer being moved, and ensure their previous transform = current transform so they cease vibrating
  • Store the current transform (from the last tick) to the previous transform, so they can be interpolated after the tick

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.

Implement physics interpolation in 2D (already implemented in 3.x)