godot-cpp icon indicating copy to clipboard operation
godot-cpp copied to clipboard

[DRAFT] Add `ScriptInstance` class which mimicks the API of Godot's internal `ScriptInstance` class

Open dsnopek opened this issue 1 year ago • 5 comments
trafficstars

Background

Godot's internal ScriptInstance class (which is used in implementing scripting languages, like GDScript) isn't exposed the normal way, because we don't want scripts (ex a user script written in GDScript) to be able to directly interact with it.

Instead, GDExtensions get a special API from gdextension_interface.h to create script instances, allowing GDExtensions to provide new scripting languages for Godot.

However, working directly with that C API from C++ isn't ideal. And it also means big differences between GDExtensions and modules.

A number of extension projects have made their own C++ wrapper classes over this API to make life easier:

  • https://github.com/Fumohouse/godot-luau-script/blob/master/src/scripting/luau_script.h#L216
  • https://gitlab.com/snopek-games/godot-gravity-lang/-/blob/main/src/gravity_script_instance.h?ref_type=heads#L40

Given that this is something frequently done, I personally think it makes sense to have a wrapper in godot-cpp, so each project doesn't need to recreate it.

Implementation

This PR has the implementation I used in 'godot-gravity-lang', updated for the latest gdextension_interface.h, but I haven't re-tested it.

~~The main thing I don't like about it, is that it doesn't exactly mimick the API of ScriptInstance because it uses some types from gdextension_interface.h, rather than the equivalent Godot types (most of which also exist in godot-cpp).~~

~~The reason for this is that we'd need to copy data back and forth between the Godot types and gdextension_interface.h types, but script instances need to aim to be fairly performant. However, complete API compatibility may be worth it? It's something we'll need to discuss.~~

UPDATE: We discussed and decided that it was more important to mimick the Godot API as much as possible, and not worry about the performance of copying data in some cases, so the PR has been updated to reflect that.

Making a DRAFT for now so that we can discuss whether or not we want this in godot-cpp, and if this implementation makes sense.

dsnopek avatar Aug 07 '24 19:08 dsnopek

We discussed this PR at the last GDExtension meetup, and decided that it'd be best for ScriptInstanceExtension to exactly match the method signatures from Godot, even if that means allocating and copying a bunch of extra memory.

I've updated this PR with everything that I think we need to do in order to do that, however, I still haven't had a chance to actually test it. So, leaving as a draft until I do.

dsnopek avatar Aug 12 '24 21:08 dsnopek

Do you think it is possible to add the same for ScriptInstancePlaceholder in this PR ?

piiertho avatar Apr 28 '25 16:04 piiertho

Do you think it is possible to add the same for ScriptInstancePlaceholder in this PR ?

Yes, I think we could do a similar thing with PlaceHolderScriptInstance, but I don't know if it should be in this PR.

This one has already been sitting here for ~9 months because it's gotten insufficient testing. Adding a new thing means even more things to test :-)

If you had time to help test this PR, that would be much appreciated!

dsnopek avatar Apr 28 '25 16:04 dsnopek

I've made some updates (a few of which were discussed on RocketChat):

  • Renamed from ScriptInstanceExtension to ScriptInstance to match the class name within Godot
  • Added a default implementation of ScriptInstance::get_property_state() copied from the Godot version
  • Avoid thread_local on MacOS when hot-reload is enabled (just like in wraped.hpp/.cpp) since there's issues with thread_local breaking reload on that platform

dsnopek avatar Apr 29 '25 21:04 dsnopek

I've added an implementation of PlaceHolderScriptInstance now too, but this is all untested code

dsnopek avatar May 02 '25 14:05 dsnopek