Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dsnopek
Copy link
Collaborator

@dsnopek dsnopek commented Aug 7, 2024

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:

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 dsnopek added the enhancement This is an enhancement on the current functionality label Aug 7, 2024
@dsnopek dsnopek added this to the 4.x milestone Aug 7, 2024
@dsnopek dsnopek requested a review from a team as a code owner August 7, 2024 19:55
@dsnopek dsnopek force-pushed the script-instance-wrapper branch from b8ecc3d to 11f8c4c Compare August 12, 2024 21:41
@dsnopek
Copy link
Collaborator Author

dsnopek commented Aug 12, 2024

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 dsnopek force-pushed the script-instance-wrapper branch 3 times, most recently from 649b5ff to 00c42a5 Compare January 5, 2025 03:09
@piiertho
Copy link
Contributor

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

@dsnopek
Copy link
Collaborator Author

dsnopek commented Apr 28, 2025

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 dsnopek force-pushed the script-instance-wrapper branch 3 times, most recently from 5bf7efc to cf248a0 Compare April 29, 2025 21:00
@dsnopek
Copy link
Collaborator Author

dsnopek commented Apr 29, 2025

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 dsnopek force-pushed the script-instance-wrapper branch from 43c5844 to 70f584e Compare April 29, 2025 21:13
@dsnopek dsnopek changed the title [DRAFT] Add ScriptInstanceExtension class which mimicks the API of Godot's internal ScriptInstance class [DRAFT] Add ScriptInstance class which mimicks the API of Godot's internal ScriptInstance class Apr 29, 2025
@dsnopek dsnopek force-pushed the script-instance-wrapper branch from 70f584e to 59bd75f Compare April 30, 2025 11:33
@dsnopek dsnopek force-pushed the script-instance-wrapper branch from 59bd75f to 2e40d13 Compare May 2, 2025 14:52
@dsnopek
Copy link
Collaborator Author

dsnopek commented May 2, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This is an enhancement on the current functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants