godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
90.74k stars 21.12k forks source link

`apply_impulse` uses outdated `mass` value #78866

Open Haydoggo opened 1 year ago

Haydoggo commented 1 year ago

Godot version

v4.1.rc.custom_build [16dd4e572]

System information

Godot v4.1.rc (16dd4e572) - Windows 10.0.19045 - Vulkan (Forward+) - dedicated NVIDIA GeForce GTX 1080 (NVIDIA; 31.0.15.1694) - Intel(R) Core(TM) i5-10600KF CPU @ 4.10GHz (12 Threads)

Issue description

calling apply_impulse or apply_central_impulse shortly after changing the mass of a RigidBody3D or RigidBody2D results in an impulse that does not respect the new mass.

Steps to reproduce

Change the mass of a rigidbody and apply an impulse before 2 physics frames have passed.

body.mass = 0.5
body.apply_central_impulse(Vector3.FORWARD)

A workaround is to force update the mass of the body as a side effect of calling:

PhysicsServer3D.body_set_mode(body.get_rid(), PhysicsServer3D.BODY_MODE_RIGID)

or to wait 2 physics frames with

for i in 2:
  await get_tree().physics_frame

(waiting one frame with this method doesn't seem to work, nor does a deferred call to apply_impulse)

The MRP has buttons for applying impulses and changing mass.

Minimal reproduction project

MRP.zip

quinnyo commented 1 year ago

The behaviour is as you describe it, but you've got an error/typo in your example/MRP -- note the '.bind' in body.apply_central_impulse.bind(Vector3.FORWARD), in your comment and project.

I think it's expected that there will be some delay when setting the physics properties via the node's properties. Especially so when changing the properties is done in gui/idle process. But the current behaviour feels like calling the method is working (albeit with a stale value) and setting the property is not, which is unhelpful and surprising.

Haydoggo commented 1 year ago

you've got an error/typo in your example/MRP

Oops, looks like the was leftover from some last minute testing with deferred calls, updated the MRP now!

DashiellRaccoon commented 1 year ago

I discovered that RigidBody3D.ApplyImpulse will always use the wrong mass when called in the _Ready function. I think this is caused by the same issue. In my case the mass was defined in the scene, not changed in the code. The workaround is to skip a physics frame before applying impulses.

Qoltaic commented 1 year ago

The same behavior occurs when instantiating a rigidbody3d scene with a preset mass value. If apply_impulse or apply_central_impulse are used directly it seems to use a default of 1Kg regardless of the set value, however, waiting a bit resolves it.

knil79 commented 1 year ago

I can confirm that for i in 2: await get_tree().physics_frame Is a working workaround. Mass is used properly by then.

I was starting to go crazy trying to figure out what was wrong before I found this bug!

bbbscarter commented 6 months ago

Just to add - this affects inertia as well; if you call apply_impulse just after an object has been created, no rotational velocity is applied, since inertia is 0.

Digging around, the problem appears to be that apply_impulse uses _inv_mass and _inv_inertia directly; but _inv_mass and inv_inertia are initialised in the physics tick, rather than when the object is constructed. As such any calls to apply_impulse and related methods call before the first physics tick are working with bad data.

Detail:

Workaround without modifying Godot:

Options

I've been looking around at how to fix most easily within Godot, and there seem to be a few options:

  1. When calling GodotPhysicsServer3D::body_apply_impulse(), insert a call to update_mass_properties. e.g. here: https://github.com/godotengine/godot/blob/7529c0bec597d70bc61975a82063bb5112ac8879/servers/physics_3d/godot_physics_server_3d.cpp#L704-L712. This would be the smallest change. Potential downsides:

    • From the code it's not obvious why calls to update_mass_properties are deferred in the first place - e.g. I'm assuming it's for multithreaded physics. If so, calling it out of order will be problematic.
  2. Treat apply_impulse like apply_force - accumulate the impulses, and defer multiplying them with mass/inertia to integrate_forces. This doesn't change the existing order of mass calculations, so should be safer - but, potential downsides:

    • Internal parts of the physics code rely on apply_impulse being applied immediately - e.g. godot_body_pair, joints, etc, so this would need to only apply to server API calls - e.g GodotPhysicsServer3D::body_apply_impulse.
    • Currently linear_velocity and angular_velocity are immediately updated when body_apply_impulse() is called - deferring it means they won’t be updated until the next physics tick. This might not matter, if it only affects external calls to body_apply_impulse.

Proposal

Given the multi-threading concern with option 1, I'm tempted to give option 2 a go, unless someone tells me it's a terrible idea?

Thanks!

Simon