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

Allow implementing get_class_category in GDExtension #78995

Merged
merged 1 commit into from
Sep 27, 2023

Conversation

Gallilus
Copy link
Contributor

@Gallilus Gallilus commented Jul 3, 2023

Change GetClassCategory so it is handled in native directly.

After #78392 I tried extending MyScriptExtension to include get_class_category while I was probably doing something wrong it seemed more consistent to also get_class_category true as a native function and let the extension handle it the way they want.

CC @dsnopek

@Gallilus Gallilus requested review from a team as code owners July 3, 2023 18:03
@YuriSizov YuriSizov changed the title add GDExtensionScriptInstanceGetClassCategory Allow implementing get_class_category in GDExtension Jul 5, 2023
@YuriSizov YuriSizov added this to the 4.2 milestone Jul 5, 2023
@dsnopek
Copy link
Contributor

dsnopek commented Jul 7, 2023

I'm just not very familiar with how scripting languages are implemented in Godot, so I feel really under-qualified to comment on whether this is a good idea or not. I'm hoping to learn more about this soon (I have an experiment I want to try ;-)) but until then, I think we'll need feedback from someone who's more well versed this area.

However, as to the implementation, I do have one comment with regard to making this change in a backward compatible way (assuming we decide to do this)...

@Gallilus Gallilus marked this pull request as draft July 8, 2023 13:06
@dsnopek
Copy link
Contributor

dsnopek commented Jul 14, 2023

Discussed at the GDExtension meeting, and we think adding this makes sense! Still needs compatibility addressed per previous comment.

@Gallilus Gallilus marked this pull request as ready for review August 19, 2023 07:14
@Gallilus Gallilus marked this pull request as draft August 19, 2023 07:15
@Gallilus Gallilus force-pushed the master branch 3 times, most recently from ed84380 to fcb8e86 Compare August 19, 2023 11:28
@dsnopek
Copy link
Contributor

dsnopek commented Sep 18, 2023

@Gallilus If this is something you're still interested in finishing, here's the steps that I think are needed to do it:

  1. Rebase on latest master
  2. We've already added a GDExtensionScriptInstanceInfo2 struct in master - you can just add your new get_class_category_func and free_class_category_func to that struct
  3. And, there's already a gdextension_script_instance_create2 function too - you'll just need to add code to gdextension_script_instance_create() to set your new fields to nullptr when using the old struct

This is assuming that this will get merged for Godot 4.2 when we can still make changes to the 2 version of the struct -- it doesn't, then it will need to add a new 3 version of the struct and function.

Since we've already approved the general idea behind this PR at a previous GDExtension meeting, I think it should be a quick one to review and approve!

core/object/script_language_extension.h Outdated Show resolved Hide resolved
core/extension/gdextension_interface.h Outdated Show resolved Hide resolved
@Gallilus Gallilus force-pushed the master branch 2 times, most recently from bd2fe63 to 73beb40 Compare September 26, 2023 20:56
@Gallilus
Copy link
Contributor Author

Tested it with the latest build.
Works when adding get_class_category to instance_info.
And there is no class category when not creating the function. For me, it used to crash. wanted to mention thist as some may expect the default implementation to take over.

@Gallilus Gallilus marked this pull request as ready for review September 26, 2023 21:05
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 is looking good!

Aside from my comment on the code, there's only one more little thing missing - in gdextension_script_create_instance() we need a line that does:

info_2->get_class_category_func = nullptr;

So that that value is set to nullptr when older GDExtensions use script_instance_create() with the old struct.

core/object/script_language_extension.h Outdated Show resolved Hide resolved
Change GetClassCategory so it is handled in native directly
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 is looking good to me :-)

@YuriSizov YuriSizov merged commit 5f53ec9 into godotengine:master Sep 27, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

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.

3 participants