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

GDExtension: Pass count when freeing method and property lists for script instances #89055

Merged

Conversation

dsnopek
Copy link
Contributor

@dsnopek dsnopek commented Mar 1, 2024

Currently, the free_property_list and free_method_list functions on GDExtensionScriptInstanceInfo2 don't accept the number of items in the list. Since the GDExtension probably allocated memory for each item in the list, it will need to loop over all the elements in the list in order to free that memory.

In order to work around this, all existing GDExtensions that provide a scripting language do some sort of hack to save the list count. For example, the Luau bindings allocate the list with some extra room at the front to stash the count and then do some pointer math:

https://github.com/Fumohouse/godot-luau-script/blob/69d5e02fc1e94101cf31d7bd9b384ad2423cdd96/src/luau_script.h#L222

In any case, none of these hacks or workarounds would be necessary if Godot simply passed the count to the free_property_list and free_method_list functions, which is what this PR does!

(In addition to these changes, I also wrapped more of the code to support deprecated structs in ScriptInstanceExtension with #ifndef DISABLE_DEPRECATED to make builds without deprecated code a little lighter.)

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Code looks good and the feature makes sense

@dsnopek
Copy link
Contributor Author

dsnopek commented Mar 2, 2024

Thinking about this a little more, I think now that the ScriptInstanceExtension::deprecated_native_info struct contains multiple function pointers, it would probably make sense to dynamically allocate it, so that all ScriptInstanceExtensions (including those that don't use the deprecated pointers) don't get larger.

I'll update this PR to do that a little later.

@dsnopek dsnopek force-pushed the gdextension-script-free-lists branch from 27ea7ed to 0badf07 Compare March 2, 2024 16:13
@dsnopek
Copy link
Contributor Author

dsnopek commented Mar 2, 2024

I just pushed the changes to dynamically allocate the deprecated native info struct!

I re-tested this with the Luau GDExtension, using the older interface, and everything still seems to be working.

@akien-mga akien-mga merged commit 50ca190 into godotengine:master Mar 4, 2024
16 checks passed
@akien-mga
Copy link
Member

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