Skip to content
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

Expose PlaceHolderScriptInstance to GDExtension #80394

Conversation

maiself
Copy link
Contributor

@maiself maiself commented Aug 8, 2023

Exposes PlaceHolderScriptInstance used by the gdscript and csharp_script modules in Godot to GDExtension.

While use of this class is technically optional as placeholder implementation can be done with the existing interface, exposing it allows extensions to avoid reimplementing the entire behavior.

This would likely be useful for existing language bindings:

  • godot_dart has used their normal ScriptInstance implementation and disabled call when used as a placeholder.
  • godot-luau-script has completely reimplemented PlaceHolderScriptInstance

I've started using this in my Python bindings and it seems to work quite well. Without this I would be doing what has been done for the luau bindings.

@maiself maiself requested a review from a team as a code owner August 8, 2023 01:40
@maiself maiself force-pushed the expose-placeholder-script-instance-to-gde branch 2 times, most recently from 91055ac to 9c2651a Compare August 8, 2023 01:57
@akien-mga akien-mga added this to the 4.x milestone Aug 8, 2023
Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

I haven't had a chance to test this yet, which I'd really like to do, but at a high-level, this looks great :-)

core/extension/gdextension_interface.h Outdated Show resolved Hide resolved
core/extension/gdextension_interface.cpp Outdated Show resolved Hide resolved
@maiself maiself force-pushed the expose-placeholder-script-instance-to-gde branch 2 times, most recently from 6e04259 to 6ca7ced Compare August 8, 2023 22:01
Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've started messing around with this with godot-cpp, and I think it'd be sufficient for my cppscript experiment.

I also spent some time reading through the Luau binding's code, and I think this would be sufficient for its purposes as well.

However, I noticed a small naming issue which is pointed out in the code comments. But after that is fixed, I think I'm personally happy with this PR!

core/extension/gdextension_interface.h Outdated Show resolved Hide resolved
core/extension/gdextension_interface.h Outdated Show resolved Hide resolved
Co-authored-by: David Snopek <dsnopek@gmail.com>
@maiself maiself force-pushed the expose-placeholder-script-instance-to-gde branch from 70e98a2 to 4b5da7e Compare August 28, 2023 21:44
@maiself
Copy link
Contributor Author

maiself commented Aug 28, 2023

Fixed the naming issue and rebased on master.

@akien-mga akien-mga modified the milestones: 4.x, 4.2 Aug 28, 2023
Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This looks great to me :-)

akien-mga added a commit to akien-mga/godot that referenced this pull request Aug 29, 2023
…script-instance-to-gde

Expose PlaceHolderScriptInstance to GDExtension
@akien-mga akien-mga merged commit 38a69c0 into godotengine:master Aug 29, 2023
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@Sauermann Sauermann mentioned this pull request Aug 29, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants