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

Panic on accessing Gd<Self> in contructors #997

Open
malj opened this issue Jan 4, 2025 · 7 comments
Open

Panic on accessing Gd<Self> in contructors #997

malj opened this issue Jan 4, 2025 · 7 comments
Labels
bug c: register Register classes, functions and other symbols to GDScript

Comments

@malj
Copy link

malj commented Jan 4, 2025

Accessing Gd<Self> pointer in constructors silently crashes the editor.

I haven't dug too deep into the library implementation so I'm not sure if this is a bug or a constructor API design limitation, but considering that WithBaseField::to_gd is part of the public API and the equivalent C++ and GDScript code works fine I thought I'd mention it.

Minimal example:

#[derive(GodotClass)]
#[class(base=Object)]
struct MyObject {
    base: Base<Object>,
}

#[godot_api]
impl IObject for MyObject {
    fn init(base: Base<Object>) -> Self {
        let instance = Self { base };
        instance.to_gd(); // <-- crash (downcast panic)
        instance
    }
}

The intended use case was to connect some local signals in the constructor while building a custom editor plugin, which seems to be a common pattern in engine code. This requires Gd<Self> for constructing a callable via Callable::from_object_method(&instance.to_gd(), "method_name").

EDIT: Removed "silent" since it was an unrelated issue

@malj malj changed the title Crash on accessing Gd<Self> in contructors Crash on accessing Gd<Self> in contructors Jan 4, 2025
@Houtamelo
Copy link
Contributor

WithBaseField::to_gd has the attribute #[doc(hidden)], it's not meant to be part of the public API, IIRC it's public because some macro-generated code needs to use it.

The crash isn't truly silent, if you run Godot from the command line you'll be able to see the panic output.

@malj
Copy link
Author

malj commented Jan 4, 2025

  1. It doesn't
  2. Running Godot CLI on Windows doesn't give me any output at all, not even in verbose mode.

@Houtamelo
Copy link
Contributor

  1. It doesn't

My apologies, I mistook it with Base::to_gd:

pub fn to_gd(&self) -> Gd<T> {

2. Running Godot CLI on Windows doesn't give me any output at all, not even in verbose mode.

Does the terminal window stay open after Godot crashes?

@malj
Copy link
Author

malj commented Jan 4, 2025

Np, I also thought that it might be Base::to_gd related because of #557.

Re terminal yes, the exit code is 127

@malj
Copy link
Author

malj commented Jan 4, 2025

Actually the silent part seems to be an unrelated Git Bash issue, running Godot CLI via PowerShell outputs a panic message:

C:\path\to\godot-core-0.2.2:
downcast from Object to MyObject failed; instance Gd { id: 1109376686628, class: Object }

@Bromeon
Copy link
Member

Bromeon commented Jan 4, 2025

It's closely related to #557 though, as the use case is the same: running certain initialization logic in the init constructor. It's just that some people try to achieve this by accessing hidden Base methods, and others use the constructed (but not yet wired-up) Self instance.

Generally it might only be safe to hand out Gd<Self::Base> pointers at this point, because the extension instance (of type Self) isn't fully constructed yet. I'm not sure if we can support this in the type system alone -- at least not without introducing quite a bit of complexity -- so it might be better to have specific APIs for this and document it well?

Small note: please avoid using the term "crash" if there's a panic. The latter is controlled, even if not always desirable 😉

@Bromeon Bromeon added bug c: register Register classes, functions and other symbols to GDScript labels Jan 4, 2025
@Bromeon Bromeon changed the title Crash on accessing Gd<Self> in contructors Panic on accessing Gd<Self> in contructors Jan 4, 2025
@malj
Copy link
Author

malj commented Jan 4, 2025

Unlike Base::to_gd this panic is achievable using the public API, and the example which triggers it is actually mentioned as a valid workaround in #557 description. Granted, both issues might seem like edge cases because most initialization logic can be moved to enter_tree or ready in case of nodes, but it's not so clear cut for resources.

I'll continue with any general constructor API discussions in #557 to have everything in one place.

Re panic note, agreed, my apologies. When I opened the issue I didn't have the panic output yet due to a terminal issue, so it seemed like an unexplained crash.

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