Generate accessor methods for indexed properties #970

chitoyuu · 2022-10-31T08:40:05Z

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

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

chitoyuu · 2022-10-31T08:43:47Z

Old docs

Notice how the docs included are for the transmission texture only:

New docs

Both the indexed accessors and the underlying ones have their original docs attached:

Bromeon

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?

Bromeon · 2022-10-31T09:41:13Z

bindings-generator/src/methods.rs

+            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 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.

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.

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 🙂

Bromeon · 2022-10-31T09:49:47Z

bors try

bors · 2022-10-31T10:03:45Z

try

Build succeeded:

full-ci

chitoyuu · 2022-10-31T10:47:47Z

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?

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.

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?

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.

Bromeon · 2022-10-31T13:51:36Z

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+

bors · 2022-10-31T14:10:20Z

Build succeeded:

full-ci

Bromeon · 2023-07-28T09:37:16Z

For reference, Godot 4's take on it:
godotengine/godot#79763
godotengine/godot-cpp#1186

chitoyuu · 2023-07-28T18:03:50Z

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 gdnative honestly, given we can simply use attributes to mass-declare properties if desired.

Bromeon · 2023-07-28T21:58:43Z

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 #[method] accepting an index parameter.

Bromeon added feature Adds functionality to the library c: bindings Component: GDNative bindings (mod api) labels Oct 31, 2022

Bromeon reviewed Oct 31, 2022

View reviewed changes

bors bot added a commit that referenced this pull request Oct 31, 2022

Try #970:

e8dc8f6

bors bot merged commit 51ff12f into godot-rust:master Oct 31, 2022

chitoyuu mentioned this pull request Nov 4, 2022

Documentation for SpatialMaterial::set_flag is incorrect. #699

Closed

chitoyuu deleted the feature/indexed-accessor branch July 28, 2023 17:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate accessor methods for indexed properties #970

Generate accessor methods for indexed properties #970

chitoyuu commented Oct 31, 2022

chitoyuu commented Oct 31, 2022

Bromeon left a comment •

edited

Loading

Bromeon Oct 31, 2022

chitoyuu Oct 31, 2022

Bromeon Oct 31, 2022

Bromeon commented Oct 31, 2022

bors bot commented Oct 31, 2022

chitoyuu commented Oct 31, 2022

Bromeon commented Oct 31, 2022

bors bot commented Oct 31, 2022

Bromeon commented Jul 28, 2023 •

edited

Loading

chitoyuu commented Jul 28, 2023 •

edited

Loading

Bromeon commented Jul 28, 2023

Generate accessor methods for indexed properties #970

Generate accessor methods for indexed properties #970

Conversation

chitoyuu commented Oct 31, 2022

chitoyuu commented Oct 31, 2022

Old docs

New docs

Bromeon left a comment • edited Loading

Choose a reason for hiding this comment

Bromeon Oct 31, 2022

Choose a reason for hiding this comment

chitoyuu Oct 31, 2022

Choose a reason for hiding this comment

Bromeon Oct 31, 2022

Choose a reason for hiding this comment

Bromeon commented Oct 31, 2022

bors bot commented Oct 31, 2022

try

chitoyuu commented Oct 31, 2022

Bromeon commented Oct 31, 2022

bors bot commented Oct 31, 2022

Bromeon commented Jul 28, 2023 • edited Loading

chitoyuu commented Jul 28, 2023 • edited Loading

Bromeon commented Jul 28, 2023

Bromeon left a comment •

edited

Loading

Bromeon commented Jul 28, 2023 •

edited

Loading

chitoyuu commented Jul 28, 2023 •

edited

Loading