-
-
Notifications
You must be signed in to change notification settings - Fork 576
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
Fix object return value of builtin types' methods. #1363
Conversation
ca2da27
to
79f3e77
Compare
Thanks! I haven't tested, although, looking at the code I think you're right about the bug here. However, rather than having the For example, we have We could add an equivalent |
79f3e77
to
29d19fb
Compare
@dsnopek OK, I added |
There was a problem hiding this 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?
binding_generator.py
Outdated
@@ -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 |
There was a problem hiding this comment.
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
07f5a72
to
544675a
Compare
544675a
to
6a3753c
Compare
@dsnopek Although I make it like engine classes now, it seems that there have not method with enum return type in builtin types. |
There was a problem hiding this 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
Cherry-picked for 4.2 in PR #1410 |
Cherry-picked for 4.1 in PR #1411 |
Currently
Signal::get_object()
andCallable::get_object()
will return an invalidObject *
without converting byinternal::get_object_instance_binding()
.This pr fix this problem by adding necessary Conversion for builtin types in binding generator.