lilizoey
commented
10 months ago seems related to #156
Closed andreymal closed 1 month ago
lilizoey
commented
10 months ago seems related to #156
Bromeon
commented
10 months ago Yep in fact it's already tracked by #156. The current workaround is also explained there 🙂
andreymal
commented
10 months ago @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 :(
Bromeon
commented
10 months ago 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.
Bromeon
commented
10 months ago 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: https://github.com/godotengine/godot-proposals/issues/2550
StatisMike
commented
10 months ago @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.
Bromeon
commented
10 months ago @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 https://github.com/godotengine/godot/pull/86079 😀
Mercerenies
commented
9 months ago 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.
StatisMike
commented
2 months ago @Bromeon I've got the same message for EditorInspectorPlugin::parse_property() - the Gd<Object> can be null, so the signature there is also not correct. I don't think it warrants a new Issue to be opened, just leaving breadcrumb there.
If I find any more of these, I can edit this message to keep track of them but not bloat this comment thread.
Bromeon
commented
2 months ago Yes, I think we have to conservatively change all object parameters in virtual functions to Option<Gd<T>>. It's annoying, but without nullability information our hands are tied.
StatisMike
commented
2 months ago @Bromeon For now it seems to be a few functions affected - I think making every Gd<T> an Option in virtuals can be a little too much?
Maybe a special cases could be made there? Or just only Gd<Object> signatures (as above only these were reported)?
Mercerenies
commented
2 months ago @StatisMike This issue is blocking any attempt to use those functions right now. I'd rather have several functions that always return a Some than a bunch of functions that are literally unusable due to low-level segfaults. The former is solved by a downstream unwrap; the latter can't be solved by downstream libraries and apps.
Bromeon
commented
2 months ago @StatisMike But who maintains the list of functions that are nullable?
I have basically two options:
Switch between Option<Gd<T>> and Gd<T>. Whether it's black- or whitelist based, the effort needs to be done manually. If we forget a nullable parameter, this becomes a bug that now requires a breaking change in the API. And it's actually worse, it's perfectly within Godot's right to never return null until they do, and we have no way of learning about this.
Conservatively stick to Option<Gd<T>>. It's annoying to unwrap, I agree -- but like @Mercerenies says, this can simply be adjusted by the user side, and there's no scenario where we prevent the user from using virtual functions (like today).
I don't like pushing the responsibility to the user, but I don't think we have much choice?
We can try to experiment with a best-effort approach (whitelist based: everything is null unless stated otherwise), but I'd need to crowdsource this effort, as I'm not familiar with all the Godot APIs. Do you have a starting point here?
StatisMike
commented
2 months ago @Bromeon I think we can begin with the ones mentioned in this thread. I will try to spot some differences in godot source code with EditorPlugin and EditorInspectorPlugin implementations -maybe it will be discernable that these methods can receive null.
Most if not all of these virtual methods already have some implementation in Godot's engine. I'm not cpp expert by any chance, but I presume it needs to be handled somehow.
If no, maybe the better call will be to list the special cases per class, not method basis: so all virtual methods intaking Gd<T> in given class will be changed to Option<Gd<T>>.
If we forget a nullable parameter, this becomes a bug that now requires a breaking change in the API.
It seems that all methods that can receive a nullable parameter in the current state are unusable, so I wouldn't worry about breaking change in their implementation. TBH I'm more concerned about breaking change that the global change in all virtual methods would bring.
@Mercerenies What would you think about these? Do you deem doable tracking down the methods in ScriptLanguageExtension API which can receive null, or the approach of targeting whole classes would be better?
Bromeon
commented
2 months ago We should still make the default Option<Gd<T>>, and then explicitly list cases where we can assume non-null Gd<T> (whitelist). This way, the worst that can happen is that users need extra .unwrap(). But the other way around, Godot introducing new APIs may cause new bugs.
I know it's a lot of effort, that's why I was skeptical in the first place -- but simply choosing the easy path means that we may miss a lot of APIs where our assumptions are incorrect, and this is unfixable by the user.
We can consider doing wildcards on a class level if that helps, but I'm not sure if the condition "multiple virtual methods will behave the same within a class" is true? If the parameter doesn't refer to an object of the own type especially, why do you think we can assume this?
Breaking change all over is too bad, but it's a bug and will be done in v0.2. I would encourage users who know that certain parameters are non-null to speak up, so we can keep things non-breaking wherever possible.
ICEFIR
commented
2 months ago making the default Option<Gd<T>> does make sense as its way more defensive, but i do wonder how we'll handle Godot's method hints once nullability is fully implemented? They seems to already started adding nullability (as seen in https://github.com/godotengine/godot/pull/90767)
My concern is this: if nullability is introduced, will we need to explicitly mark every method we know to be non-nullable as Option<Gd<T>>? This approach seems to disregard the nullability method hints?
Though be it white or blacklist, either approach would be better then what we have at the moment :)
as for the list of functions, the godot doc repo should be a pretty good starting point? a quick doc repo search returns a bunch of situations that a null might be returned.
Btw I am able to work around the issue using a bridging call from GDScript. It's not ideal nor safe, but it does unblock my work. I am NOT recommending this approach, but if you just wants to unblock yourself, Here's what I've implemented:
On the Rust side, I'm using a global variable and a static function:
static mut EDITOR_PLUGIN_INSTANCE: Option<*mut MyEditorPlugin> = None;
#[func]
fn update_object(object: Option<Gd<Object>>) {
if let Some(object) = object {
if object.is_class(GString::from("TileSet")) {
let tile_set = object.cast::<TileSet>();
unsafe {
if let Some(instance) = EDITOR_PLUGIN_INSTANCE {
(*instance).tile_set_changed(tile_set);
godot_print!("TileSet updated");
} else {
godot_error!("instance not found");
}
}
}
}
}
fn tile_set_changed(&mut self, tile_set: Gd<TileSet>) {
self.tile_set = Some(tile_set);
godot_print!("TileSet changed");
}And on the GDScript side:
func _edit(object) -> void:
if object == null:
return
print("edit triggered, pass object to gdextension")
MyEditorPlugin.update_better_terrain_faster_object(object)Obviously this kind of bridging is not ideal for multiple of reasons. I was hoping that i will be able to acquire the plugin instance from gdscript directly but, it appears that the necessary functions (get_editor_plugin and get_editor_plugin_count) are not exposed from godot engine, forcing me to implement the solution on the GDExtension side, which is far from ideal.
Bromeon
commented
2 months ago My concern is this: if nullability is introduced, will we need to explicitly mark every method we know to be non-nullable as
Option<Gd<T>>? This approach seems to disregard the nullability method hints?
Definitely not, we would of course use this information as soon as GDExtension exposes it.
But the point is:
as for the list of functions, the godot doc repo should be a pretty good starting point? a quick doc repo search returns a bunch of situations that a null might be returned.
Hm, good idea! 💡
It seems like the docs generally provide info when something is null, not the opposite. So we can't safely say that a parameter is non-null, it might just not be documented 🤔 so maybe we should really start some crowdsourcing approach, and encourage people to contribute functions they regularly use and are known to be non-null.
One option is also to run the in-progress PRs and export nullability info from there, then hardcode that. I don't know how far those have progressed yet, so it might not yet be possible to (reliably) export anything. I'll also check with Godot developers...
ICEFIR
commented
2 months ago it will only do so for newer Godot versions (>= 4.4 at least), meaning people supporting older versions won't have nullability information.
Good point!
we can't safely say that a parameter is non-null
Indeed. I've been thinking - since for most situations we don't care when the input parameter is null, we could potentially mark all functions as taking Option<Gd
If Option.None, it would result in a no-op.
If data exists, it would transform it into Gd<T>.This should be relatively straightforward to implement. If users want to handle null cases themselves, they could opt out of the macro. Or we create two different version of the #[func] macro, one auto handles null and another pass Option<Gd<T>>. But that's just implementation detail, and you get the drill :)
If done correctly this wouldn't even break the backward compatibility
There might be edge cases where a function takes multiple parameters and only one of them could be null. But, at the end of the day, users can still choose to use the Option<Gd
so maybe we should really start some crowdsourcing approach
Second that!
Bromeon
commented
2 months ago If Option.None, it would result in a no-op. If data exists, it would transform it into Gd
.
This already exists as a function: Option::map 🙂
I don't think we should silently do nothing on the library side, if the parameter is null. This is almost always a bad idea, because it hides logic errors and makes things harder to debug.
ICEFIR
commented
2 months ago This already exists as a function
I always hesitant to use map for this purpose as, from functional programming perspective map isn't supposed to have side effects. But maybe i am being too pedantic.
besides what i was thinking was more of a higher order function, something along the line of
([a] -> b) -> [Maybe a] -> Maybe b which transform a function that doesn't deal with nullable params into one that handles them. But i digress
hides logic errors
Not too sure if i'd agree given i am mainly referring to engine calls, and this is just a default catch-all case that we still apply special case treatment on top, and will use nullability information once GDExtension exposes it. But i do not have too strong of an opinion on this, nor do i claim to be an expert in this area ;)
crowdsourcing
So far what we've found in this thread
EditorPlugin::_edit
EditorPlugin::_handles <- this takes in an Object*
EditorInspectorPlugin::parse_begin
EditorInspectorPlugin::parse_category
EditorInspectorPlugin::parse_group
EditorInspectorPlugin::parse_property
EditorInspectorPlugin::parse_end
and a quick look at ScriptLanguageExtension there's a bunch of function there that takes in pointers.... unfortunately i am not familiar enough with godot code base to call out what actually can be null, so someone needs to fill in here.
I will update back if i discover more
Bromeon
commented
2 months ago I always hesitant to use
mapfor this purpose as, from functional programming perspectivemapisn't supposed to have side effects. But maybe i am being too pedantic.
There's Option::inspect(), stabilized in Rust 1.76.
So far what we've found in this thread
Thanks! Yeah if we can collect the methods that's great, I could integrate them into the library 🙂
fpdotmonkey
commented
1 month ago
- Even once it does, it will only do so for newer Godot versions (>= 4.4 at least), meaning people supporting older versions won't have nullability information.
I wonder if Godot devs wouldn't be interested in backporting such a feature. It'd certainly be nice, though I know that the policy is that engine users should keep up with minor versions.
Bromeon
commented
1 month ago Good point, but typically the backporting is for 1-2 minor versions. But in practice, most people aren't further behind than that, either :slightly_smiling_face:
I'm trying to make an editor plugin:
For some reason
object's type is non-optionalGd<Object>, but the documentation says:As a result, when no node is selected, Godot tries to call
._edit(null)and crashes.Backtrace if needed
``` thread '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.