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

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

Open
atrefonas opened this issue Mar 7, 2023 · 5 comments
Labels
bug c: engine Godot classes (nodes, resources, ...)

Comments

@atrefonas
Copy link

atrefonas commented Mar 7, 2023

[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 #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 #155

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

@Bromeon Bromeon added bug c: engine Godot classes (nodes, resources, ...) labels Mar 7, 2023
@Bromeon
Copy link
Member

Bromeon commented Mar 7, 2023

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.

@lilizoey
Copy link
Member

lilizoey commented Mar 7, 2023

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>>>

@Bromeon
Copy link
Member

Bromeon commented Mar 7, 2023

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 Bromeon changed the title Codegen issue: the collision argument of PhysicsBody3D.test_move() should be Option<Gd<KinematicBody3D>> Engine APIs with nullable parameters should accept Option<Gd<T>>, but currently require Gd<T> Aug 27, 2023
@jrenner
Copy link

jrenner commented Nov 23, 2023

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();

@lilizoey
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug c: engine Godot classes (nodes, resources, ...)
Projects
None yet
Development

No branches or pull requests

4 participants