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

Fix object return value of builtin types' methods. #1363

Conversation

Daylily-Zeleen
Copy link
Contributor

@Daylily-Zeleen Daylily-Zeleen commented Jan 18, 2024

Currently Signal::get_object() and Callable::get_object() will return an invalid Object * without converting by internal::get_object_instance_binding().

This pr fix this problem by adding necessary Conversion for builtin types in binding generator.

@Daylily-Zeleen Daylily-Zeleen requested a review from a team as a code owner January 18, 2024 09:35
@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/fix_object_return_value_of_builtin_types_methods branch 2 times, most recently from ca2da27 to 79f3e77 Compare January 18, 2024 10:56
@dsnopek
Copy link
Collaborator

dsnopek commented Jan 18, 2024

Thanks!

I haven't tested, although, looking at the code I think you're right about the bug here.

However, rather than having the binding_generator.py add the call to internal::get_object_instance_binding(), I think it'd be better to handle this similar to how we do in other situations.

For example, we have internal::_call_native_mb_ret() (which is similar to the internal::_call_builtin_method_ptr_ret() used here), and it has a partner function to handle returning objects called internal::_call_native_mb_ret_obj() (note the _obj at the end).

We could add an equivalent internal::_call_builtin_method_ptr_ret_obj() and then binding_generator.py could call that when the method returns an Object *.

@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/fix_object_return_value_of_builtin_types_methods branch from 79f3e77 to 29d19fb Compare January 19, 2024 06:18
@Daylily-Zeleen
Copy link
Contributor Author

@dsnopek OK, I added internal::_call_builtin_method_ptr_ret_obj(), it should be no problem now.

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!

However, it would be nice if this code was also re-organized to look like the similar pre-existing code for engine classes - see https://github.com/godotengine/godot-cpp/blob/master/binding_generator.py#L1585

I personally find that a little easier to follow, and it'll make maintaining both chunks of code easier if they have roughly the same structure. Comparing with that code, we also may be missing a case related to enums?

@@ -964,8 +964,20 @@ def generate_builtin_class_source(builtin_api, size, used_classes, fully_used_cl
result.append(method_signature + "{")

method_call = "\t"
need_additional_right_bracke = False
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's a type-o here - this variable name should end with a t. However, I think it'd be better if this was made to look more like the code for engine classes which use is_ref as the variable for this. See https://github.com/godotengine/godot-cpp/blob/master/binding_generator.py#L1585

@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/fix_object_return_value_of_builtin_types_methods branch 3 times, most recently from 07f5a72 to 544675a Compare February 12, 2024 19:19
@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/fix_object_return_value_of_builtin_types_methods branch from 544675a to 6a3753c Compare February 12, 2024 19:20
@Daylily-Zeleen
Copy link
Contributor Author

@dsnopek Although I make it like engine classes now, it seems that there have not method with enum return type in builtin types.
And the structure of engine classes' methods is different from builtin types' methods in "extension_api.json", so I ignore "meta".

@dsnopek dsnopek added bug This has been identified as a bug cherrypick:4.1 cherrypick:4.2 labels Feb 12, 2024
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 looks great to me

@dsnopek dsnopek merged commit 9a13efa into godotengine:master Feb 12, 2024
12 checks passed
@Daylily-Zeleen Daylily-Zeleen deleted the daylily-zeleen/fix_object_return_value_of_builtin_types_methods branch February 13, 2024 06:45
@dsnopek
Copy link
Collaborator

dsnopek commented Mar 11, 2024

Cherry-picked for 4.2 in PR #1410

@dsnopek
Copy link
Collaborator

dsnopek commented Mar 11, 2024

Cherry-picked for 4.1 in PR #1411

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This has been identified as a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants