-
Notifications
You must be signed in to change notification settings - Fork 210
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
Generate accessor methods for indexed properties #970
Conversation
Getters and setters are now generated for indexed properties that are naturally accessible in GDScript (i.e. do not have '/' in the name), like `SpatialMaterial::albedo_texture`. Doc generation for the underlying accessors has also been improved. Close godot-rust#689
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.
Very nice, thanks a lot for this! 🙂
So far, for methods we have stripped the get_*
prefix:
https://github.com/godot-rust/godot-rust/blob/710281395a98b6f92b9951bce1045981b46311a1/bindings-generator/src/methods.rs#L249-L255
Since we're now using the same convention for property getters, I was first worried that this might cause conflicts (explicit getter + property provided by Godot leading to same name). But apparently this is not the case?
Also, there might now be things like node.scale()
which is a bit inconsistent/misleading, but I guess that's something we can live with. Are you aware of other such cases?
let rusty_name = rust_safe_name(&property.name); | ||
let rust_ret_type = ty.to_rust(); | ||
|
||
let method_bind_fetch = { | ||
let method_table = format_ident!("{}MethodTable", class.name); | ||
let rust_method_name = format_ident!("{}", property.getter); | ||
|
||
quote! { | ||
let method_bind: *mut sys::godot_method_bind = #method_table::get(get_api()).#rust_method_name; | ||
} | ||
}; | ||
|
||
let doc_comment = docs | ||
.and_then(|docs| { | ||
docs.get_class_method_desc( | ||
class.name.as_str(), | ||
&format!("get_{}", property.name), | ||
) | ||
}) | ||
.unwrap_or(""); | ||
|
||
let recover = ret_recover(&ty, *icall_ty); | ||
|
||
let output = quote! { | ||
#[doc = #doc_comment] | ||
#[doc = #maybe_unsafe_reason] | ||
#[inline] | ||
pub #maybe_unsafe fn #rusty_name(&self) -> #rust_ret_type { | ||
unsafe { | ||
#method_bind_fetch | ||
|
||
let ret = crate::icalls::#icall(method_bind, self.this.sys().as_ptr(), #property_index); | ||
|
||
#recover | ||
} | ||
} | ||
}; | ||
|
||
result.extend(output); |
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 is now quite a bit of code duplication:
- methods
- getters
- setters
Would it make sense to extract these parts into a function? It might need a few closure parameters to extract names, but I could imagine it still makes things easier to understand. If yes, this change could gladly be a separate commit.
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.
I've tried naive method extraction, but it didn't work very well due to the amount of arguments. Nested functions also didn't really help with readability. A larger scale refactoring is probably necessary if we want to meaningfully improve the clarity of this part, and I didn't want to do that without some prior communication.
If you think that's OK, I'll see what I can do about it.
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.
I see, thanks for investigating.
It's not urgent -- if you want to do a refactoring, I'd support that, but it's also OK if we leave it for now. In either case, I think this could be done as a separate PR, to unblock this already 🙂
bors try |
tryBuild succeeded: |
Yes. There are no conflicts currently, and I think it's unlikely that any will be introduced, given that properties and methods live under the same namespace in GDScript.
There are a lot of nouns that double as verbs in English, and I'm sure there are probably some of them in some property names out there. Given the lack of language-level properties in Rust we'll have to choose between living with a few ambiguous names, or stop following the general name conventions of the language. That should be another discussion, though, as I'm merely doing the same thing the library already does in this PR. |
I agree, it's good to be consistent with the rest. I also don't see the need to change the convention, as long as there are no naming conflicts and not too much confusion. I just checked the docs locally, looks good from what I've seen! So let's merge, if people don't like it, they will hopefully complain 🙂 bors r+ |
Build succeeded: |
For reference, Godot 4's take on it: |
Looks like the linked PRs are mostly about registering indexed properties from GDExtension, rather than generating bindings for existing ones for it? I'm not sure how useful the former is for |
You're right. It seems not that useful beyond syntax sugar in GDScript -- besides mass-declaring properties, it would also be possible to just have a |
Getters and setters are now generated for indexed properties that are naturally accessible in GDScript (i.e. do not have '/' in the name), like
SpatialMaterial::albedo_texture
. Doc generation for the underlying accessors has also been improved.Close #689