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

Crash when assigning a Rust instance to a variable with a type #905

Open
kylewlacy opened this issue Jul 16, 2022 · 5 comments
Open

Crash when assigning a Rust instance to a variable with a type #905

kylewlacy opened this issue Jul 16, 2022 · 5 comments
Labels
bug documentation good first issue status: upstream Issue that originates in an upstream project and can't be dealt with here.

Comments

@kylewlacy
Copy link

kylewlacy commented Jul 16, 2022

Rust version: rustc 1.62.0 (a8314ef7d 2022-06-27)
Godot version: Godot Engine v3.4.4.stable.official.419e713a2
godot-rust version: 0.10.0
Platform: Windows 11 (x86_64-pc-windows-msvc)

I ran into what looks like a segfault when interacting with a GDNative class created in godot-rust from a GDScript class. I did my best to narrow down the problem as much as possible, but it seems like it's still a pretty niche set of conditions. Here's the Rust source:

use gdnative::prelude::*;

godot_init!(init);

fn init(handle: InitHandle) {
    handle.add_class::<HelloWorld>();
}

#[derive(NativeClass)]
#[inherit(Node)]
pub struct HelloWorld;

impl HelloWorld {
    fn new(_owner: &Node) -> Self {
        godot_print!("[HelloWorld] new");
        HelloWorld
    }
}

#[methods]
impl HelloWorld {
    #[export]
    fn make_another(&self, _owner: &Node) -> Instance<Self, Unique> {
        let result = HelloWorld.emplace();
        godot_print!("[HelloWorld] make_another");
        result
    }
}

Here's the GDScript source:

extends Node

const HelloWorld = preload("res://HelloWorld.gdns")

var hello: HelloWorld

func _ready():
	print("[Example] _ready: Start")
	var new_hello_world: HelloWorld = HelloWorld.new()
	print("[Example] _ready: Created new_hello_world")
	var hello: HelloWorld = new_hello_world.make_another() # <-- Crash here
	print("[Example] _ready: Called make_another")

Here's a full example project with this setup: GodotRustEmplacePoc.zip

When I attach the above GDScript file to a node, the project crashes when assigning to the hello member variable. No error is included in the output, the process just exits with a bad exit code. I'm able to reproduce this consistently when trying to assign the result of a godot-rust method that returns Instance<_, _> to a GDScript variable with a type annotation that matches that instance type. Either using a supertype like Node or removing the type annotation from the hello variable seems to fix the crash.

Is this a bug, or is this violating some constraint I'm not aware of with Instance<_, _> or .emplace()? If this is a bug, is it a bug in godot-rust, or is it an upstream bug in Godot?

@kylewlacy kylewlacy added the bug label Jul 16, 2022
@Bromeon
Copy link
Member

Bromeon commented Jul 16, 2022

Hello!

GDNative types have unfortunately had a 2nd-class-citizen status in GDScript for quite a while. Last time I checked, these things were not fully supported:

I searched extensively for an official issue regarding the type-hint situation, but didn't find one. I'm actually surprised that "most" of your code in the project works at all; I had the situation in the past where GDNative parameter types in GDScript would just be passed as null. Possibly some of the issues were addressed in the meantime, but not fully.

Thanks for providing the example project -- I could reproduce the crash.
I also modified the GDScript code a bit and noticed something interesting:

extends Node

const HelloWorld = preload("res://HelloWorld.gdns")

func _ready():
	print("[Example] _ready: Start")

	# Context information: 'HelloWorld' is mostly an instance of NativeScript
	print("HelloWorld: ", HelloWorld)
	print("typeof:     ", typeof(HelloWorld))
	print("get_class:  ", HelloWorld.get_class())
	print()

	var new_hello_world = HelloWorld.new()
	print("[Example] _ready: Created new_hello_world: ", new_hello_world)

	var hello = new_hello_world.make_another()
	print("[Example] _ready: Called make_another: ", hello)

	var hello2: HelloWorld = hello
	print("[Example] _ready: Assigned GDScript var: ", hello2)

	# Exit afterwards; should return exit code 0 if no crash
	get_tree().quit()

Invocation from command line (Windows 10, Cmder terminal):

λ godot
Godot Engine v3.4.4.stable.official.419e713a2 - https://godotengine.org
OpenGL ES 3.0 Renderer: NVIDIA GeForce GTX 1650 with Max-Q Design/PCIe/SSE2
OpenGL ES Batching: ON

[Example] _ready: Start
HelloWorld: [NativeScript:1233]
typeof:     17
get_class:  NativeScript

[HelloWorld] new
[Example] _ready: Created new_hello_world: [Node:1246]
[HelloWorld] make_another
[Example] _ready: Called make_another: [Node:1248]

λ echo %errorlevel%
-1073741819

As you see, the Assigned GDScript var statement is not printed, and the exit code is something undefined, i.e. the engine crashed. Since the crash occurs in a pure GDScript line (assigning one var to another), it seems likely that the problem is not directly related to godot-rust; especially because the hello variable itself was printed successfully before.

I think for now, the best workaround is to use the "most-derived Godot class" as a type, i.e. Node in your case. If you find something regarding the official type-hint status in Godot, let me know!

By the way, these things should all be fixed in Godot 4, since GDExtension is a much more robust and better-integrated way of registering custom classes. You will also not need .gdns files anymore.

@Bromeon Bromeon added this to the v0.10.1 milestone Jul 16, 2022
@Bromeon
Copy link
Member

Bromeon commented Aug 25, 2022

Given the above, I'm not sure if godot-rust can do anything to fix this. Does someone know if godot-cpp has the same issue?

If there are no more ideas on the topic, I am probably going to close the issue.

@Bromeon Bromeon removed this from the v0.10.1 milestone Aug 25, 2022
@chitoyuu chitoyuu added documentation good first issue status: upstream Issue that originates in an upstream project and can't be dealt with here. labels Dec 6, 2022
@chitoyuu
Copy link
Contributor

chitoyuu commented Dec 6, 2022

Given the discussion above, this is probably better reconsidered as a documentation issue. We could add a "known limitations" section to the FAQ, where we collect information about things outside our control like this, or the "abstract class" problem.

@Bromeon
Copy link
Member

Bromeon commented Dec 6, 2022

Great idea! I created the issue https://github.com/godot-rust/book/issues/86, which also links to the "abstract class" issue.
Feel free to add more suggestions there.

Do we also want to mention this in the API docs, maybe under #[derive(NativeClass)]? We could link to the book page.
If not, there is nothing left to do in this repo and we can close this issue.

@chitoyuu
Copy link
Contributor

chitoyuu commented Dec 6, 2022

Yeah, a mention in the API docs should be great too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug documentation good first issue status: upstream Issue that originates in an upstream project and can't be dealt with here.
Projects
None yet
Development

No branches or pull requests

3 participants