Virtual methods must take `Option<Gd<T>>` instead of `Gd<T>`
andreymal opened this issue · 23 comments
I'm trying to make an editor plugin:
use godot::engine::IEditorPlugin;
use godot::prelude::*;
#[derive(GodotClass)]
#[class(tool, editor_plugin, init, base=EditorPlugin)]
pub struct MyEditorPlugin {}
#[godot_api]
impl IEditorPlugin for MyEditorPlugin {
fn handles(&self, _: Gd<Object>) -> bool {
true
}
fn edit(&mut self, object: Gd<Object>) {}
}For some reason object's type is non-optional Gd<Object>, but the documentation says:
objectcan benullif the plugin was editing an object, but there is no longer any selected object handled by this plugin. It can be used to cleanup editing state.
As a result, when no node is selected, Godot tries to call ._edit(null) and crashes.
Backtrace if needed
thread '<unnamed>' panicked at /home/andreymal/.cargo/git/checkouts/gdext-76630c89719e160c/13ab375/godot-core/src/builtin/meta/signature.rs:450:5:
edit: parameter [0] has type godot_core::obj::gd::Gd<godot_core::gen::classes::object::re_export::Object>, which is unable to store argument "TODO"
stack backtrace:
0: 0x7fb90fde422c - std::backtrace_rs::backtrace::libunwind::trace::h67a838aed1f4d6ec
at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/std/src/../../backtrace/src/backtrace/libunwind.rs:93:5
1: 0x7fb90fde422c - std::backtrace_rs::backtrace::trace_unsynchronized::h1d1786bb1962baf8
at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/std/src/../../backtrace/src/backtrace/mod.rs:66:5
2: 0x7fb90fde422c - std::sys_common::backtrace::_print_fmt::h5a0b1f807a002d23
at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/std/src/sys_common/backtrace.rs:67:5
3: 0x7fb90fde422c - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::hf84ab6ad0b91784c
at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/std/src/sys_common/backtrace.rs:44:22
4: 0x7fb90fe07fec - core::fmt::rt::Argument::fmt::h28f463bd1fdabed5
at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/core/src/fmt/rt.rs:138:9
5: 0x7fb90fe07fec - core::fmt::write::ha37c23b175e921b3
at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/core/src/fmt/mod.rs:1114:21
6: 0x7fb90fde1ebe - std::io::Write::write_fmt::haa1b000741bcbbe1
at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/std/src/io/mod.rs:1763:15
7: 0x7fb90fde4014 - std::sys_common::backtrace::_print::h1ff1030b04dfb157
at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/std/src/sys_common/backtrace.rs:47:5
8: 0x7fb90fde4014 - std::sys_common::backtrace::print::hb982056c6f29541c
at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/std/src/sys_common/backtrace.rs:34:9
9: 0x7fb90fde5783 - std::panicking::default_hook::{{closure}}::h11f92f82c62fbd68
at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/std/src/panicking.rs:272:22
10: 0x7fb90fde54a4 - std::panicking::default_hook::hb8810fe276772c66
at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/std/src/panicking.rs:292:9
11: 0x7fb90fde5e81 - <alloc::boxed::Box<F,A> as core::ops::function::Fn<Args>>::call::h87b887549356728a
at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/alloc/src/boxed.rs:2021:9
12: 0x7fb90fde5e81 - std::panicking::rust_panic_with_hook::hd2f0efd2fec86cb0
at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/std/src/panicking.rs:735:13
13: 0x7fb90fde5c01 - std::panicking::begin_panic_handler::{{closure}}::h3651b7fc4f61d784
at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/std/src/panicking.rs:609:13
14: 0x7fb90fde4756 - std::sys_common::backtrace::__rust_end_short_backtrace::hbc468e4b98c7ae04
at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/std/src/sys_common/backtrace.rs:170:18
15: 0x7fb90fde5952 - rust_begin_unwind
at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/std/src/panicking.rs:597:5
16: 0x7fb90fc0ed25 - core::panicking::panic_fmt::h979245e2fdb2fabd
at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/core/src/panicking.rs:72:14
17: 0x7fb90fc370e0 - godot_core::builtin::meta::signature::param_error::h9e5e0410d0fe12c0
at /home/andreymal/.cargo/git/checkouts/gdext-76630c89719e160c/13ab375/godot-core/src/builtin/meta/signature.rs:450:5
18: 0x7fb90fc37b46 - godot_core::builtin::meta::signature::ptrcall_arg::{{closure}}::h3a978076abd52718
at /home/andreymal/.cargo/git/checkouts/gdext-76630c89719e160c/13ab375/godot-core/src/builtin/meta/signature.rs:430:41
19: 0x7fb90fc268dc - core::option::Option<T>::unwrap_or_else::h235c3edeffbe6d51
at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/core/src/option.rs:979:21
20: 0x7fb90fc37afc - godot_core::builtin::meta::signature::ptrcall_arg::hda9ba436188e0501
at /home/andreymal/.cargo/git/checkouts/gdext-76630c89719e160c/13ab375/godot-core/src/builtin/meta/signature.rs:430:5
21: 0x7fb90fc3d3fd - <(R,P0) as godot_core::builtin::meta::signature::PtrcallSignatureTuple>::in_ptrcall::hd190a75f79badd54
at /home/andreymal/.cargo/git/checkouts/gdext-76630c89719e160c/13ab375/godot-core/src/builtin/meta/signature.rs:272:30
22: 0x7fb90fc3612d - <my_project::MyEditorPlugin as godot_core::obj::traits::cap::ImplementsGodotVirtual>::__virtual_call::function::hbdcacccc161356a7
at /home/andreymal/projects/my_project/src/lib.rs:67:1
23: 0x55e9ee942dec - _ZN12EditorPlugin21_gdvirtual__edit_callILb0EEEbP6Object
at /usr/src/debug/godot/godot-4.1.3-stable/editor/editor_plugin.h:86:2
24: 0x55e9ee942dec - _ZN12EditorPlugin4editEP6Object
at /usr/src/debug/godot/godot-4.1.3-stable/editor/editor_plugin.cpp:320:2
25: 0x55e9ee8e6ac0 - _ZN10EditorNode19hide_unused_editorsEPK6Object
at /usr/src/debug/godot/godot-4.1.3-stable/editor/editor_node.cpp:2237:21
26: 0x55e9eec441b8 - _ZN13SceneTreeDock18_selection_changedEv
at /usr/src/debug/godot/godot-4.1.3-stable/editor/scene_tree_dock.cpp:2271:13
27: 0x55e9f1d4f25b - _ZN6Object12emit_signalpERK10StringNamePPK7Varianti
at /usr/src/debug/godot/godot-4.1.3-stable/core/object/object.cpp:1070:20
28: 0x55e9ef67df96 - _ZN6Object11emit_signalIJEEE5ErrorRK10StringNameDpT_
at /usr/src/debug/godot/godot-4.1.3-stable/./core/object/object.h:884:22
29: 0x55e9ef67df96 - _ZN15EditorSelection12_emit_changeEv
at /usr/src/debug/godot/godot-4.1.3-stable/editor/editor_data.cpp:1292:13
30: 0x55e9ed92430d - _Z29call_with_variant_args_helperI17__UnexistingClassJEJEEvPT_MS1_FvDpT0_EPPK7VariantRN8Callable9CallErrorE13IndexSequenceIJXspT1_EEE
at /usr/src/debug/godot/godot-4.1.3-stable/./core/variant/binder_common.h:303:25
31: 0x55e9ed92430d - _Z25call_with_variant_args_dvI17__UnexistingClassJEEvPT_MS1_FvDpT0_EPPK7VariantiRN8Callable9CallErrorERK6VectorIS7_E
at /usr/src/debug/godot/godot-4.1.3-stable/./core/variant/binder_common.h:450:31
32: 0x55e9ed92430d - _ZNK11MethodBindTIJEE4callEP6ObjectPPK7VariantiRN8Callable9CallErrorE
at /usr/src/debug/godot/godot-4.1.3-stable/./core/object/method_bind.h:331:28
33: 0x55e9f1d4527b - _ZN6Object5callpERK10StringNamePPK7VariantiRN8Callable9CallErrorE
at /usr/src/debug/godot/godot-4.1.3-stable/core/object/object.cpp:739:21
34: 0x55e9f1a5db9f - _ZNK8Callable5callpEPPK7VariantiRS0_RNS_9CallErrorE
at /usr/src/debug/godot/godot-4.1.3-stable/core/variant/callable.cpp:62:30
35: 0x55e9f1d3b930 - _ZN9CallQueue14_call_functionERK8CallablePK7Variantib
at /usr/src/debug/godot/godot-4.1.3-stable/core/object/message_queue.cpp:219:18
36: 0x55e9f1d3f0f0 - _ZN9CallQueue5flushEv
at /usr/src/debug/godot/godot-4.1.3-stable/core/object/message_queue.cpp:320:20
37: 0x55e9efb184fa - _ZN9SceneTree15physics_processEd
at /usr/src/debug/godot/godot-4.1.3-stable/scene/main/scene_tree.cpp:471:38
38: 0x55e9ed7f6c08 - _ZN4Main9iterationEv
at /usr/src/debug/godot/godot-4.1.3-stable/main/main.cpp:3390:60
39: 0x55e9ed780340 - _ZN11OS_LinuxBSD3runEv
at /usr/src/debug/godot/godot-4.1.3-stable/platform/linuxbsd/os_linuxbsd.cpp:912:22
40: 0x55e9ed76fc98 - main
at /usr/src/debug/godot/godot-4.1.3-stable/platform/linuxbsd/godot_linuxbsd.cpp:74:9
41: 0x7fb915358cd0 - <unknown>
42: 0x7fb915358d8a - __libc_start_main
43: 0x55e9ed77d255 - _start
44: 0x0 - <unknown>
fatal runtime error: failed to initiate panic, error 5
I don't know if this is a Godot bug or godot-rust bug, but since I ran into this issue during playing with Rust, I'm leaving this issue here.
Yep in fact it's already tracked by #156. The current workaround is also explained there 🙂
@Bromeon that workaround is not applicable here because I can't control how Godot calls functions.
This means it's currently not possible to make an editor plugin in Rust :(
Hm good point. Maybe we should track it separately, in case the two things are not fixed at the same time.
I'll edit the description of the other issue to highlight this.
An issue is that the GDExtension API does not expose the information whether a parameter or return type can be null or not. That means we either have to conservatively assume everything can be Option<Gd<T>>, which turns the entire API into a billion-dollar mistake; or we manually go through the process of maintaining this information somewhere, which is a huge effort.
I opened a proposal ages ago, maybe you can upvote it. It is a bit more ambitious, but nullability might already be a good first step:
godotengine/godot-proposals#2550
@Bromeon I'm not that knowledgeable about inner workings of Gd, what is the implication of using Option<Gd> instead of Gd for selected builtin method signatures? By no means I'm having in mind compromising null safety by cluttering gdext API with Option<Gd>, but maybe procuring some kind of special cases until upstream solves the issue could be a viable option? Seeing as your issue was posted on godot proposals in 2021 I'm worried it may not be adressed soon, and it is possible that it will gatekeep many very useful features, like the whole IEditorPlugin impl in this case. Which I think is really one of special cases, as it is called within the Godot Editor and its the whole point of that class.
I'm asking about the implications to assess the danger of reverse occuring: I mean Godot's non-nullable marked as nullable on gdext side in case of upstream changes.
@StatisMike yes, that's exactly the plan.
I'm working on a proposal that would combine this with an other feature, impl-conversions. At the moment, a lot of arguments need to be passed as "hello".into(), which could be avoided by something like impl Into<StringName>. The idea here would be similar, that a user could pass &Gd, Gd or Option<Gd> for ergonomics. There is some prior art in gdnative::object::AsArg.
Seeing as your issue was posted on godot proposals in 2021 I'm worried it may not be adressed soon
No worries, I (among others) brought it up with the GDExtension team again. We would all benefit from a real solution on Godot side here, and I agree we shouldn't wait for it before we take action on Rust side.
Coincidentally, in the same minute you posted your reply, Godot devs opened godotengine/godot#86079 😀
Happy to see this is being worked on (by the Godot folks). I just ran into the same problem: Many of the ScriptLanguageExtension API methods take an Object*, and they vary on whether or not the argument can be null. This really does need to be documented somewhere upstream that we can consume.