godot-rust / gdext

Rust bindings for Godot 4
https://mastodon.gamedev.place/@GodotRust
Mozilla Public License 2.0
3.09k stars 194 forks source link

Integer is zero when passing it to function that takes f32/f64 #105

Open thebigG opened 1 year ago

thebigG commented 1 year ago

Hi there,

I defined the following function in my gdextension:

    #[func]
    fn add_two(&mut self, a: f32) -> f32{
        println!("a:{}",a);
        return a + 2.0;
    }

The following call has no problems from gdscript

    print(add_two(1.0))

This will print a:1, which is expected.

However if I call this

print(add_two(1))

This prints a:0, which is not what I expected.

Is this behavior expected, or is there something else going here I'm missing?

My mind went directly to ABI--I thought for some reason something the rust compiler is doing...?

In any case I looked at the dwarf and it looks like the type abi from my rust gdextension matches that of Godot functions "real_t".

Here is real_t dwarf info from godot compiled from source:


0x00003839:   DW_TAG_base_type
                DW_AT_byte_size (0x04)
                DW_AT_encoding  (DW_ATE_float)
                DW_AT_name  ("float")

Here is the one for my rust code


0x00024b82:   DW_TAG_base_type
                DW_AT_name  ("f32")
                DW_AT_encoding  (DW_ATE_float)
                DW_AT_byte_size (0x04)

I hope I explained this clearly enough. Am I missing something here?

Thanks in advance!

thebigG commented 1 year ago

Here is some more info about my system/environment:

VERSION="22.04 LTS"
ID=pop
ID_LIKE="ubuntu debian"
PRETTY_NAME="Pop!_OS 22.04 LTS"
VERSION_ID="22.04"
HOME_URL="https://pop.system76.com"
SUPPORT_URL="https://support.system76.com"
BUG_REPORT_URL="https://github.com/pop-os/pop/issues"
PRIVACY_POLICY_URL="https://system76.com/privacy"
VERSION_CODENAME=jammy
UBUNTU_CODENAME=jammy
LOGO=distributor-logo-pop-os

Godot_v4.0-beta15_linux.x86_64 rustc 1.65.0 (897e37553 2022-11-02)

Bromeon commented 1 year ago

My mind went directly to ABI--I thought for some reason something the rust compiler is doing...?

I think conversions from GDScript int to Rust f32 are not yet implemented. The initial implementation was quite strict, types needed to match exactly.

In ergonomics sake, we can maybe define some rules regarding which conversions are safe. Unfortunately implicit conversions introduce a lot of potential for error, so I'm not sure if we should support it at all.

Anyway, thanks for reporting!

lilizoey commented 1 year ago

So i think this issue is at least partially fixed. i.e when done as a ptrcall it will succeed as expected, however when done as a varcall this will report a spurious error that the method does not exist for the second call.

this is caused (i believe) by us panicking when we try to convert a variant containing 1 into a float, which triggers the above message by #254. however with a pointer call we just use as to convert.

Bromeon commented 11 months ago

@thebigG do you know if this is still a problem with latest master and latest Godot (4.2-rc1)?

lilizoey commented 7 months ago

Current status of this, running this code:

var bar: Bar = $Bar
bar.add_two(1.0)
bar.add_two(1)
$Bar.add_two(1.0)
$Bar.add_two(1)

will print a: 1 for the first three, but then error on the fourth one with a type conversion error.

for consistency we should probably try to make it so that 2 and 4 have the same behavior. either both have a type conversion error or they both succeed with type coercion.

Bromeon commented 6 months ago
var bar: Bar = $Bar   # Rust receives:
bar.add_two(1.0)      # 1) 1.0
bar.add_two(1)        # 2) 1.0
$Bar.add_two(1.0)     # 3) 1.0
$Bar.add_two(1)       # 4) ERROR

I looked further into this.

We can't change the first two, because they use ptrcalls and Rust only receives the already correctly typed value. The engine is responsible for providing the argument, and performs the 1 -> 1.0 conversion in GDScript.

3) and 4) are varcalls, and 4) currently fails due to variant conversions being forbidden between int and float. We could relax that, however:

  1. This would weaken general type safety around Variant and lead to some other unexpected cases, such as:
    let i = Variant::from(1);
    i.try_to::<f32>()        // now Some(1.0), even though:
    i.get_type()             // Int
    i == Variant::from(1.0)  // false
  2. It would also affect Rust-based reflection calls:
    bar.call("add_two".into(), &[Variant::from(1)]); // now succeeds, too

Another option would be to have a separate set of conversions, like gdnative's CoerceFromVariant. The question is then still whether #[func] should use those by default, instead of the current Variant conversions? I was thinking about a #[func(strict)] for the current behavior, but if 2) is still silently converted, that doesn't have much point...

Apart from that, GDScript provides a strict typing mode, but I'm not sure if this just requires type annotations or also prevents implicit conversions. But I don't think beyond that we can influence the behavior of 1) and 2).

Bromeon commented 2 months ago

See also: https://github.com/godot-rust/gdext/issues/845#issuecomment-2284291896

So, Godot has two functions that check conversions between different Variant types:

Both are directly available in the GDExtension C API.


Our current Rust implementation for Variant conversions is actually strict: it's disallowed if the variant type is different. We can probably add functionality like gdnative's CoerceFromVariant to allow the "Godot strict" version.

To have varcalls behave exactly like ptrcalls from GDScript perspective, I see no alternative than downgrading to "Godot strict" semantics. While it allows more harmless things like 1 -> 1.0, you can also hide very costly conversions (packed array <-> array) as well as nonsensical ones (bool -> float) that are 99% of the time straight user errors. Then again, this seems* to already happen for ptrcalls, which are used whenever someone uses typing in GDScript.

In other words, this has the ironic effect that not using types in GDScript is actually more type-safe 😬


* It needs to be confirmed whether GDScript-side implicit conversions before ptrcalls indeed use the same semantics as Variant::can_convert_strict.