Virtual methods must take Option<Gd<T>> instead of Gd<T>
#494
Comments
|
seems related to #156 |
|
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. |
Bromeon
added
bug
c: register
|
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 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: |
|
@Bromeon I'm not that knowledgeable about inner workings of I'm asking about the implications to assess the danger of reverse occuring: I mean Godot's non-nullable marked as nullable on |
|
@StatisMike yes, that's exactly the plan. I'm working on a proposal that would combine this with an other feature,
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 |
|
@Bromeon I've got the same message for If I find any more of these, I can edit this message to keep track of them but not bloat this comment thread. |
|
Yes, I think we have to conservatively change all object parameters in virtual functions to |
|
@Bromeon For now it seems to be a few functions affected - I think making every Maybe a special cases could be made there? Or just only |
|
@StatisMike This issue is blocking any attempt to use those functions right now. I'd rather have several functions that always return a |
|
@StatisMike But who maintains the list of functions that are nullable? I have basically two options:
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? |
|
@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
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 |
|
We should still make the default 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. |
|
making the default My concern is this: if nullability is introduced, will we need to explicitly mark every method we know to be non-nullable as 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. |
Definitely not, we would of course use this information as soon as GDExtension exposes it. But the point is:
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... |
Good point!
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> and use a macro to handle the different cases: 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 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> approach if they prefer.
Second that! |
This already exists as a function: 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. |
I always hesitant to use 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
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 ;)
So far what we've found in this thread and a quick look at I will update back if i discover more |
There's
Thanks! Yeah if we can collect the methods that's great, I could integrate them into the library 🙂 |
Bromeon
changed the title
Option<Gd<T>> instead of Gd<T>
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. |
|
Good point, but typically the backporting is for 1-2 minor versions. But in practice, most people aren't further behind than that, either 🙂 |
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
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.
The text was updated successfully, but these errors were encountered: