godotengine / godot

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

PhysicsDirectBodyState is singleton? #56245

Closed steelywing closed 2 years ago

steelywing commented 2 years ago

Godot version

3.4

System information

Windows 10

Issue description

I get 2 PhysicsDirectBodyState of 2 PhysicsBody using PhysicsServer.BodyGetDirectState(), but they became the same object.

The doc doesn't mention this thing.

Steps to reproduce

    public override void _PhysicsProcess(float delta)
    {
        var state = PhysicsServer.BodyGetDirectState(this.chest.GetRid());
        var state2 = PhysicsServer.BodyGetDirectState(this.hips.GetRid());
        GD.Print($"state {state == state2}"); // true, they are same object
    }

Minimal reproduction project

No response PhysicsDirectBodyState.zip

Calinou commented 2 years ago

@steelywing Can you reproduce this in GDScript instead of C#?

steelywing commented 2 years ago

@steelywing Can you reproduce this in GDScript instead of C#?

Yes

extends Spatial

# Declare member variables here. Examples:
# var a = 2
# var b = "text"

# Called when the node enters the scene tree for the first time.
func _ready():
    pass # Replace with function body.

# Called every frame. 'delta' is the elapsed time since the previous frame.
#func _process(delta):
#   pass
func _physics_process(delta):
    var r1 = PhysicsServer.body_get_direct_state($RigidBody.get_rid())
    var r2 = PhysicsServer.body_get_direct_state($RigidBody2.get_rid())
    r1.linear_velocity += Vector3.FORWARD
    r2.linear_velocity += Vector3.FORWARD

Only r2 move, r1 doesn't move. I have create a demo in the first post.

Zylann commented 2 years ago

That's indeed how this was designed... Still in master, for Bullet:

    static BulletPhysicsDirectBodyState3D *get_singleton(RigidBodyBullet *p_body) {
        singleton->body = p_body;
        return singleton;
    }

Means the direct state will always be the last requested one. Allowing more than one to exist requires either creating new instances of the object everytime, or have a sort of pool, or change the API to take the body as parameter of each function.

In GodotPhysics however, this seems to not be the case, as every body gets its own attached state object:

GodotPhysicsDirectBodyState3D *GodotBody3D::get_direct_state() {
    if (!direct_state) {
        direct_state = memnew(GodotPhysicsDirectBodyState3D);
        direct_state->body = this;
    }
    return direct_state;
}
steelywing commented 2 years ago

How do I work with 2 PhysicsDirectBodyState at the same time?

I cannot access BulletPhysicsDirectBodyState class in C#, so I cannot instance it. I only see abstract class PhysicsDirectBodyState.

Zylann commented 2 years ago

I know, it's an implementation detail, a bad one though because it exploits a grey area of the documented behavior and is generally an unexpected behavior. You get the abstract class in your script but it is in reality one of the two specializations depending on which physics engine you use (it's like getting an interface in C#, you do not need to know what the implementation is, usually).

If you want to use Bullet, the only workaround I see is to update the bodies one at a time, and not have both at the same time. Given your snippet, you could rewrite it like this:

    var r1 = PhysicsServer.body_get_direct_state($RigidBody.get_rid())
    r1.linear_velocity += Vector3.FORWARD
    # Done with r1

    var r2 = PhysicsServer.body_get_direct_state($RigidBody2.get_rid())
    r2.linear_velocity += Vector3.FORWARD
    # done with r2

In more complex scenarios it might not be pretty but it's workable, you can get the state of any body but you can only hold one at a time.

Otherwise this needs to be fixed because it is a nasty API hack... or you have to use GodotPhysics.

madmiraal commented 2 years ago

Duplicate of #42877. This is fixed in 3.x (i.e. in 3.5) with #42929. There was a master branch version (#42928) which included the fix for Bullet too, but it was closed because #49471 was merged instead.