Property attribute for GDClass. by mhoff12358 · Pull Request #31 · godot-rust/gdext (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation30 Commits0 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Working on #3

This is in-progress. I have tested that multiple properties do seem to work on the Godot 4 beta 6 release. The code could undoubtedly use a cleanup pass before going in.

It only handles the basic case with a property + getter/setter. There's no support for editor hints.

The issue I've run into is that the way attribute arguments are being parsed at the moment isn't expressive enough. They're parsed into a Map<String, KvValue>, and KvValue only handles literals or identifiers, which doesn't seem to account for something like godot_ffi::VariantType::Int. Either KvValue will have to be expanded to account for more general expressions, or the property attribute will have to parse its arguments some other way.

Bromeon added feature

Adds functionality to the library

c: register

labels

Nov 25, 2022

Choose a reason for hiding this comment

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

Thanks a lot for this contribution, very nice to see property support! 👍

I need to look closer into it over the next few days, here just a broad first feedback. I would appreciate if you could outline in code how you intend the API to look, so it's clearer what the proc-macro is doing internally. This would also serve as a base for writing tests.

I also happily merge PRs that implement only a part of property functionality. In fact, smaller PRs are often at an advantage because they are easier to review, easier to unit-test, need less effort in implementation and thus also run a lower risk of requiring rewrite/discarding. So don't hesitate to go for an initial implementation (like the current, or even less) that is not feature-complete, but can serve as a base for future improvements! It's also very well possible that things change anyway for consistency/naming/ergonomic reasons.

Comment on lines 336 to 338

let name = proc_macro2::Literal::from_str(&property_info.name).unwrap();
let getter = proc_macro2::Literal::from_str(&property_info.getter).unwrap();
let setter = proc_macro2::Literal::from_str(&property_info.setter).unwrap();

Choose a reason for hiding this comment

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

Since you expect those literals to be strings, you might want to use Literal::string().

Also, for methods we might just want to use symbols (setter = my_setter instead of setter = "my_setter") for consistency with #[class(base=Node)] and #[gdextension(entry_point=my_func)], but that can always be changed later.

Choose a reason for hiding this comment

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

The reason I went with ="my_setter" rather than =my_setter is because there's no guarantee that users will expose methods to godot using their rust name (though I guess this library currently enforces that).
I've dealt with enough binding libraries that I've seen all kinds of conflicting naming requirements and would recommend building to support divorcing rust's function name form the godot-exposed name. I could totally see going with the function identifier for now and adding an option for string values later once #[func] gets support for renaming.

What do you want me to go with for this first PR?

Choose a reason for hiding this comment

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

Oh, the reason I couldn't go with Literal::string() was (IIRC) that the property_info fields include the enclosing " characters. So #getter would expand to "\"my_getter\"" rather than "my_getter".
Is there a better way to handle this?

Choose a reason for hiding this comment

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

The reason I went with ="my_setter" rather than =my_setter is because there's no guarantee that users will expose methods to godot using their rust name (though I guess this library currently enforces that).

That's a valid point indeed. It depends a bit how this is used -- is it referring to a Rust function, or just the name of a registered function? Maybe some example code would be helpful 😉

Is there a better way to handle this?

I'm not aware of one, feel free to write a utility function for this if it occurs multiple times.

Choose a reason for hiding this comment

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

Responding to/resolving some comments.

Comment on lines 336 to 338

let name = proc_macro2::Literal::from_str(&property_info.name).unwrap();
let getter = proc_macro2::Literal::from_str(&property_info.getter).unwrap();
let setter = proc_macro2::Literal::from_str(&property_info.setter).unwrap();

Choose a reason for hiding this comment

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

What do you want me to go with for this first PR?

Comment on lines 336 to 338

let name = proc_macro2::Literal::from_str(&property_info.name).unwrap();
let getter = proc_macro2::Literal::from_str(&property_info.getter).unwrap();
let setter = proc_macro2::Literal::from_str(&property_info.setter).unwrap();

Choose a reason for hiding this comment

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

Thanks for the update! I think at this stage this would be most helpful:

I would appreciate if you could outline in code how you intend the API to look, so it's clearer what the proc-macro is doing internally. This would also serve as a base for writing tests.

So before further implementation, maybe write an example code that demonstrates the features implemented in this PR, from a user perspective. This could then serve as a base for API discussion 🙂

I added an integration test, that using a godot script to get & set the property. This then also self-documents in the code how it's meant to be used (though it could probably use actual documentation, once we finalize the API I should write some, lemme know where).

Duplicating the example here for discussion:

#[derive(GodotClass)] #[class(base=Node)] #[property(name = "val", getter = "get_val", setter = "set_val")] struct HasProperty { val: i32, #[base] base: Base, }

#[godot_api] impl HasProperty { #[func] pub fn get_val(&self) -> i32 { return self.val; }

#[func]
pub fn set_val(&mut self, val: i32) {
    self.val = val;
}

}

#[godot_api] impl GodotExt for HasProperty { fn init(base: Base) -> Self { HasProperty { val: 0, base } } }

The rest of the features (func, base classing) that I saw all used macros, so I initially jumped to a macro. And an attribute on the GDClass derive seemed best way to handle it.
If we wanted to be more prescriptive a much cleaner API would be to do something like inclue an attribute on the #[func] macro above the getter that registers it as a property (with the name of the property being gotten by stripping the get_ prefix. Though if people wanted that I'd recommend having both.

Edit bromeon: ```rs code tags

Thanks for the example!

#[derive(GodotClass)] #[class(base=Node)] #[property(name = "val", getter = "get_val", setter = "set_val")] struct HasProperty { val: i32, #[base] base: Base, }

This is good as a start, maybe mid-term we can go for an API similar to the one we have in GDNative, see docs for some insights. But I'd be happy to start with PR that functionally works, and then tweak the proc-macro API myself later 🙂

I'll comment on the implementation later today.

We should probably be careful with adding new item-level attributes. They go out of control pretty fast. We've had that in GDNative (godot-rust/gdnative#848) and hopefully not again here.

Also, for methods we might just want to use symbols (setter = my_setter instead of setter = "my_setter") for consistency with #[class(base=Node)] and #[gdextension(entry_point=my_func)], but that can always be changed later.

Regarding this, I want to add that the common practice in the ecosystem right now is to quote identifiers and paths in attribute parameters. See serde, derivative for some prominent examples.

IIRC it mostly have to do with complications parsing paths and complex types (<Thing<Foo> as Trait>::Assoc kind of stuff) directly within a Meta context. It might not be a current concern for us, but that's the general syntax users have come to expect from similar APIs.

We need to decide whether the convenience right now is worth the surprise factor, and a possible future breaking change here.

Regarding this, I want to add that the common practice in the ecosystem right now is to quote identifiers and paths in attribute parameters. See serde, derivative for some prominent examples.

Yeah, it can also look a bit nicer if more complex values are in quotes (syntax highlighting makes clear what is the value).

Not sure about super-short idents. Would you then write #[class(base="Node")]? In GDNative we also kept #[inherits(Node)], not #[inherits("Node")].

Not sure about super-short idents. Would you then write #[class(base="Node")]? In GDNative we also kept #[inherits(Node)], not #[inherits("Node")].

There's a tiny difference here though. The current GDNative inherits attribute takes a MetaList instead of a MetaNameValue (in syn terms), and generally identifiers do feel a lot more natural there, the most common example being #[derive(Debug)]. In MetaNameValues however I do feel that #[class(base="Node")] comes more naturally, yes. I can't really recall many examples of bare identifiers being used in this position.

This is mostly an argument from implementation perspective though -- for a user, it could make sense to have values in (key=value) and (value) syntax treated the same. I would not change anything in GDNative, so this is more for moving forward in GDExtension.

We currently support this:

#[class(init, base=RefCounted)] // or, equivalent: #[class(init)]

and here, init is a key, not value. Interestingly, GitHub's syntax highlighting recognizes the value, seemingly based on capitalization 😀

For GDExtension, I'm not sure if we would even have the syntax

somewhere, or rather use = directly.

This is mostly an argument from implementation perspective though -- for a user, it could make sense to have values in (key=value) and (value) syntax treated the same.

It's not -- I'm only talking in terms of syn type names because we don't have natural language names for those "grammatical constructs". Regardless of the technicality, the common expectation would still rather have #[class(init, base="RefCounted")] over #[class(init, base=RefCounted)]. This is evident even in this very PR: the author may have rationalized that he is quoting the identifiers for renaming support when asked about it (which honestly doesn't really stand), but still it's important that he chose intuitively to start with quoting them in the first place.

Of course, the decision here is yours to make. I'm just trying to say that going forward, you are likely to see this as a point of contention for many proc-macro PRs to come.

I agree with "" probably being more consistent.

What I meant is: from perspective of a GDExtension user, I would probably regard the #[attr("Value")] syntax more in line with this convention than #[attr(Value)], given there are constructs like #[class(init)]. Otherwise the same syntax sometimes means key, sometimes value. GDExtension handles this a bit differently than GDNative -- the tokens inside ( ) are treated like a map, with commas separating elements.

@mhoff12358 sorry for the off-topic discussion 😀
For this PR, you can gladly continue with "", if needed we can always change things later.

As opposed to whom? I see that it's your stance to draw a clear separation between the two projects, which I'll respect. 😃

Haha no, that's not how I meant it 😅 more that the proc-macro APIs are naturally diverging due to underlying architecture (#[base] and GodotExt are good examples for this). But at the same time, I'll try to use the learnings from GDNative, which could mean (key="value") in this case.

In general, I believe both projects can greatly benefit from each other. There's a ton of prior art in GDNative for all kinds of mechanisms, and years of knowledge that we can build upon. At the same time, GDExtension is a unique chance to experiment with some new approaches, without causing a large userbase to grab their pitchforks 😀

We should probably be careful with adding new item-level attributes. They go out of control pretty fast. We've had that in GDNative (godot-rust/gdnative#848) and hopefully not again here.

Yeah, I definitely think it's worth moving some of the suggestions from here into the issue and working on a longer term API design (relinking here for convenience of anyone following along #3).

@Bromeon was there anything else you wanted me to do/add before merging this?

@mhoff12358 Sorry for the delay.

#[derive(GodotClass)] #[class(base=Node)] #[property(name = "val", getter = "get_val", setter = "set_val")] struct HasProperty { val: i32, #[base] base: Base, }

#[godot_api] impl HasProperty { // Does not need to be #[func], correct? fn get_val(&self) -> i32 { return self.val; } fn set_val(&mut self, val: i32) { self.val = val; } }

As mentioned above, what do you think about having the annotation on the property rather than on the class? You would have more locality and not need to repeat "val".

#[derive(GodotClass)] #[class(base=Node)] struct HasProperty { #[property(getter = "get_val", setter = "set_val")] val: i32,

#[base]
base: Base<Node>,

}

Also, we should probably name it #[export] instead of #[property]. The proc-macro API in GDExtension classes is designed after the GDScript keywords:

class -> #[class]
func -> #[func]
func -> #[func]
@export -> #[export]

It could also be #[var], although a var alone in GDScript is a private variable.

@Bromeon Sure, yeah I can do that.

I personally still have a gripe with how Godot does exports/properties, coming from C#. It has you put the export on a member variable, but then you can provide arbitrary setter and getter methods that don't necessarily need to use that member variable for storage. A GDScript that wraps a non-exported array might want a property to set the array's size, but would have to define an extra member variable just to make the size-changing methods available to the editor.

But in this case, yeah, if the goal is to have the API directly mirror GDScript I can see how that matters more. I think my needs can be served by just better documenting whatever method I had previously found that lets you add custom code to the registration and letting the user call the registration methods themselves directly.

I'll probably start with a fresh PR in this case and copy things over. I'll close this once that happens.

...but then you can provide arbitrary setter and getter methods that don't necessarily need to use that member variable for storage. A GDScript that wraps a non-exported array might want a property to set the array's size, but would have to define an extra member variable just to make the size-changing methods available to the editor.

FYI we have Property in godot-rust GDNative for exactly that scenario. I think the intention is to eventually implement something similar for GDExtension as well.

Labels

c: register

feature

Adds functionality to the library