From 797c641efbae85ffec9cbdb0c06c2ee8888e9e37 Mon Sep 17 00:00:00 2001 From: Simon Leiner Date: Sat, 12 Nov 2022 17:31:33 +0100 Subject: [PATCH 1/3] Add tests for current behaviour of #[derive(Params)] --- Cargo.lock | 1 + nih_plug_derive/Cargo.toml | 3 + nih_plug_derive/tests/params.rs | 116 ++++++++++++++++++++++++++++++++ 3 files changed, 120 insertions(+) create mode 100644 nih_plug_derive/tests/params.rs diff --git a/Cargo.lock b/Cargo.lock index 35ec9728..089a934a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2640,6 +2640,7 @@ source = "git+https://github.com/robbert-vdh/nih_plug_assets.git#a04e327923e120b name = "nih_plug_derive" version = "0.1.0" dependencies = [ + "nih_plug", "quote", "syn", ] diff --git a/nih_plug_derive/Cargo.toml b/nih_plug_derive/Cargo.toml index e43ad305..e8374869 100644 --- a/nih_plug_derive/Cargo.toml +++ b/nih_plug_derive/Cargo.toml @@ -11,3 +11,6 @@ proc-macro = true [dependencies] syn = { version = "1.0", features = ["extra-traits"] } quote = "1.0" + +[dev-dependencies] +nih_plug = { path = ".." } diff --git a/nih_plug_derive/tests/params.rs b/nih_plug_derive/tests/params.rs new file mode 100644 index 00000000..22d8755e --- /dev/null +++ b/nih_plug_derive/tests/params.rs @@ -0,0 +1,116 @@ +use nih_plug::prelude::*; + +#[derive(Params)] +struct FlatParams { + #[id = "one"] + pub one: BoolParam, + + #[id = "two"] + pub two: FloatParam, + + #[id = "three"] + pub three: IntParam, +} + +impl Default for FlatParams { + fn default() -> Self { + FlatParams { + one: BoolParam::new("one", true), + two: FloatParam::new("two", 0.0, FloatRange::Linear { min: 0.0, max: 1.0 }), + three: IntParam::new("three", 0, IntRange::Linear { min: 0, max: 100 }), + } + } +} + +#[derive(Params)] +struct GroupedParams { + #[id = "one"] + pub one: BoolParam, + + #[nested(group = "Some Group", id_prefix = "group1")] + pub group1: FlatParams, + + #[id = "three"] + pub three: IntParam, + + #[nested(group = "Another Group", id_prefix = "group2")] + pub group2: FlatParams, +} + +impl Default for GroupedParams { + fn default() -> Self { + GroupedParams { + one: BoolParam::new("one", true), + group1: FlatParams::default(), + three: IntParam::new("three", 0, IntRange::Linear { min: 0, max: 100 }), + group2: FlatParams::default(), + } + } +} + +#[derive(Params)] +struct NestedParams { + #[id = "one"] + pub one: BoolParam, + + #[nested(id_prefix = "two")] + pub two: FlatParams, + + #[id = "three"] + pub three: IntParam, +} + +impl Default for NestedParams { + fn default() -> Self { + NestedParams { + one: BoolParam::new("one", true), + two: FlatParams::default(), + three: IntParam::new("three", 0, IntRange::Linear { min: 0, max: 100 }), + } + } +} + +mod param_order { + use super::*; + #[test] + fn flat() { + let p = FlatParams::default(); + + // Parameters must have the same order as they are defined in + let param_ids: Vec = p.param_map().into_iter().map(|(id, _, _)| id).collect(); + assert_eq!(param_ids, ["one", "two", "three",]); + } + + #[test] + fn grouped() { + let p = GroupedParams::default(); + + // Parameters must have the same order as they are defined in. Groups are put in the end though. + let param_ids: Vec = p.param_map().into_iter().map(|(id, _, _)| id).collect(); + assert_eq!( + param_ids, + [ + "one", + "three", + "group1_one", + "group1_two", + "group1_three", + "group2_one", + "group2_two", + "group2_three", + ] + ); + } + + #[test] + fn nested() { + let p = NestedParams::default(); + + // Parameters must have the same order as they are defined in. Nested parameters are put in the end though. + let param_ids: Vec = p.param_map().into_iter().map(|(id, _, _)| id).collect(); + assert_eq!( + param_ids, + ["one", "three", "two_one", "two_two", "two_three"] + ); + } +} From d9797a606e3f7520146bd0b608fe54a37617a143 Mon Sep 17 00:00:00 2001 From: Simon Leiner Date: Sat, 12 Nov 2022 17:34:42 +0100 Subject: [PATCH 2/3] Move handling of NestedParams into a separate function --- Cargo.lock | 1 + nih_plug_derive/Cargo.toml | 1 + nih_plug_derive/src/params.rs | 169 ++++++++++++++++++---------------- 3 files changed, 93 insertions(+), 78 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 089a934a..5c676621 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2641,6 +2641,7 @@ name = "nih_plug_derive" version = "0.1.0" dependencies = [ "nih_plug", + "proc-macro2", "quote", "syn", ] diff --git a/nih_plug_derive/Cargo.toml b/nih_plug_derive/Cargo.toml index e8374869..746c6b65 100644 --- a/nih_plug_derive/Cargo.toml +++ b/nih_plug_derive/Cargo.toml @@ -11,6 +11,7 @@ proc-macro = true [dependencies] syn = { version = "1.0", features = ["extra-traits"] } quote = "1.0" +proc-macro2 = "1.0" [dev-dependencies] nih_plug = { path = ".." } diff --git a/nih_plug_derive/src/params.rs b/nih_plug_derive/src/params.rs index 10786a62..960f2eb4 100644 --- a/nih_plug_derive/src/params.rs +++ b/nih_plug_derive/src/params.rs @@ -252,83 +252,7 @@ pub fn derive_params(input: TokenStream) -> TokenStream { |Param {field, id}| quote! { (String::from(#id), self.#field.as_ptr(), String::new()) }, ); - // How nested parameters are handled depends on the `NestedParams` variant. - // These are pairs of `(parameter_id, param_ptr, param_group)`. The specific - // parameter types know how to convert themselves into the correct ParamPtr variant. - // Top-level parameters have no group, and we'll prefix the group name specified in - // the `#[nested(...)]` attribute to fields coming from nested groups - let param_mapping_nested_tokens = nested_params.iter().map(|nested| match nested { - // TODO: No idea how to splice this as an `Option<&str>`, so this involves some - // copy-pasting - NestedParams::Inline { field, group: Some(group) } => quote! { - param_map.extend(self.#field.param_map().into_iter().map(|(param_id, param_ptr, nested_group_name)| { - if nested_group_name.is_empty() { - (param_id, param_ptr, String::from(#group)) - } else { - (param_id, param_ptr, format!("{}/{}", #group, nested_group_name)) - } - })); - }, - NestedParams::Inline { field, group: None } => quote! { - param_map.extend(self.#field.param_map()); - }, - NestedParams::Prefixed { - field, - id_prefix, - group: Some(group), - } => quote! { - param_map.extend(self.#field.param_map().into_iter().map(|(param_id, param_ptr, nested_group_name)| { - let param_id = format!("{}_{}", #id_prefix, param_id); - - if nested_group_name.is_empty() { - (param_id, param_ptr, String::from(#group)) - } else { - (param_id, param_ptr, format!("{}/{}", #group, nested_group_name)) - } - })); - }, - NestedParams::Prefixed { - field, - id_prefix, - group: None, - } => quote! { - param_map.extend(self.#field.param_map().into_iter().map(|(param_id, param_ptr, nested_group_name)| { - let param_id = format!("{}_{}", #id_prefix, param_id); - - (param_id, param_ptr, nested_group_name) - })); - }, - // We'll start at index 1 for display purposes. Both the group and the parameter ID get - // a suffix matching the array index. - NestedParams::Array { field, group: Some(group) } => quote! { - param_map.extend(self.#field.iter().enumerate().flat_map(|(idx, params)| { - let idx = idx + 1; - - params.param_map().into_iter().map(move |(param_id, param_ptr, nested_group_name)| { - let param_id = format!("{}_{}", param_id, idx); - let group = format!("{} {}", #group, idx); - - // Note that this is different from the other variants - if nested_group_name.is_empty() { - (param_id, param_ptr, group) - } else { - (param_id, param_ptr, format!("{}/{}", group, nested_group_name)) - } - }) - })); - }, - NestedParams::Array { field, group: None } => quote! { - param_map.extend(self.#field.iter().enumerate().flat_map(|(idx, params)| { - let idx = idx + 1; - - params.param_map().into_iter().map(move |(param_id, param_ptr, nested_group_name)| { - let param_id = format!("{}_{}", param_id, idx); - - (param_id, param_ptr, nested_group_name) - }) - })); - }, - }); + let param_mapping_nested_tokens = nested_params.iter().map(|p| p.to_token()); quote! { // This may not be in scope otherwise, used to call .as_ptr() @@ -337,7 +261,7 @@ pub fn derive_params(input: TokenStream) -> TokenStream { #[allow(unused_mut)] let mut param_map = vec![#(#param_mapping_self_tokens),*]; - #(#param_mapping_nested_tokens);* + #(param_map.extend(#param_mapping_nested_tokens); )* param_map } @@ -508,3 +432,92 @@ enum NestedParams { group: Option, }, } + +impl NestedParams { + fn to_token(&self) -> proc_macro2::TokenStream { + // How nested parameters are handled depends on the `NestedParams` variant. + // These are pairs of `(parameter_id, param_ptr, param_group)`. The specific + // parameter types know how to convert themselves into the correct ParamPtr variant. + // Top-level parameters have no group, and we'll prefix the group name specified in + // the `#[nested(...)]` attribute to fields coming from nested groups + + match self { + // TODO: No idea how to splice this as an `Option<&str>`, so this involves some + // copy-pasting + NestedParams::Inline { + field, + group: Some(group), + } => quote! { + self.#field.param_map().into_iter().map(|(param_id, param_ptr, nested_group_name)| { + if nested_group_name.is_empty() { + (param_id, param_ptr, String::from(#group)) + } else { + (param_id, param_ptr, format!("{}/{}", #group, nested_group_name)) + } + }) + }, + NestedParams::Inline { field, group: None } => quote! { + self.#field.param_map(); + }, + NestedParams::Prefixed { + field, + id_prefix, + group: Some(group), + } => quote! { + self.#field.param_map().into_iter().map(|(param_id, param_ptr, nested_group_name)| { + let param_id = format!("{}_{}", #id_prefix, param_id); + + if nested_group_name.is_empty() { + (param_id, param_ptr, String::from(#group)) + } else { + (param_id, param_ptr, format!("{}/{}", #group, nested_group_name)) + } + }) + }, + NestedParams::Prefixed { + field, + id_prefix, + group: None, + } => quote! { + self.#field.param_map().into_iter().map(|(param_id, param_ptr, nested_group_name)| { + let param_id = format!("{}_{}", #id_prefix, param_id); + + (param_id, param_ptr, nested_group_name) + }) + }, + // We'll start at index 1 for display purposes. Both the group and the parameter ID get + // a suffix matching the array index. + NestedParams::Array { + field, + group: Some(group), + } => quote! { + self.#field.iter().enumerate().flat_map(|(idx, params)| { + let idx = idx + 1; + + params.param_map().into_iter().map(move |(param_id, param_ptr, nested_group_name)| { + let param_id = format!("{}_{}", param_id, idx); + let group = format!("{} {}", #group, idx); + + // Note that this is different from the other variants + if nested_group_name.is_empty() { + (param_id, param_ptr, group) + } else { + (param_id, param_ptr, format!("{}/{}", group, nested_group_name)) + } + }) + }) + }, + NestedParams::Array { field, group: None } => quote! { + self.#field.iter().enumerate().flat_map(|(idx, params)| { + let idx = idx + 1; + + params.param_map().into_iter().map(move |(param_id, param_ptr, nested_group_name)| { + let param_id = format!("{}_{}", param_id, idx); + + (param_id, param_ptr, nested_group_name) + }) + }) + }, + } + } +} From 4affa40244bccdea1de64d5049a17a3eb0d7d5a5 Mon Sep 17 00:00:00 2001 From: Simon Leiner Date: Sat, 12 Nov 2022 17:48:21 +0100 Subject: [PATCH 3/3] Preserve the order of non-grouped nested parameters --- nih_plug_derive/src/params.rs | 77 +++++++++++++++++++++++++-------- nih_plug_derive/tests/params.rs | 5 ++- 2 files changed, 63 insertions(+), 19 deletions(-) diff --git a/nih_plug_derive/src/params.rs b/nih_plug_derive/src/params.rs index 960f2eb4..e11c5c7a 100644 --- a/nih_plug_derive/src/params.rs +++ b/nih_plug_derive/src/params.rs @@ -31,9 +31,9 @@ pub fn derive_params(input: TokenStream) -> TokenStream { // keys at compile time. // TODO: This duplication check doesn't work for nested fields since we don't know anything // about the fields on the nested structs - let mut params: Vec = Vec::new(); + let mut params: Vec = Vec::new(); let mut persistent_fields: Vec = Vec::new(); - let mut nested_params: Vec = Vec::new(); + let mut group_params: Vec = Vec::new(); for field in fields.named { let field_name = match &field.ident { Some(ident) => ident, @@ -62,7 +62,10 @@ pub fn derive_params(input: TokenStream) -> TokenStream { // This is a vector since we want to preserve the order. If structs get // large enough to the point where a linear search starts being expensive, // then the plugin should probably start splitting up their parameters. - if params.iter().any(|p| p.id == s) { + if params.iter().any(|p| match p { + AnyParam::Single(param) => param.id == s, + _ => false, + }) { return syn::Error::new( field.span(), "Multiple parameters with the same ID found", @@ -71,10 +74,10 @@ pub fn derive_params(input: TokenStream) -> TokenStream { .into(); } - params.push(Param { + params.push(AnyParam::Single(Param { id: s, field: field_name.clone(), - }); + })); processed_attribute = true; } @@ -204,7 +207,7 @@ pub fn derive_params(input: TokenStream) -> TokenStream { } } - nested_params.push(match (nested_array, nested_id_prefix) { + let param = match (nested_array, nested_id_prefix) { (true, None) => NestedParams::Array { field: field_name.clone(), group: nested_group, @@ -226,7 +229,20 @@ pub fn derive_params(input: TokenStream) -> TokenStream { .to_compile_error() .into() } - }); + }; + + // Grouped parameters are handled differently than nested parameters: + // DAWs that support grouped parameters usually display parameter groups below "regular" ones + // in a tree view. To keep the order consistent with those, group parameters are collected + // separately, so they can be inserted at the end of the parameter list. + // Nested parameters without a group on the other hand are merely an implementation detail of + // the plugin and thus should not behave different than regular parameters to the user. Because + // of this, they are inserted alongside the "regular" ones. + if param.is_group() { + group_params.push(param); + } else { + params.push(AnyParam::Nested(param)); + } processed_attribute = true; } @@ -247,21 +263,17 @@ pub fn derive_params(input: TokenStream) -> TokenStream { // The next step is build the gathered information into tokens that can be spliced into a // `Params` implementation let param_map_tokens = { - // `param_map` adds the parameters from this struct, and then handles the nested tokens. - let param_mapping_self_tokens = params.into_iter().map( - |Param {field, id}| quote! { (String::from(#id), self.#field.as_ptr(), String::new()) }, - ); - - let param_mapping_nested_tokens = nested_params.iter().map(|p| p.to_token()); + let param_mapping_self_tokens = params.into_iter().map(|p| p.to_token()); + let param_mapping_group_tokens = group_params.iter().map(|p| p.to_token()); quote! { // This may not be in scope otherwise, used to call .as_ptr() use ::nih_plug::params::Param; #[allow(unused_mut)] - let mut param_map = vec![#(#param_mapping_self_tokens),*]; - - #(param_map.extend(#param_mapping_nested_tokens); )* + let mut param_map = Vec::new(); + #(param_map.extend(#param_mapping_self_tokens); )* + #(param_map.extend(#param_mapping_group_tokens); )* param_map } @@ -318,7 +330,7 @@ pub fn derive_params(input: TokenStream) -> TokenStream { .unzip(); let (serialize_fields_nested_tokens, deserialize_fields_nested_tokens): (Vec<_>, Vec<_>) = - nested_params + group_params .iter() .map(|nested| match nested { NestedParams::Inline { field, .. } | NestedParams::Prefixed { field, .. } => ( @@ -390,6 +402,23 @@ pub fn derive_params(input: TokenStream) -> TokenStream { .into() } +#[derive(Debug)] +enum AnyParam { + Single(Param), + Nested(NestedParams), +} + +impl AnyParam { + fn to_token(&self) -> proc_macro2::TokenStream { + match self { + AnyParam::Single(Param { field, id }) => { + quote! { [(String::from(#id), self.#field.as_ptr(), String::new())] } + } + AnyParam::Nested(params) => params.to_token(), + } + } +} + /// A parameter that should be added to the parameter map. #[derive(Debug)] struct Param { @@ -434,6 +463,20 @@ enum NestedParams { } impl NestedParams { + fn is_group(&self) -> bool { + let group = match self { + NestedParams::Inline { field: _, group } => group, + NestedParams::Prefixed { + field: _, + id_prefix: _, + group, + } => group, + NestedParams::Array { field: _, group } => group, + }; + + group.is_some() + } + fn to_token(&self) -> proc_macro2::TokenStream { // How nested parameters are handled depends on the `NestedParams` variant. // These are pairs of `(parameter_id, param_ptr, param_group)`. The specific diff --git a/nih_plug_derive/tests/params.rs b/nih_plug_derive/tests/params.rs index 22d8755e..b65bccad 100644 --- a/nih_plug_derive/tests/params.rs +++ b/nih_plug_derive/tests/params.rs @@ -106,11 +106,12 @@ mod param_order { fn nested() { let p = NestedParams::default(); - // Parameters must have the same order as they are defined in. Nested parameters are put in the end though. + // Parameters must have the same order as they are defined in. The position of nested parameters which are not + // grouped explicitly is preserved. let param_ids: Vec = p.param_map().into_iter().map(|(id, _, _)| id).collect(); assert_eq!( param_ids, - ["one", "three", "two_one", "two_two", "two_three"] + ["one", "two_one", "two_two", "two_three", "three",] ); } }