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

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

Open
thebigG opened this issue Feb 2, 2023 · 7 comments
Open

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

thebigG opened this issue Feb 2, 2023 · 7 comments
Labels
bug c: register Register classes, functions and other symbols to GDScript

Comments

@thebigG
Copy link
Contributor

thebigG commented Feb 2, 2023

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
Copy link
Contributor Author

thebigG commented Feb 2, 2023

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
Copy link
Member

Bromeon commented Feb 2, 2023

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!

@Bromeon Bromeon added bug c: register Register classes, functions and other symbols to GDScript labels Feb 2, 2023
@lilizoey
Copy link
Member

lilizoey commented May 1, 2023

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
Copy link
Member

Bromeon commented Nov 30, 2023

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

@lilizoey
Copy link
Member

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
Copy link
Member

Bromeon commented May 2, 2024

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
Copy link
Member

Bromeon commented Aug 12, 2024

See also: #845 (comment)

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

  • Variant::can_convert_strict
    • "Strict" is misleading here, it allows interesting conversions like BOOL -> FLOAT, BASIS -> QUATERNION, etc.
    • godot-cpp uses this for checking argument types on varcalls, as well as Variant conversions in general.
    • It might be used for implicit GDScript-side conversions before passing them to ptrcalls*
  • Variant::can_convert
    • Even less strict.
    • This additionally allows numeric <-> string conversions.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug c: register Register classes, functions and other symbols to GDScript
Projects
None yet
Development

No branches or pull requests

3 participants