From 0e9878b479ea2c6f68d0e9eda4646efd3b2728d1 Mon Sep 17 00:00:00 2001 From: Lukas Rieger Date: Sat, 26 Oct 2024 22:51:40 +0200 Subject: [PATCH 1/8] workflows/full-ci: allow manually triggering the workflow --- .github/workflows/full-ci.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/full-ci.yml b/.github/workflows/full-ci.yml index 14249778e..1796f73d7 100644 --- a/.github/workflows/full-ci.yml +++ b/.github/workflows/full-ci.yml @@ -8,6 +8,8 @@ name: Full CI # Runs before merging. Rebases on master to make sure CI passes for latest integration, not only for the PR at the time of creation. on: + # allow manually triggering the workflow (https://docs.github.com/en/actions/writing-workflows/choosing-when-your-workflow-runs/events-that-trigger-workflows#workflow_dispatch) + workflow_dispatch: merge_group: # push: From f8fbd6d62a394c69cd578c59c242c7ecb302e637 Mon Sep 17 00:00:00 2001 From: Lukas Rieger Date: Thu, 7 Nov 2024 02:28:06 +0100 Subject: [PATCH 2/8] add new option 'notify=callback_fn' to the #[var] attribute it can only be used in conjunction with an autogenerated setter. the callback_fn is a 'fn(&[mut ]self) { }', which will be called whenever the backing value of the var changes. --- .../src/class/data_models/field_var.rs | 53 +++++++++++++++++-- .../src/class/data_models/property.rs | 5 +- itest/rust/src/object_tests/property_test.rs | 37 +++++++++++++ 3 files changed, 90 insertions(+), 5 deletions(-) diff --git a/godot-macros/src/class/data_models/field_var.rs b/godot-macros/src/class/data_models/field_var.rs index 267ae0de9..74918a500 100644 --- a/godot-macros/src/class/data_models/field_var.rs +++ b/godot-macros/src/class/data_models/field_var.rs @@ -12,7 +12,7 @@ use crate::class::{ into_signature_info, make_existence_check, make_method_registration, Field, FieldHint, FuncDefinition, }; -use crate::util::KvParser; +use crate::util::{bail, KvParser}; use crate::{util, ParseResult}; /// Store info from `#[var]` attribute. @@ -20,10 +20,31 @@ use crate::{util, ParseResult}; pub struct FieldVar { pub getter: GetterSetter, pub setter: GetterSetter, + pub notify: Option, pub hint: FieldHint, pub usage_flags: UsageFlags, } +fn parse_notify(parser: &mut KvParser, key: &str) -> ParseResult> { + match parser.handle_any(key) { + // No `notify` argument + None => Ok(None), + Some(value) => match value { + // `notify` without value is an error + None => { + bail!(parser.span(), "The correct syntax is 'notify = callback_fn'") + }, + // `notify = expr` + Some(value) => { + match value.ident() { + Ok(ident) => Ok(Some(ident)), + Err(_) => bail!(parser.span(), "The correct syntax is 'notify = callback_fn'") + } + } + } + } +} + impl FieldVar { /// Parse a `#[var]` attribute to a `FieldVar` struct. /// @@ -36,12 +57,17 @@ impl FieldVar { pub(crate) fn new_from_kv(parser: &mut KvParser) -> ParseResult { let mut getter = GetterSetter::parse(parser, "get")?; let mut setter = GetterSetter::parse(parser, "set")?; + let mut notify = parse_notify(parser, "notify")?; if getter.is_omitted() && setter.is_omitted() { getter = GetterSetter::Generated; setter = GetterSetter::Generated; } + if notify.is_some() && !setter.is_generated() { + return bail!(parser.span(), "When using 'notify', the property must also use an autogenerated 'set'"); + } + let hint = parser.handle_ident("hint")?; let hint = if let Some(hint) = hint { @@ -69,6 +95,7 @@ impl FieldVar { Ok(FieldVar { getter, setter, + notify, hint, usage_flags, }) @@ -112,12 +139,13 @@ impl GetterSetter { &self, class_name: &Ident, kind: GetSet, + notify: Option, field: &Field, ) -> Option { match self { GetterSetter::Omitted => None, GetterSetter::Generated => Some(GetterSetterImpl::from_generated_impl( - class_name, kind, field, + class_name, kind, notify, field, )), GetterSetter::Custom(function_name) => { Some(GetterSetterImpl::from_custom_impl(function_name)) @@ -128,6 +156,10 @@ impl GetterSetter { pub fn is_omitted(&self) -> bool { matches!(self, GetterSetter::Omitted) } + + pub fn is_generated(&self) -> bool { + matches!(self, GetterSetter::Generated) + } } /// Used to determine whether a [`GetterSetter`] is supposed to be a getter or setter. @@ -154,7 +186,7 @@ pub struct GetterSetterImpl { } impl GetterSetterImpl { - fn from_generated_impl(class_name: &Ident, kind: GetSet, field: &Field) -> Self { + fn from_generated_impl(class_name: &Ident, kind: GetSet, notify: Option, field: &Field) -> Self { let Field { name: field_name, ty: field_type, @@ -179,8 +211,23 @@ impl GetterSetterImpl { signature = quote! { fn #function_name(&mut self, #field_name: <#field_type as ::godot::meta::GodotConvert>::Via) }; + + let notify_block = match notify { + Some(ident) => { + quote! { + if self.#field_name != #field_name { + self.#ident(); + } + } + }, + None => { + quote! { } + } + }; + function_body = quote! { <#field_type as ::godot::register::property::Var>::set_property(&mut self.#field_name, #field_name); + #notify_block }; } } diff --git a/godot-macros/src/class/data_models/property.rs b/godot-macros/src/class/data_models/property.rs index cae0defc9..15839984b 100644 --- a/godot-macros/src/class/data_models/property.rs +++ b/godot-macros/src/class/data_models/property.rs @@ -68,6 +68,7 @@ pub fn make_property_impl(class_name: &Ident, fields: &Fields) -> TokenStream { let FieldVar { getter, setter, + notify, hint, mut usage_flags, } = var; @@ -134,12 +135,12 @@ pub fn make_property_impl(class_name: &Ident, fields: &Fields) -> TokenStream { }; let getter_name = make_getter_setter( - getter.to_impl(class_name, GetSet::Get, field), + getter.to_impl(class_name, GetSet::Get, None, field), &mut getter_setter_impls, &mut export_tokens, ); let setter_name = make_getter_setter( - setter.to_impl(class_name, GetSet::Set, field), + setter.to_impl(class_name, GetSet::Set, notify, field), &mut getter_setter_impls, &mut export_tokens, ); diff --git a/itest/rust/src/object_tests/property_test.rs b/itest/rust/src/object_tests/property_test.rs index a8d5fd2ad..fcc8e46e6 100644 --- a/itest/rust/src/object_tests/property_test.rs +++ b/itest/rust/src/object_tests/property_test.rs @@ -450,3 +450,40 @@ fn override_export() { fn check_property(property: &Dictionary, key: &str, expected: impl ToGodot) { assert_eq!(property.get_or_nil(key), expected.to_variant()); } + + +// --------------------------------------------------------------- + +#[derive(GodotClass)] +#[class(base=Node, init)] +struct NotifyTest { + #[var(notify = on_change)] + a: i32, + #[var(notify = on_change)] + b: i32, + + pub call_count: u32 +} + +impl NotifyTest { + fn on_change(&mut self) { + self.call_count += 1; + } +} + +#[itest] +fn test_var_notify() { + let mut class = NotifyTest::new_alloc(); + + assert_eq!(class.bind().call_count, 0); + + class.call("set_a", &[3.to_variant()]); + assert_eq!(class.bind().a, 3); + assert_eq!(class.bind().call_count, 1); + + + class.call("set_b", &[5.to_variant()]); + assert_eq!(class.bind().a, 5); + assert_eq!(class.bind().call_count, 2); +} + From d72e5e226dde7004d9532f564aa83333bc5d875a Mon Sep 17 00:00:00 2001 From: Lukas Rieger Date: Thu, 7 Nov 2024 02:50:14 +0100 Subject: [PATCH 3/8] (try to) fix setter --- .../src/class/data_models/field_var.rs | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/godot-macros/src/class/data_models/field_var.rs b/godot-macros/src/class/data_models/field_var.rs index 74918a500..14d4f2728 100644 --- a/godot-macros/src/class/data_models/field_var.rs +++ b/godot-macros/src/class/data_models/field_var.rs @@ -212,22 +212,21 @@ impl GetterSetterImpl { fn #function_name(&mut self, #field_name: <#field_type as ::godot::meta::GodotConvert>::Via) }; - let notify_block = match notify { + let function_body_set = quote! { + <#field_type as ::godot::register::property::Var>::set_property(&mut self.#field_name, #field_name); + }; + + function_body = match notify { Some(ident) => { quote! { - if self.#field_name != #field_name { + let prev_value = self.#field_name; + #function_body_set + if prev_value != self.#field_name { self.#ident(); } } }, - None => { - quote! { } - } - }; - - function_body = quote! { - <#field_type as ::godot::register::property::Var>::set_property(&mut self.#field_name, #field_name); - #notify_block + None => function_body_set }; } } From 8b8130c275c8a852506b603b593707691882bd83 Mon Sep 17 00:00:00 2001 From: Lukas Rieger Date: Thu, 7 Nov 2024 02:54:54 +0100 Subject: [PATCH 4/8] fix test --- itest/rust/src/object_tests/property_test.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/itest/rust/src/object_tests/property_test.rs b/itest/rust/src/object_tests/property_test.rs index fcc8e46e6..444927a27 100644 --- a/itest/rust/src/object_tests/property_test.rs +++ b/itest/rust/src/object_tests/property_test.rs @@ -483,7 +483,7 @@ fn test_var_notify() { class.call("set_b", &[5.to_variant()]); - assert_eq!(class.bind().a, 5); + assert_eq!(class.bind().b, 5); assert_eq!(class.bind().call_count, 2); } From 1b0a29fdcb43462c3a780931e40104d5772ca74d Mon Sep 17 00:00:00 2001 From: Lukas Rieger Date: Thu, 7 Nov 2024 02:55:36 +0100 Subject: [PATCH 5/8] fix clippy --- godot-macros/src/class/data_models/field_var.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/godot-macros/src/class/data_models/field_var.rs b/godot-macros/src/class/data_models/field_var.rs index 14d4f2728..77c534243 100644 --- a/godot-macros/src/class/data_models/field_var.rs +++ b/godot-macros/src/class/data_models/field_var.rs @@ -57,7 +57,7 @@ impl FieldVar { pub(crate) fn new_from_kv(parser: &mut KvParser) -> ParseResult { let mut getter = GetterSetter::parse(parser, "get")?; let mut setter = GetterSetter::parse(parser, "set")?; - let mut notify = parse_notify(parser, "notify")?; + let notify = parse_notify(parser, "notify")?; if getter.is_omitted() && setter.is_omitted() { getter = GetterSetter::Generated; From d22cfeb9607013ad3a9842bbc9d438ca42ebfafb Mon Sep 17 00:00:00 2001 From: Lukas Rieger Date: Thu, 7 Nov 2024 02:56:47 +0100 Subject: [PATCH 6/8] cargo fmt --- .../src/class/data_models/field_var.rs | 38 ++++++++++++------- itest/rust/src/object_tests/property_test.rs | 5 +-- 2 files changed, 26 insertions(+), 17 deletions(-) diff --git a/godot-macros/src/class/data_models/field_var.rs b/godot-macros/src/class/data_models/field_var.rs index 77c534243..de403ecc5 100644 --- a/godot-macros/src/class/data_models/field_var.rs +++ b/godot-macros/src/class/data_models/field_var.rs @@ -32,16 +32,20 @@ fn parse_notify(parser: &mut KvParser, key: &str) -> ParseResult> Some(value) => match value { // `notify` without value is an error None => { - bail!(parser.span(), "The correct syntax is 'notify = callback_fn'") - }, - // `notify = expr` - Some(value) => { - match value.ident() { - Ok(ident) => Ok(Some(ident)), - Err(_) => bail!(parser.span(), "The correct syntax is 'notify = callback_fn'") - } + bail!( + parser.span(), + "The correct syntax is 'notify = callback_fn'" + ) } - } + // `notify = expr` + Some(value) => match value.ident() { + Ok(ident) => Ok(Some(ident)), + Err(_) => bail!( + parser.span(), + "The correct syntax is 'notify = callback_fn'" + ), + }, + }, } } @@ -65,7 +69,10 @@ impl FieldVar { } if notify.is_some() && !setter.is_generated() { - return bail!(parser.span(), "When using 'notify', the property must also use an autogenerated 'set'"); + return bail!( + parser.span(), + "When using 'notify', the property must also use an autogenerated 'set'" + ); } let hint = parser.handle_ident("hint")?; @@ -186,7 +193,12 @@ pub struct GetterSetterImpl { } impl GetterSetterImpl { - fn from_generated_impl(class_name: &Ident, kind: GetSet, notify: Option, field: &Field) -> Self { + fn from_generated_impl( + class_name: &Ident, + kind: GetSet, + notify: Option, + field: &Field, + ) -> Self { let Field { name: field_name, ty: field_type, @@ -225,8 +237,8 @@ impl GetterSetterImpl { self.#ident(); } } - }, - None => function_body_set + } + None => function_body_set, }; } } diff --git a/itest/rust/src/object_tests/property_test.rs b/itest/rust/src/object_tests/property_test.rs index 444927a27..53e990a1c 100644 --- a/itest/rust/src/object_tests/property_test.rs +++ b/itest/rust/src/object_tests/property_test.rs @@ -451,7 +451,6 @@ fn check_property(property: &Dictionary, key: &str, expected: impl ToGodot) { assert_eq!(property.get_or_nil(key), expected.to_variant()); } - // --------------------------------------------------------------- #[derive(GodotClass)] @@ -462,7 +461,7 @@ struct NotifyTest { #[var(notify = on_change)] b: i32, - pub call_count: u32 + pub call_count: u32, } impl NotifyTest { @@ -481,9 +480,7 @@ fn test_var_notify() { assert_eq!(class.bind().a, 3); assert_eq!(class.bind().call_count, 1); - class.call("set_b", &[5.to_variant()]); assert_eq!(class.bind().b, 5); assert_eq!(class.bind().call_count, 2); } - From e1118aa2aa20a8ce0fb0c4c8c7a7eba43b271b9b Mon Sep 17 00:00:00 2001 From: Lukas Rieger Date: Thu, 7 Nov 2024 03:00:11 +0100 Subject: [PATCH 7/8] itest: class.free(); --- itest/rust/src/object_tests/property_test.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/itest/rust/src/object_tests/property_test.rs b/itest/rust/src/object_tests/property_test.rs index 53e990a1c..545bc58db 100644 --- a/itest/rust/src/object_tests/property_test.rs +++ b/itest/rust/src/object_tests/property_test.rs @@ -483,4 +483,6 @@ fn test_var_notify() { class.call("set_b", &[5.to_variant()]); assert_eq!(class.bind().b, 5); assert_eq!(class.bind().call_count, 2); + + class.free(); } From 3c11f733bf6cc34c33f6d763257fa600aed9979b Mon Sep 17 00:00:00 2001 From: Lukas Rieger Date: Fri, 8 Nov 2024 12:32:26 +0100 Subject: [PATCH 8/8] #[var]: add 'set_ex' --- godot-core/src/meta/mod.rs | 2 + godot-core/src/meta/property_update.rs | 14 ++++ .../src/class/data_models/field_var.rs | 83 ++++++++++++++++++- .../src/class/data_models/property.rs | 8 +- itest/rust/src/object_tests/property_test.rs | 43 ++++++++++ 5 files changed, 145 insertions(+), 5 deletions(-) create mode 100644 godot-core/src/meta/property_update.rs diff --git a/godot-core/src/meta/mod.rs b/godot-core/src/meta/mod.rs index 708d5e3d9..66dcbed85 100644 --- a/godot-core/src/meta/mod.rs +++ b/godot-core/src/meta/mod.rs @@ -50,10 +50,12 @@ mod signature; mod traits; pub mod error; +pub mod property_update; pub use as_arg::*; pub use class_name::ClassName; pub use godot_convert::{FromGodot, GodotConvert, ToGodot}; +pub use property_update::PropertyUpdate; #[cfg(feature = "codegen-full")] pub use rpc_config::RpcConfig; pub use traits::{ArrayElement, GodotType, PackedArrayElement}; diff --git a/godot-core/src/meta/property_update.rs b/godot-core/src/meta/property_update.rs new file mode 100644 index 000000000..b037f1abd --- /dev/null +++ b/godot-core/src/meta/property_update.rs @@ -0,0 +1,14 @@ +pub struct PropertyUpdate<'a, C, T> { + pub new_value: T, + pub field_name: &'a str, // might also be &'a StringName, depending on what's available + pub get_field_mut: fn(&mut C) -> &mut T, +} + +impl 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; + } +} diff --git a/godot-macros/src/class/data_models/field_var.rs b/godot-macros/src/class/data_models/field_var.rs index de403ecc5..a099b5efa 100644 --- a/godot-macros/src/class/data_models/field_var.rs +++ b/godot-macros/src/class/data_models/field_var.rs @@ -49,6 +49,30 @@ fn parse_notify(parser: &mut KvParser, key: &str) -> ParseResult> } } +fn parse_setter_ex(parser: &mut KvParser, key: &str) -> ParseResult> { + match parser.handle_any(key) { + // No `notify` argument + None => Ok(None), + Some(value) => match value { + // `notify` without value is an error + None => { + bail!( + parser.span(), + "The correct syntax is 'setter_ex = callback_fn'" + ) + } + // `notify = expr` + Some(value) => match value.ident() { + Ok(ident) => Ok(Some(ident)), + Err(_) => bail!( + parser.span(), + "The correct syntax is 'setter_ex = callback_fn'" + ), + }, + }, + } +} + impl FieldVar { /// Parse a `#[var]` attribute to a `FieldVar` struct. /// @@ -62,8 +86,9 @@ impl FieldVar { let mut getter = GetterSetter::parse(parser, "get")?; let mut setter = GetterSetter::parse(parser, "set")?; let notify = parse_notify(parser, "notify")?; + let setter_ex = parse_setter_ex(parser, "set_ex")?; - if getter.is_omitted() && setter.is_omitted() { + if getter.is_omitted() && setter.is_omitted() && setter_ex.is_none() { getter = GetterSetter::Generated; setter = GetterSetter::Generated; } @@ -75,6 +100,17 @@ impl FieldVar { ); } + if setter_ex.is_some() && !setter.is_omitted() { + return bail!( + parser.span(), + "You may not use 'set' and 'set_ex' at the same time, remove one" + ); + } + + if let Some(ident) = setter_ex { + setter = GetterSetter::Ex(ident); + } + let hint = parser.handle_ident("hint")?; let hint = if let Some(hint) = hint { @@ -120,6 +156,9 @@ pub enum GetterSetter { /// Getter/setter is handwritten by the user, and here is its identifier. Custom(Ident), + + /// only applicable to setter. A generic setter that takes 'PropertyUpdate' is handwritten by the user + Ex(Ident), } impl GetterSetter { @@ -157,6 +196,12 @@ impl GetterSetter { GetterSetter::Custom(function_name) => { Some(GetterSetterImpl::from_custom_impl(function_name)) } + GetterSetter::Ex(function_name) => { + assert!(matches!(kind, GetSet::SetEx(_))); + Some(GetterSetterImpl::from_generated_impl( + class_name, kind, notify, field, + )) + } } } @@ -170,17 +215,18 @@ impl GetterSetter { } /// Used to determine whether a [`GetterSetter`] is supposed to be a getter or setter. -#[derive(Copy, Clone, Eq, PartialEq, Debug)] +#[derive(Clone, Eq, PartialEq, Debug)] pub enum GetSet { Get, Set, + SetEx(Ident), } impl GetSet { pub fn prefix(&self) -> &'static str { match self { GetSet::Get => "get_", - GetSet::Set => "set_", + GetSet::Set | GetSet::SetEx(_) => "set_", } } } @@ -228,6 +274,37 @@ impl GetterSetterImpl { <#field_type as ::godot::register::property::Var>::set_property(&mut self.#field_name, #field_name); }; + function_body = match notify { + Some(ident) => { + quote! { + let prev_value = self.#field_name; + #function_body_set + if prev_value != self.#field_name { + self.#ident(); + } + } + } + None => function_body_set, + }; + } + GetSet::SetEx(callback_fn_ident) => { + signature = quote! { + fn #function_name(&mut self, #field_name: <#field_type as ::godot::meta::GodotConvert>::Via) + }; + + let field_name_string_constant = field_name.to_string(); + + let function_body_set = quote! { + + let new_value = ::godot::meta::FromGodot::from_godot(#field_name); + let property_update = ::godot::meta::PropertyUpdate { + new_value: new_value, + field_name: #field_name_string_constant, + get_field_mut: |c: &mut #class_name|&mut c.#field_name + }; + self.#callback_fn_ident(property_update); + }; + function_body = match notify { Some(ident) => { quote! { diff --git a/godot-macros/src/class/data_models/property.rs b/godot-macros/src/class/data_models/property.rs index 15839984b..029248ff2 100644 --- a/godot-macros/src/class/data_models/property.rs +++ b/godot-macros/src/class/data_models/property.rs @@ -7,7 +7,7 @@ //! Parsing the `var` and `export` attributes on fields. -use crate::class::{Field, FieldVar, Fields, GetSet, GetterSetterImpl, UsageFlags}; +use crate::class::{Field, FieldVar, Fields, GetSet, GetterSetter, GetterSetterImpl, UsageFlags}; use proc_macro2::{Ident, TokenStream}; use quote::quote; @@ -139,8 +139,12 @@ pub fn make_property_impl(class_name: &Ident, fields: &Fields) -> TokenStream { &mut getter_setter_impls, &mut export_tokens, ); + let setter_kind = match &setter { + GetterSetter::Ex(ident) => GetSet::SetEx(ident.clone()), + _ => GetSet::Set, + }; let setter_name = make_getter_setter( - setter.to_impl(class_name, GetSet::Set, notify, field), + setter.to_impl(class_name, setter_kind, notify, field), &mut getter_setter_impls, &mut export_tokens, ); diff --git a/itest/rust/src/object_tests/property_test.rs b/itest/rust/src/object_tests/property_test.rs index 545bc58db..ebdac8e96 100644 --- a/itest/rust/src/object_tests/property_test.rs +++ b/itest/rust/src/object_tests/property_test.rs @@ -486,3 +486,46 @@ fn test_var_notify() { class.free(); } + +// --------------------------------------------------------------- + +#[derive(GodotClass)] +#[class(base=Node, init)] +struct SetExTest { + #[var(set_ex = custom_set)] + a: i32, + #[var(set_ex = custom_set)] + b: i32, + + pub call_count: u32, +} + +impl SetExTest { + fn custom_set(&mut self, update: godot::meta::property_update::PropertyUpdate) { + // pre-set checks + + update.set(self); + + // post-set actions + self.call_count += 1; + } +} + +#[itest] +fn test_var_set_ex() { + let mut class = NotifyTest::new_alloc(); + + assert_eq!(class.bind().call_count, 0); + + class.call("set_a", &[3.to_variant()]); + assert_eq!(class.bind().a, 3); + assert_eq!(class.bind().call_count, 1); + + class.call("set_b", &[5.to_variant()]); + assert_eq!(class.bind().b, 5); + assert_eq!(class.bind().call_count, 2); + + class.free(); +} + +// ---------------------------------------------------------------