Give each RigidBody its own DirectBodyState wrapper.

[madmiraal]

Currently 2D and 3D, Godot and Bullet physics servers have a singleton wrapper for DirectBodyState. This means that only one RigidBody's DirectBodyState can be accessed at any one time. This is an unnecessary and cumbersome restriction that is not thread safe. Aside from also being used to store the current step size, it's just a wrapper, so I can see no reason why this should be a singleton.

This PR gives each RigidBody its own DirectBodyState wrapper and moves the current step size to Space, where it belongs and already done in Bullet.

Fixes #42877.

[madmiraal]

Rebased following merge of #42167.

[AndreaCatania]

Allow the user to store this pointer is unsafe. The pointer lifetime is totally handled by the PhysicsServer (Bullet: creation , drop; PhysicsSW creation, drop). Allow the user to store this pointer will give them the power to introduce memory leaks into their game.

On the other hand, with the removal of the multi-thread sync mechanism, the existence of this Direct access class is useless, so I think that would be better remove those classes and move the APIs on the PhysicsServer.

[madmiraal]

Allow the user to store this pointer will give them the power to introduce memory leaks into their game.

The same can be said of any variable pointing to any object. However, it's just a wrapper so its memory requirements are tiny (the pointer) compared to the underlying RigidBody.

On the other hand, with the removal of the multi-thread sync mechanism, the existence of this Direct access class is useless, so I think that would be better remove those classes and move the APIs on the PhysicsServer.

This is beyond the scope of this PR, would be compat breaking, and would probably need to be discussed in a proposal first. This PR simply address issue #42877. However, bear in mind that there is still a requirement to prevent access to the underlying PhysicsServer during the physics step even if this is not needed in a single threaded environment.

[AndreaCatania]

Though, an object whom lifetime depends on the lifetime of another object, related for sure but not so much code wise.

The API was not designed to work like this; it should never be stored. Also, body_get_direct_state was meant to return nullptr when not called within a physics_frame or _integrate_forces, so cleanly it was not designed to be used in this way.

Considering that this PR is changing the API behaviour and the physics engine sync mechanism was removed without a proposal, my idea was to simplify it further so to keep it more safe (and for sure less annoying to use); but if you feel like that the time taken to change it doesn't worth the result, feel free to merge it.

[jordo]

+1 for this

[reduz]

I feel it may be a better idea to create this on demand (when requested, if null, create and return)

[madmiraal]

It is used to sync the server state with the Godot scene every iteration; so I think there is little point in delaying its creation:

godot/servers/physics_2d/body_2d_sw.cpp

Lines 586 to 592 in a5fa919

	void Body2DSW::call_queries() {
	if (fi_callback) {
	PhysicsDirectBodyState2DSW *dbs = PhysicsDirectBodyState2DSW::singleton;
	dbs->body = this;
	Variant v = dbs;
	const Variant *vp[2] = { &v, &fi_callback->callback_udata };

BTW This PR simplifies this to simply use it's own copy:

Note: fi_callback is defined during the RigidBody creation:

godot/scene/2d/physics_body_2d.cpp

Line 863 in a5fa919

PhysicsServer2D::get_singleton()->body_set_force_integration_callback(get_rid(), this, "_direct_state_changed");

[jordo]

👍

[jordo]

anecdotally, I just pulled this into our fork and it's great. To the point that we're just going to use the direct state for everything before we step our physics spaces.

[jordo]

And just a comment and maybe something to note/consider:

@reduz why is it explicitly the physics server's job to wakeup() a body after applying force, impulse, setting velocities, etc? Can't the body wake itself up within it's modifier function?

For example, this is how it's done in Box2d: b2Body flags itself awake when modifier function is called. There's no need for another object to know about this detail..

Just something to consider. When we apply_impulse or set_linear_velocity manually on the direct state, we now (as did the physics server) have to explicitly wake the body up in direct state as well... More bookeeping. IMO the physics body can compare the incoming parameter and decide if it should wake itself up or not..

[jordo]

I also think this handles multi-threaded nicely. i.e. if someone uses this incorrectly they will know.

[pouleyKetchoupp]

This change looks good to me, it's a nice fix to have and doesn't break compatibility.

I agree with what was said before about simplification: we could probably remove the direct body state completely and handle state changes in the rigid body node directly, but that would require a separate proposal/PR.

[pouleyKetchoupp]

Update from discussing this PR on IRC with @reduz:

• On 3.2: This can be merged to fix Storing two Physics2DDirectBodyState as var from Physics2DServer.body_get_direct_state in GDScript overwrites the first var #42877 without compatibility breakage
• On 4.0: DirectBodyState will be removed completely and replaced with a better way to ensure thread safety

So I guess for now #42929 could be merged?

[jordo]

@pouleyKetchoupp Pretty disappointing to hear this will not be merged in 4.0. Can the discussion be shared so others can get up to speed on the removal for the proposed better way?

[pouleyKetchoupp]

@jordo The summary of the discussion is:

-The only point of the direct access is that it protects the object at a very specific point in time which is the direct state callback and gives you access to the node internals.

-The proper fix is making the threading in the physics servers work properly, @reduz will work on rendering soon, so he can work on the physics side at the same time.

-Once this is done, DirectBodyState can be removed and be replaced with a callback (C++ Callback for C++ and a Callable if you supply one).

[madmiraal]

If it's going to be merged into 3.2 it should be merged into master first. Arguing that it will be removed later as a reason not to merge it into master, doesn't make sense, because there is no guarantee that it will be removed before 4.0 is released. Even if there was already another PR that removed it, until that PR is merged, and hence supersedes this one, there is no reason not to merge this one into master first.

[madmiraal]

Rebased following merge of #45519.

[madmiraal]

Rebased following merge of #43952 and #45852.

[reduz]

With #49471 merged, I believe there is not much of a point in directly accessing the body state as this is only a way to pass the state from physics server to the node faster. For most cases, accessing via server is enough and its safer. If something is missing there, it can be changed. As such, I will be closing this.