gdext icon indicating copy to clipboard operation
gdext copied to clipboard

Engine APIs with nullable parameters should accept `Option<Gd<T>>`, but currently require `Gd<T>`

Open atrefonas opened this issue 1 year ago • 5 comments

[Edit bromeon]

If you come here because of an error message in gdext, that error message is correct. You likely encounter an API which is supposed to accept null arguments, but has a Gd<T> parameter declared -- so it's not possible to pass null.

Possible workaround: use Object::call() with Variant::nil().

Issue https://github.com/godot-rust/gdext/issues/494 tracks a similar problem, but applied to virtual functions (the I* traits) instead of godot::engine API. That one cannot be worked around currently.

Original title of this issue was:

Codegen issue: the collision argument of PhysicsBody3D.test_move() should be Option<Gd<KinematicBody3D>>


Original message

In the GDScript docs for PhysicsBody3D, there is:

bool test_move ( Transform3D from, Vector3 motion, KinematicCollision3D collision=null,
  float safe_margin=0.001, bool recovery_as_collision=false, int max_collisions=1 )

As you can see, the 3rd argument has a default null value. Rust does not allow nulls, and uses Option instead.

However, in the generated Rust function, it (IMO incorrectly) requires a Gd value that needs to be non-null.

pub fn test_move(
    &mut self,
    from: Transform3D,
    motion: Vector3,
    collision: Gd<KinematicCollision3D>,
    safe_margin: f64,
    recovery_as_collision: bool,
    max_collisions: i64
) -> bool

The collision argument should be an Option so that it can take a None value. Otherwise, there is no known way to use the default arguments. This issue is related to https://github.com/godot-rust/gdextension/issues/155

It's also likely that similar codegen issues exist in many other APIs.

atrefonas avatar Mar 07 '23 20:03 atrefonas

Good catch! Yes, the object parameters should possibly be generic -- conceptually something like impl Into<Gd<T>>, but could also be a dedicated trait. The GDNative binding did quite some prior art here.

Bromeon avatar Mar 07 '23 21:03 Bromeon

Is Gd<T> always intended to be non-null? Cause in that case, doing literally impl Into<Gd<T>> would not work here. Maybe a impl Into<NullableGd<T>> could work or just an impl Into<Option<Gd<T>>>

lilizoey avatar Mar 07 '23 23:03 lilizoey

Is Gd<T> always intended to be non-null?

Yes. You're right, impl Into<Option<Gd<T>>> would fit better, but likely it would be a dedicated trait anyway.

Bromeon avatar Mar 07 '23 23:03 Bromeon

I currently ran into a panic related to this issue when using surface tool. It looks like some work has been done to add extended parameters through the ExCommit type, but it currently throws an exception. I'm assuming it's just because it's a WIP?

EDIT: as per the suggestion below, this workaround does the trick

let mut mesh: Gd<ArrayMesh> = st.call("commit".into(), &[]).to();

jrenner avatar Nov 23 '23 01:11 jrenner

I currently ran into a panic related to this issue when using surface tool. It looks like some work has been done to add extended parameters through the ExCommit type, but it currently throws an exception. I'm assuming it's just because it's a WIP?

currently these just don't work properly if the default argument is null. but you can use the obj.call("method",..) as a workaround. but yeah it's mostly just that we havent implemented the necessary codegen here.

lilizoey avatar Nov 23 '23 01:11 lilizoey