Implementation of godots GDExtensionScriptInstance

[TitanNano]

I have recently worked on an implementation of GDExtensionScriptInstance and would now like to contribute it to gdext. This is required to be able to properly implement a custom ScriptExtension / ScriptLanguageExtension.

Please let me know your thoughts on the changes, and I welcome every feedback.

[Bromeon]

This is very cool, thanks a lot! 🙂

Regarding the CI, you already found some issues. The classes Script, ScriptLanguage would need to be enabled here.

Did you have a project that uses them? I'm thinking about ways to test this (maybe in an automated way)...? 🤔

[GodotRust]

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-492

[TitanNano]

Did you have a project that uses them? I'm thinking about ways to test this (maybe in an automated way)...? 🤔

@Bromeon I'm using them in https://github.com/titannano/godot-rust-script.

CI tests are also something I have been thinking about. To create a ScriptInstance Godot needs a ScriptExtension and that in turn needs a ScriptLanugageExtension. So I'm not sure how straightforward it would be to test a script instance without implementing a (bare bones) script language first.

💭 Perhaps we could implement the minimally stubbed out interfaces in itest and attach some script to a Godot object… I haven't tried it yet.

[lilizoey]

Thank you! im glad someone did this since i remember it being kinda tedious to do last time.

You leak values a lot without much justification, it'd be nice to know why it's fine to leak memory when you do. Whether it is because you're actually passing ownership to godot, or the memory will later be freed by another callback.

Maybe it'd be nice if we made create_script_instance return a smart pointer? one which stores the instance pointer, and is able to check if it has been freed yet. And have that object implement ScriptInstance?

Also you haven't documented any of the public methods/structs, you should do that.

And some tests as @Bromeon mentioned would be good. It should be possible to create a very barebones script "language", maybe one that just checks if a script contains one of a few hardcoded strings to represent the different things we wanna check?

[Bromeon]

Regarding tests:

💭 Perhaps we could implement the minimally stubbed out interfaces in itest and attach some script to a Godot object… I haven't tried it yet.

That would be great, because it would likely reveal UB and memory leaks during CI. Currently there's no way to check the safety of this PR, since the functionality is never used.

Let us know if you need some support there 🙂

[TitanNano]

Maybe it'd be nice if we made create_script_instance return a smart pointer? one which stores the instance pointer, and is able to check if it has been freed yet. And have that object implement ScriptInstance?

@lilizoey so create_script_instance creates a GDExtensionScriptInstanceInfo which is immediately passed to Godots script_instance_create which wraps the InstanceInfo with a GDExtensionScriptInstance and returns the pointer to it back to the GDExtension. The GDExtension then has to pass this pointer from the extensions ScriptExtension implemenation to back to the engine. Thereafter, the script instance is out of our hands. Godot only calls the GDExtensionScriptInstanceInfo::free_func once it wants to free the InstanceInfo.

Also you haven't documented any of the public methods/structs, you should do that.

yes, I should do that. Thanks.

[TitanNano]

@Bromeon I just noticed that Godot 4.2 will introduce a second API (gdextension_script_instance_create2) for creating script instances. I implemented this PR against Godot 4.1 and wasn't aware of this change. Would you be fine with merging this PR targeting the 4.1 API and looking into the newer API afterward? Or should I try to implement this against the newer API directly? I haven't looked at the new API and am not aware what exactly changed, so I would prefer to finish this and then tackle the API change separately.

[Bromeon]

I implemented this PR against Godot 4.1 and wasn't aware of this change. Would you be fine with merging this PR targeting the 4.1 API and looking into the newer API afterward?

Yep, that's definitely fine! Let's tackle one thing at a time, this is already great work 🙂
As a nice bonus, we could then maybe keep a fallback version of script instances for Godot 4.1, even later.

Right. This is being cleaned up in the script instance free_method_list_func... I see now that this is not ideal. Any suggestion how we could improve it.

In general, I think this is OK and the workflow intended by Godot. The surprise is probably there because this is not obvious from the code when just looking at one of the functions.

As mentioned, I would probably write a few functions that convert Rust domain models to C pointer types and back (so all conversions go through those functions). Maybe they could be named something that highlights the change of ownership, e.g. transfer_xy_to_godot and transfer_xy_from_godot. Or "submit/retrieve", "hand over/take over", "acquire/release", ...

[Bromeon]

Thanks a lot!

I added lots of small formatting comments, but most of them come with suggestions that you can directly commit in the UI, and later squash locally.

Also, how do we want to proceed with tests? You said you're already using this in a project of yours?

[TitanNano]

I have now implemented itests for the methods which can be called via a Godot API.

While implementing tests, I came into a situation where the engine would crash because the ScriptInstance::refcount_decremented function would panic due to a todo!. It was quite difficult to notice this because no panic information was being printed. I have therefore now wrapped all calls into the trait with a handle_panic.

Tests are failing in CI because of an outdated cache where godot-codegen is not being rebuilt? At least, I think that is what is happening. Besides that, there appears to be a memory leak, and I'm not yet sure where it originated.

EDIT: I have so far narrowed the memory leak down to the property_list test. No other test appears to leak memory.

EDIT: I was able to resolve the memory leaks.

[Bromeon]

Rebased onto master (after Godot 4.2 change). Most things look good to me now.

A general note about the public API. The test does this:

#[godot_api]
impl IScriptExtension for TestScript {
    unsafe fn instance_create(&self, _for_object: Gd<Object>) -> *mut c_void {
        create_script_instance(TestScriptInstance::new(self.to_gd().upcast())) as *mut c_void
    }

    fn can_instantiate(&self) -> bool {
        true
    }
}

It would be good if we could somehow abstract the raw FFI types for the user. Remember, sys:: (i.e. the godot-ffi crate) is not part of the public API. I'm aware that not all the things have been properly mapped to public pendants yet, but let's avoid them where possible.

For example, instance_create could directly return a *mut c_void, no? Then your implementation could become simpler (and a bit more explicit with the let script declaration):

#[godot_api]
impl IScriptExtension for TestScript {
    unsafe fn instance_create(&self, _for_object: Gd<Object>) -> *mut c_void {
        let script: Gd<Script> = self.to_gd().upcast();
        create_script_instance(TestScriptInstance::new(script))
    }

    fn can_instantiate(&self) -> bool {
        true
    }
}

(Btw, I think we mark IScriptExtension::instance_create incorrectly as unsafe, but we can fix that later...)

[TitanNano]

It would be good if we could somehow abstract the raw FFI types for the user. Remember, sys:: (i.e. the godot-ffi crate) is not part of the public API. I'm aware that not all the things have been properly mapped to public pendants yet, but let's avoid them where possible.

@Bromeon so the last sys type still left in the public API is sys::GDExtensionCallErrorType. How should we deal with that?

[Bromeon]

so the last sys type still left in the public API is sys::GDExtensionCallErrorType. How should we deal with that?

I think that's OK for now, we still need to map some remaining ones. Maybe you can add a // TODO comment.

Thanks a lot for all the updates! 👍

[Bromeon]

@lilizoey any more comments on this one? Do you think we can merge a first version soon?

[lilizoey]

I think it looks good yeah

[Bromeon]

@TitanNano Thanks a lot for the great work and your patience during the lengthy review process! 👍

I can fix the remaining CI issues.