godot-rust
[proposal] add 'notify' to '#[var]'
I've noticed that I have the following pattern, where I need to do some action after one of many vars has been changed:
#[derive(GodotClass)]
#[class(base=Node2D, init, tool)]
pub struct HealthBar {
#[var(get, set = set_filled)]
#[export(range = (0.0, 1.0))]
filled: f32,
#[init(val = Color::GREEN_YELLOW)]
#[var(get, set = set_outline_color)]
#[export]
outline_color: Color,
#[init(val = Color::GREEN)]
#[var(get, set = set_fill_color)]
#[export]
fill_color: Color,
base: Base<Node2D>
}
#[godot_api]
impl HealthBar {
#[func]
pub fn set_filled(&mut self, new_value: f32) {
if self.filled != new_value {
self.filled = new_value;
self.base_mut().queue_redraw();
}
}
#[func]
pub fn set_outline_color(&mut self, new_value: Color) {
if self.outline_color != new_value {
self.outline_color = new_value;
self.base_mut().queue_redraw();
}
}
#[func]
pub fn set_fill_color(&mut self, new_value: Color) {
if self.fill_color != new_value {
self.fill_color = new_value;
self.base_mut().queue_redraw();
}
}
}
That's ... quite verbose.
Instead I'd like to propose the following:
#[derive(GodotClass)]
#[class(base=Node2D, init, tool)]
pub struct HealthBar {
#[var(get, set, notify = redraw)]
#[export(range = (0.0, 1.0))]
filled: f32,
#[init(val = Color::GREEN_YELLOW)]
#[var(get, set, notify = redraw)]
#[export]
outline_color: Color,
#[init(val = Color::GREEN)]
#[var(get, set, notify = redraw)]
#[export]
fill_color: Color,
#[var(get, set, notify = redraw)]
width: f32,
#[var(get, set, notify = redraw)]
height: f32,
base: Base<Node2D>
}
#[godot_api]
impl HealthBar {
fn redraw(&mut self) {
self.base_mut().queue_redraw();
}
}
Here notify is a new option. If it is set, the autogenerated setter looks like this:
pub fn set_a(&mut self, a: <i32 as ::godot::meta::GodotConvert>::Via) {
let prev_value = self.a;
<i32 as ::godot::register::property::Var>::set_property(&mut self.a, a);
if prev_value != self.a {
self.on_change();
}
}
This currently requires that the type of the property implements Eq. Another option would be to unconditionally call the notify fn.
Is this something that makes sense to include in gdext for you? I'm totally fine if you say no and close this PR.
If yes, what do you think about the design, w.r.t. naming and equality?
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-946
There are different ways how I could imagine users to benefit from a notify function, for example:
- a callback invoked on change (like you propose)
- a callback invoked on setting (not sure if common, maybe for things like passwords?)
- a callback invoked when an input operation is complete (press OK button rather than type a new character)
Anecdote: there was an after_set property in gdnative, see e.g. https://github.com/godot-rust/gdnative/issues/715. However it was removed at some point in favor of an actual set, and no one complained 🙂
There's also the question, should the notify callback receive information about a) name of the field, b) type of the value, c) the value itself? Probably not all of them (since that defeats genericity over fields), but I could imagine that someone having 10 bool fields would like to know which one was toggled.
Can we not achieve the same by providing a more flexible callback for set itself?
#[derive(GodotClass)]
#[class(base=Node2D, init, tool)]
pub struct HealthBar {
#[var(get, set = custom_set)]
width: f32,
#[var(get, set = custom_set)]
height: f32,
}
impl HealthBar {
// old way:
fn hardcoded_set(value: f32) {...}
// new possible way:
fn custom_set<T>(value: PropertyUpdate<T>) {
// may have meta-info about property name, variant etc
}
}
I'm not sure if set = itself could be made compatible with something like this (through conversion traits maybe), but otherwise rich_set = or so?
The advantage of such a mechanism as opposed to notify is that it's not limited to one specific use case, but can be generalized to any sort of update logic, with various degrees of input requirements.
That's some good points.
If you have a generalized set as opposed to an after_set, that would imply that actually updating the backing field would still be the obligation of your method. Since it a) should be applicable to multiple fields of b) potentially different types, the only way I could envision this is if the backing field is referenced in PropertyUpdate.
struct PropertyUpdate {
backing_field: &mut T,
new_value: T,
field_name: String
}
then user-code could look like this:
fn custom_set<T>(value: PropertyUpdate<T>) {
// pre-set checks
// actually write to backing field
*value.backing_field = value.new_value;
// post-set actions
if value.field_name == "a" || value.field_name == "b" { /* ... */ }
}
That's pseudo code written outside the editor, I'm not sure how to handle that with regard to lifetimes etc; especially since you very likely want custom_set to also receive &mut self, and I believe that might cause issues with aliasing, as you can reference the field either through self or through value.backing_field.
An after_set would be much easier,
struct PropertyUpdate {
prev_value: T,
new_value: T,
field_name: String
}
fn custom_set<T>(&mut self, value: PropertyUpdate<T>) {
if value.field_name == "a" || value.field_name == "b" { /* ... */ }
}
I'll see if I can spend some time of my life learning more about rust lifetimes
Yes, you're correct that referencing &'a mut T in the struct (where 'a is the lifetime) and simultaneously calling custom_set(&mut self) would violate aliasing rules, since it would allow the function to obtain two exclusive (mut) references to the field.
However, it's possible that PropertyUpdate instead stores the function updating the field (aka the default setter), in the form of fn(&mut C, T) or Box<dyn Fn(&mut C, T)> to allow closures.
Or, even more flexible, it could return the exclusive reference to the field, given an exclusive reference to the struct:
// C = surrounding class, T = field type
struct PropertyUpdate<'a, C, T> {
new_value: T,
field_name: &'a str, // might also be &'a StringName, depending on what's available
get_field_mut: fn(&mut C) -> &mut T,
}
impl<'a, C, T> PropertyUpdate<'a, C, T> {
// Slightly more convenient access + encapsulation.
pub fn default_set(&self, this: &mut C) {
let field: &mut T = (self.get_field_mut)(this);
*field = self.new_value;
// TODO see if we need to move out, or can have &T reference...
}
// For cases where someone modifies the input first (e.g. clamp to a range).
pub fn set(&self, this: &mut C, new_value: T) {
let field: &mut T = (self.get_field_mut)(this);
*field = new_value;
}
}
And then:
fn custom_set<T>(&mut self, update: PropertyUpdate<T>) {
// pre-set checks
// actually write to backing field
update.default_set(self);
// alternatively, if someone changes T first
let corrected_value = clamp(update.value());
update.set(self, corrected_value);
// post-set actions
...
}
An
after_setwould be much easier,
I'd argue that equally common as "notify on changed" (after-set) logic is "validate before update" (before-set) or "adjust before update" (like bringing a value into a valid range).
Having a dedicated callback for each possible workflow may quickly get out of hand. And if someone has a slightly different requirement (e.g. take not just input, but also previous value into account), then the user is again stuck 🙂 so a more general design may be more powerful, even if it means a bit more code for defining such a "rich setter" once.
With your example it can be done like so:
impl HealthBar {
/// Sets attribute, then queue rerender.
fn set_then_queue_rerender<T: PartialEq>(
&mut self,
t: T,
f: impl FnOnce(&mut Self) -> &mut T,
) {
let p = f(self);
if *p != t {
*p = t;
self.base_mut().queue_rerender();
}
}
}
#[godot_api]
impl HealthBar {
// Example
#[func]
pub fn set_filled(&mut self, new_value: f32) {
self.set_then_queue_rerender(new_value, |v| &mut v.filled);
}
}
Not the cleanest, but it does abstract after set behavior.
I'm also curious with custom property. For example, in my code there are "phantom" properties (returns zero value and ignore setter) where it's custom getter/setter is pretty expensive (serialization/deserialization with caching). Will it call getter for every setter?
@0x53A What do you think about my comment? :slightly_smiling_face:
It looks like you added a commit implementing something like this, could you elaborate a bit?
yep it's basically what it looks like - after you demonstrated how to implement it using a function to return the exclusive reference to the field, implementing it was easy. I just kinda forgot to add a comment here after i pushed the commit.
So now the bikeshedding can begin.
(Quick note, I'll clean up the implementation next week.)
I believe we are in agreement that something like custom_set, (named set_ex as a placeholder in the implementation), shall be added
set and set_ex are exclusive, you can obviously only have one.
set can be either of the form set or set=f. set_ex can only be of the form set_ex=f.
Question 1, what should be the user-facing attribute name? #[var(??? = f)]?
I actually kinda like set_ex, and the _ex suffix has precedent.
Question 2, what should be the signature of the user-defined callback function f?
A, a single struct with both a reference to the field and the new value
// defined by gdext
pub struct PropertyUpdate<'a, C, T> {
pub new_value: T,
pub field_name: &'a str,
pub get_field_mut: fn(&mut C) -> &mut T,
}
impl<C, T> PropertyUpdate<'_, C, T> {
pub fn set(self, obj: &mut C) {
*(self.get_field_mut)(obj) = self.new_value;
}
pub fn set_custom(self, obj: &mut C, value: T) {
*(self.get_field_mut)(obj) = value;
}
}
// ------------------
// defined by the user
fn custom_set<T>(&mut self, update: PropertyUpdate<Self, T>) {
// pre-set checks
update.set(self);
// or, alternatively ...
// update.set_custom(some_other_value);
// post-set actions
}
B, a struct with just the reference to the field, and a second argument for the new value
// defined by gdext
pub struct PropertyRef<'a, C, T> {
pub field_name: &'a str,
pub get_field_mut: fn(&mut C) -> &mut T,
}
impl<C, T> PropertyRef<'_, C, T> {
pub fn set(self, obj: &mut C, new_value: T) {
*(self.get_field_mut)(obj) = new_value;
}
}
// ------------------
// defined by the user
fn custom_set<T>(&mut self, field_ref:PropertyRef<Self, T>, new_value: T) {
// pre-set checks
field_ref.set(self, new_value);
// post-set actions
}
I very slightly prefer B, because it does not distinguish between setting the value as passed from godot, or setting a custom value; but I could also count that exact same argument as pro A, because maybe you do want to emphasize whether you're just passing through the value, or modifying it inside the setter.
Question 3, should only set_ex be implemented, or both set_ex to allow implementing arbitrary actions, and notify (to be renamed) to also provide an easier to use shortcut? Because I can immediately see myself making the mistake of implementing set_ex and forgetting to actually set the value.
I'd prefer b, "both", but I can see your argument, "why stop at two".
Question 1, what should be the user-facing attribute name?
#[var(??? = f)]?I actually kinda like
set_ex, and the_exsuffix has precedent.
_ex has precedent but in an entirely different context: as builders for default parameters. For this reason I would not use the same suffix here, as it creates an association that doesn't exist. Godot made this mistake in several places ("exporting", "singleton", ...) which keeps creating confusion to this day.
Not sure if I have a great name though. Some thoughts:
*_setis probably better thanset_*because it emphasizes that this is a different version ofset, rather than setting something.- I was thinking of
shared_setbut don't like that this term is mostly associated with memory management. Also, it doesn't have to be shared, it's perfectly fine to use just for one. - It's hard to find a word that expresses both "the set function is more general/takes more input" and "multiple vars can refer to the same set function". Alongside concept of general(ized), common, unified, reusable, flexible, versatile, adaptive, polymorphic, ...
- Maybe simply
general_setor so?
I'll write a separate answer about the rest, as it's going to be a bit involved...
Question 2, what should be the signature of the user-defined callback function f?
A, a single struct with both a reference to the field and the new value B, a struct with just the reference to the field, and a second argument for the new value
Hm, that is a harder problem than I initially assumed :sweat_smile:
My first instinct was to tend towards (A), because it's easier to get one parameter to have the right type than two:
fn custom_set<T>(&mut self, field_ref: PropertyRef<Self, Vector2>, new_value: Vector2i)
// During refactor, updated one type but not the other -> maybe weird compile error?
// Also less repetition + more integrated functionality in PropertyUpdate.
However, there's one problem: T is passed by value. That means if we want to extract the new_value: T from PropertyUpdate and assign it to the field, we need to either clone or consume the update object (take self).
- Cloning isn't ideal for ref-counted types -- I wouldn't want this
general_setgeneralization to punish performance. - Consuming would mean a user can no longer access the other
PropertyUpdatefields, such asget_field_mut. They would need to be extracted first.
It is technically possible to work with public fields and encourage destructuring:
fn custom_set<T>(&mut self, update: PropertyUpdate<Self, T>) {
let PropertyUpdate { new_value, get_field_mut, .. } = update;
let new_value = adjust(new_value);
*get_field_mut(self) = new_value;
}
// or:
fn custom_set<T>(&mut self, update: PropertyUpdate<Self, T>) {
update.set_with(|new_value| adjust(new_value));
// set_with takes self, i.e. update is no longer accessible after here.
}
Comparison of (A) and (B)
In summary, the trade-offs are:
(B)
- :heavy_check_mark: a bit simpler
- :heavy_check_mark: data flow from parameter into update-object is clearer
- :x: repetition of
Ttype
Possible API:
// assuming no public fields
impl PropertyRef<'a, C, T> {
fn set(&mut self, class: &mut C, new_value: T);
fn get(&self, class: &C) -> T;
fn field_name(&self) -> &'a str;
}
(A)
- :heavy_check_mark: can provide several common patterns (set-unchanged, set-with-map, set-custom) directly on the update-object
- :heavy_check_mark: allows consuming
selfof update-object to express "the value has been written", meaning it prevents accidental 2nd access to the field by design - :x: needs destructuring (or getter calls for individual fields) in more complex cases
Possible API:
// assuming no public fields
impl PropertyRef<'a, C, T> {
fn set(self, class: &mut C, different_value: T);
fn set_unchanged(self, class: &mut C);
fn set_with(self, class: &mut C, update: impl FnOnce(T) -> T);
fn field_name(&self) -> &'a str;
}
// however forcing user code into closures can be annoying/limiting...
Other aspects
One thing we haven't considered yet: in the future I'd like to provide a ZST dummy field like gdnative's Property, which would allow uses to not actually need memory for fields if they store the value elsewhere (e.g. in a HashMap...). Although I think this would still work with both approaches -- get_field_mut would just return a &mut ZstType which is useless but not problematic.
I'm not sure how I feel about public fields; it takes away our freedoms of changing implementation, optimizing things, etc. At least the struct should be #[non_exhaustive] to allow field additions, but maybe getters are better. Which would mean we can't use destructuring in case of (A).
Conclusion
I don't see one approach (A) or (B) that is clearly superior, both have their trade-offs.
But (B) feels slightly simpler as in "less overengineered", and at the same time more flexible. It comes at a cost of a bit more verbosity (repetition of T, no "set-unchanged"), but maybe that's OK if someone already opts-in for #[var(general_set)]?
Opinions?
About the naming, it might be possible to overload set through some terrible hacks, by generating the code for both cases and then letting the compiler/linker remove the dead code, I'll experiment with that over the next days. (That hack can then get removed if/when we get proper compile time introspection.)
Unrelated to this issue, would you be to open to me changing the way set=f works?
Currently the name of f is exported to godot, and if the user forgot to add #[func] (or used rename), it'll just error at runtime (I believe).
I propose the following:
The setter is always generated, with the Godot name set_{fieldname}. The user should not add #[func] to f.
That is, for the user code
pub struct MyNode {
#[var(get, set=set_foo)]
foo: bool,
}
impl MyNode {
fn set_foo(&mut self, new_value: bool) { /* */ }
}
The compiler would generate:
impl MyNode {
#[doc(hidden)]
#[func(rename=set_foo)]
fn __internal_set_foo(&mut self, new_value: bool) {
self.set_foo(new_value);
}
That would also solve https://github.com/godot-rust/gdext/issues/863
Yes, this is a big breaking change, but I believe it is straight better, and it's still early enough to do it.
About the naming, it might be possible to overload
setthrough some terrible hacks, by generating the code for both cases and then letting the compiler/linker remove the dead code, I'll experiment with that over the next days. (That hack can then get removed if/when we get proper compile time introspection.)
I found that adding complexity and brittleness for tiny naming tweaks is rarely worth it -- we tend to overestimate how big of an impact these things have. Especially as long as things are consistent.
In this particular case, having distinct names can even be good for documentation purposes -- one immediately sees that the receiving end isn't a regular setter, but a generalized one. Also, I'd rather have bad naming than extra complexity :wink:
Currently the name of
fis exported to godot, and if the user forgot to add#[func](or usedrename), it'll just error at runtime (I believe).
You're right, it's a runtime error in Godot:
ERROR: Invalid setter 'Class::set_thing' for property 'thing'.
Unrelated to this issue, would you be to open to me changing the way
set=fworks?
Interesting idea! One issue is that now we taught users to put #[func] on their custom getter/setter. If the library now registers an additional method with the same method in Godot, we have a conflict.
That would also solve #863
It solves it by having a second function that's registered with a different name, thus exposing two identical APIs to Godot. I'm not sure if this is intended -- as a user, if I rename my setter, I'd expect to have it renamed and not copied. This would also pollute Godot docs, for example :thinking:
So I think both #863 and avoiding runtime errors (or moving them to compile time) are worthwhile goals, but we should maybe iterate a bit on the way we solve them :slightly_smiling_face:
Edit: see my comment in https://github.com/godot-rust/gdext/issues/863#issuecomment-2495669334 for a potential idea. We can gladly continue discussing there, as I think the two problems are closely related.
I feel like the easiest way to deal with the name problem is to just call it set_callback Sounds good enough to me, since your adding extra logic on top of setting it
I rebased again, but I haven't received any answer to my comment from 1 month ago, although there have been some commits. It's a bit unclear to me if you still plan to pursue this?
If yes, some guidelines:
- Once you think it's ready for review, please move it out of draft state. At that point, CI should pass.
- Make sure you use sentence capitalization for comments, i.e. start with uppercase and end with fullstop (with rare exceptions, in case of just 1-2 keywords).
- When ready, please squash commits -- one per logical change. Often, this can mean just 1 commit in the PR.
If you no longer have time/interest, that's totally fine as well -- but please communicate it, so a decision can be made about moving forward, and someone else can potentially pick up the work. Thanks! :blush:
Hey sorry for the radio silence, I was planning on continuing on it but ended up doing other things.
It’s not yet in a state to be merged, so yes it’s still draft on purpose. First step was/is to define the goal. I also wanted to first solve #863.
I plan on taking another stab at this early next year; if you (or someone else) want to work on it before then I wouldn’t take it personally though.
Thanks for the update! I'm currently reworking functions/signals a bit, so there's a chance that #863 becomes easier with the new system. So let's maybe catch up in January regarding this. Happy holidays :slightly_smiling_face:
So! I thought February is a good opportunity to check back :slightly_smiling_face: do you still plan to pursue this?
Can't believe it's been three months already ...
So! I thought February is a good opportunity to check back 🙂 do you still plan to pursue this?
Well, primarily I wanted to shelve it until #1019 is in, which it now is.
I'll see about rebasing it on top of the latest changes this week.
February is probably gonna be a slow drip of changes from me because there are other things I'm supposed to do, I'll definitely be free(er) in March.
So if this is something you want in fast, you can either take this mess and run with it, or start fresh; if not, I do plan to slowly work on it.
Thanks for the review but it’s way too early - I just hacked something together that works but definitely isn’t pretty
(style-review, that is).
regarding functionality, yes “notify” is implementable by “general_set”, and I’ll back out “notify” for the first revision.
Ok, I'll wait then until you change the status to "Ready for Review" :slightly_smiling_face:
This has now been open for over 4 months, without much progress after my initial reviews. Unfortunately, I currently don't have the capacity to push this over the finish line myself, also since it's not a much-requested feature by the community.
In order to focus open pull requests on actively developed features, I'm going to close this. In case you find the time and motivation to resume this, please let me know and I'll gladly reopen. It's also possible that we pick this up in the future (and would of course co-author you), but for v0.3 there's a lot of other topics. Either way, thanks a lot for the contribution!