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

Add support for indexed properties #1186

Merged
merged 1 commit into from
Jul 26, 2023

Conversation

mihe
Copy link
Contributor

@mihe mihe commented Jul 21, 2023

Fixes #1182.
Requires godotengine/godot#79763.

This adds support for binding indexed properties from godot-cpp, which at the moment is not possible due to the p_index parameter in ClassDB::add_property being discarded completely.

This PR resolves this by passing p_index to the new GDExtension function classdb_register_extension_class_property_indexed, which is effectively the same as the old classdb_register_extension_class_property function but with the added p_index parameter.

I also snuck in the removal of PropertySetGet, which seems to be some old unused remnant from a distant past.

@dsnopek
Copy link
Collaborator

dsnopek commented Jul 25, 2023

Thanks!

Can you copy the gdextension_interface.h changes from your Godot PR, into here as well?

Otherwise, the code looks great and works perfectly in my testing :-)

@dsnopek dsnopek added the enhancement This is an enhancement on the current functionality label Jul 25, 2023
@dsnopek dsnopek added this to the 4.x milestone Jul 25, 2023
@mihe mihe marked this pull request as ready for review July 25, 2023 22:32
@mihe mihe requested a review from a team as a code owner July 25, 2023 22:32
Copy link
Collaborator

@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 won't pass tests on CI until after the Godot PR is merged, but since I've tested it locally I feel fine approving it. (We'll do a CI run just before merging anyway.)

@mihe
Copy link
Contributor Author

mihe commented Jul 25, 2023

I forgot to mention this, but note that this does technically mean that the old classdb_register_extension_class_property function is now unused in godot-cpp, so I'm not sure whether the whole LOAD_PROC_ADDRESS dance for that one should be removed perhaps.

Just give me the word and I'll yank it.

@dsnopek
Copy link
Collaborator

dsnopek commented Jul 25, 2023

No, I think we should leave it in. I doubt anyone is using that particular function, but I have seen folks directly calling some of the gdextension_interface_* functions, so I think we should load them all.

@dsnopek
Copy link
Collaborator

dsnopek commented Jul 26, 2023

Godot PR merged, and the tests here are now passing, so merging.

Thanks!

@dsnopek dsnopek merged commit d15550f into godotengine:master Jul 26, 2023
17 of 18 checks passed
@mihe mihe deleted the indexed-properties branch July 26, 2023 19:58
@akien-mga akien-mga modified the milestones: 4.x, 4.2 Oct 24, 2023
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.

Unable to bind indexed properties
3 participants