From c22e5226294fc5c642965b3541a0d96c5c1662e7 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Sun, 20 Mar 2022 13:03:03 +0100 Subject: [PATCH] Greatly simplify Params trait This now is a single vector with all of the information in the correct order instead of the hashmaps and a vector. This avoids deduplication, and it especially makes manual `Params` implementations a lot more convenient since you can't mess up with mismatching IDs between the methods. To accommodate exactly this, the persistent fields methods also have a default implementation and the trait has been marked as `unsafe` since it's the programmer's responsibility to make sure these `ParamPtr`s will remain valid. --- nih_plug_derive/src/params.rs | 75 +++++++++++------------------------ src/param/internals.rs | 46 ++++++++++----------- src/plugin.rs | 2 +- src/wrapper/clap/wrapper.rs | 59 ++++++++++++++++----------- src/wrapper/vst3/inner.rs | 57 +++++++++++++++----------- 5 files changed, 113 insertions(+), 126 deletions(-) diff --git a/nih_plug_derive/src/params.rs b/nih_plug_derive/src/params.rs index 331e51c9..82233e98 100644 --- a/nih_plug_derive/src/params.rs +++ b/nih_plug_derive/src/params.rs @@ -29,9 +29,7 @@ pub fn derive_params(input: TokenStream) -> TokenStream { // those fields individually (so they can be added and removed independently of eachother) using // JSON. The `nested` fields should also implement the `Params` trait and their fields will be // inherited and added to this field's lists. - let mut param_mapping_insert_tokens = Vec::new(); - let mut param_groups_insert_tokens = Vec::new(); - let mut param_id_string_tokens = Vec::new(); + let mut param_mapping_self_tokens = Vec::new(); let mut field_serialize_tokens = Vec::new(); let mut field_deserialize_tokens = Vec::new(); let mut nested_params_field_idents: Vec = Vec::new(); @@ -146,18 +144,16 @@ pub fn derive_params(input: TokenStream) -> TokenStream { .into(); } - // The specific parameter types know how to convert themselves into the correct ParamPtr - // variant - param_mapping_insert_tokens - .push(quote! { param_map.insert(String::from(#param_id), self.#field_name.as_ptr()); }); + // 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 - param_groups_insert_tokens - .push(quote! { param_groups.insert(String::from(#param_id), String::new()); }); - param_id_string_tokens.push(quote! { String::from(#param_id), }); + param_mapping_self_tokens.push( + quote! { (String::from(#param_id), self.#field_name.as_ptr(), String::new()) }, + ); } - (None, Some(stable_name)) => { - if !persist_ids.insert(stable_name.clone()) { + (None, Some(persist_key)) => { + if !persist_ids.insert(persist_key.clone()) { return syn::Error::new( field.span(), "Multiple persisted fields with the same ID found", @@ -175,15 +171,15 @@ pub fn derive_params(input: TokenStream) -> TokenStream { ::nih_plug::param::internals::serialize_field, ) { Ok(data) => { - serialized.insert(String::from(#stable_name), data); + serialized.insert(String::from(#persist_key), data); } Err(err) => { - ::nih_plug::nih_log!("Could not serialize '{}': {}", #stable_name, err) + ::nih_plug::nih_log!("Could not serialize '{}': {}", #persist_key, err) } }; }); field_deserialize_tokens.push(quote! { - #stable_name => { + #persist_key => { match ::nih_plug::param::internals::deserialize_field(&data) { Ok(deserialized) => { ::nih_plug::param::internals::PersistentField::set( @@ -194,7 +190,7 @@ pub fn derive_params(input: TokenStream) -> TokenStream { Err(err) => { ::nih_plug::nih_log!( "Could not deserialize '{}': {}", - #stable_name, + #persist_key, err ) } @@ -221,43 +217,29 @@ pub fn derive_params(input: TokenStream) -> TokenStream { } quote! { - impl #impl_generics Params for #struct_name #ty_generics #where_clause { + unsafe impl #impl_generics Params for #struct_name #ty_generics #where_clause { fn param_map( self: std::pin::Pin<&Self>, - ) -> std::collections::HashMap { + ) -> Vec<(String, nih_plug::param::internals::ParamPtr, String)> { // This may not be in scope otherwise, used to call .as_ptr() use ::nih_plug::param::Param; - let mut param_map = std::collections::HashMap::new(); - #(#param_mapping_insert_tokens)* - - let nested_params_fields: &[&dyn Params] = &[#(&self.#nested_params_field_idents),*]; - for nested_params in nested_params_fields { - unsafe { param_map.extend(Pin::new_unchecked(*nested_params).param_map()) }; - } - - param_map - } - - fn param_groups( - self: std::pin::Pin<&Self>, - ) -> std::collections::HashMap { - let mut param_groups = std::collections::HashMap::new(); - #(#param_groups_insert_tokens)* + let mut param_map = vec![#(#param_mapping_self_tokens),*]; let nested_params_fields: &[&dyn Params] = &[#(&self.#nested_params_field_idents),*]; let nested_params_groups: &[String] = &[#(String::from(#nested_params_group_names)),*]; for (nested_params, group_name) in nested_params_fields.into_iter().zip(nested_params_groups) { - let nested_param_groups = - unsafe { std::pin::Pin::new_unchecked(*nested_params).param_groups() }; - let prefixed_nested_param_groups = - nested_param_groups + let nested_param_map = + unsafe { std::pin::Pin::new_unchecked(*nested_params).param_map() }; + let prefixed_nested_param_map = + nested_param_map .into_iter() - .map(|(param_id, nested_group_name)| { + .map(|(param_id, param_ptr, nested_group_name)| { ( param_id, + param_ptr, if nested_group_name.is_empty() { group_name.to_string() } else { @@ -266,21 +248,10 @@ pub fn derive_params(input: TokenStream) -> TokenStream { ) }); - param_groups.extend(prefixed_nested_param_groups); + param_map.extend(prefixed_nested_param_map); } - param_groups - } - - fn param_ids(self: std::pin::Pin<&Self>) -> Vec { - let mut ids = vec![#(#param_id_string_tokens)*]; - - let nested_params_fields: &[&dyn Params] = &[#(&self.#nested_params_field_idents),*]; - for nested_params in nested_params_fields { - unsafe { ids.append(&mut Pin::new_unchecked(*nested_params).param_ids()) }; - } - - ids + param_map } fn serialize_fields(&self) -> ::std::collections::HashMap { diff --git a/src/param/internals.rs b/src/param/internals.rs index f91e23a2..4055a6ea 100644 --- a/src/param/internals.rs +++ b/src/param/internals.rs @@ -39,43 +39,37 @@ pub use serde_json::to_string as serialize_field; /// /// This implementation is safe when using from the wrapper because the plugin object needs to be /// pinned, and it can never outlive the wrapper. -pub trait Params { - // NOTE: These types use `String` even though for the `Params` derive macro `&'static str` would - // have been fine to be able to support custom reusable Params implemnetations. - - // TODO: Combine `param_map`, `param_groups`, and `param_ids` into a single function that - // returns a vector of tuples - - /// Create a mapping from unique parameter IDs to parameters. This is done for every parameter - /// field marked with `#[id = "stable_name"]`. Dereferencing the pointers stored in the values - /// is only valid as long as this pinned object is valid. - fn param_map(self: Pin<&Self>) -> HashMap; - - /// Contains group names for each parameter in [`param_map()`][Self::param_map()]. This is - /// either an empty string for top level parameters, or a slash/delimited `"Group Name 1/Group - /// Name 2"` string for parameters that belong to `#[nested = "Name"]` parameter objects. - fn param_groups(self: Pin<&Self>) -> HashMap; - - /// All parameter IDs from [`param_map()`][Self::param_map()], in a stable order. This order - /// will be used to display the parameters. +pub unsafe trait Params { + /// Create a mapping from unique parameter IDs to parameters along with the name of the + /// group/unit/module they are in. The order of the `Vec` determines the display order in the + /// (host's) generic UI. The group name is either an empty string for top level parameters, or a + /// slash/delimited `"Group Name 1/Group Name 2"` path for parameters in nested groups. All + /// components of a group path must exist or may encounter panics. The derive macro does this + /// for every parameter field marked with `#[id = "stable"]`, and it also inlines all fields + /// from child `Params` structs marked with `#[nested = "Group Name"]`, prefixing that group + /// name before the parameter's originanl group name. Dereferencing the pointers stored in the + /// values is only valid as long as this pinned object is valid. /// - /// TODO: This used to be a static slice, but now that we supported nested parameter objects - /// that's become a bit more difficult since Rust does not have a convenient way to - /// concatenate an arbitrary number of static slices. There's probably a better way to do - /// this. - fn param_ids(self: Pin<&Self>) -> Vec; + /// # Note + /// + /// This uses `String` even though for the `Params` derive macro `&'static str` would have been + /// fine to be able to support custom reusable Params implemnetations. + fn param_map(self: Pin<&Self>) -> Vec<(String, ParamPtr, String)>; /// Serialize all fields marked with `#[persist = "stable_name"]` into a hash map containing /// JSON-representations of those fields so they can be written to the plugin's state and /// recalled later. This uses [`serialize_field()`] under the hood. - fn serialize_fields(&self) -> HashMap; + fn serialize_fields(&self) -> HashMap { + HashMap::new() + } /// Restore all fields marked with `#[persist = "stable_name"]` from a hashmap created by /// [`serialize_fields()`][Self::serialize_fields()]. All of thse fields should be wrapped in a /// [`PersistentField`] with thread safe interior mutability, like an `RwLock` or a `Mutex`. /// This gets called when the plugin's state is being restored. This uses [deserialize_field()] /// under the hood. - fn deserialize_fields(&self, serialized: &HashMap); + #[allow(unused_variables)] + fn deserialize_fields(&self, serialized: &HashMap) {} } /// Internal pointers to parameters. This is an implementation detail used by the wrappers for type diff --git a/src/plugin.rs b/src/plugin.rs index cab739c3..11f556a8 100644 --- a/src/plugin.rs +++ b/src/plugin.rs @@ -135,7 +135,7 @@ pub trait Plugin: Default + Send + Sync + 'static { /// this function with the same maximum block size first before calling /// [`Smoother::next_block()`][crate::prelude::Smoother::next_block()]. fn initialize_block_smoothers(&mut self, max_block_size: usize) { - for param in self.params().param_map().values_mut() { + for (_, mut param, _) in self.params().param_map() { unsafe { param.initialize_block_smoother(max_block_size) }; } } diff --git a/src/wrapper/clap/wrapper.rs b/src/wrapper/clap/wrapper.rs index 867ade0d..0f399c5a 100644 --- a/src/wrapper/clap/wrapper.rs +++ b/src/wrapper/clap/wrapper.rs @@ -52,7 +52,7 @@ use parking_lot::RwLock; use raw_window_handle::RawWindowHandle; use std::any::Any; use std::cmp; -use std::collections::{HashMap, VecDeque}; +use std::collections::{HashMap, HashSet, VecDeque}; use std::ffi::{c_void, CStr}; use std::mem; use std::os::raw::c_char; @@ -327,45 +327,56 @@ impl Wrapper

{ let host_callback = unsafe { ClapPtr::new(host_callback) }; // This is a mapping from the parameter IDs specified by the plugin to pointers to thsoe - // parameters. Since the object returned by `params()` is pinned, these pointers are safe to - // dereference as long as `wrapper.plugin` is alive - let param_map = plugin.read().params().param_map(); - let param_ids = plugin.read().params().param_ids(); - let param_groups = plugin.read().params().param_groups(); - nih_debug_assert!( - !param_map.contains_key(BYPASS_PARAM_ID), - "The wrapper already adds its own bypass parameter" - ); - - // Only calculate these hashes once, and in the stable order defined by the plugin - let param_id_hashes_ptrs_groups: Vec<_> = param_ids - .iter() - .map(|id| { - // If any of these keys are missing then that's a bug in the Params implementation - let param_ptr = param_map[id]; - let param_group = ¶m_groups[id]; - (id, hash_param_id(id), param_ptr, param_group) + // parameters. These pointers are assumed to be safe to dereference as long as + // `wrapper.plugin` is alive. The plugin API identifiers these parameters by hashes, which + // we'll calculate from the string ID specified by the plugin. These parameters should also + // remain in the same order as the one returned by the plugin. + let param_id_hashes_ptrs_groups: Vec<_> = plugin + .read() + .params() + .param_map() + .into_iter() + .map(|(id, ptr, group)| { + let hash = hash_param_id(&id); + (id, hash, ptr, group) }) .collect(); + if cfg!(debug_assertions) { + let param_map = plugin.read().params().param_map(); + let param_ids: HashSet<_> = param_id_hashes_ptrs_groups + .iter() + .map(|(id, _, _, _)| id.clone()) + .collect(); + nih_debug_assert!( + !param_ids.contains(BYPASS_PARAM_ID), + "The wrapper already adds its own bypass parameter" + ); + nih_debug_assert_eq!( + param_map.len(), + param_ids.len(), + "The plugin has duplicate parameter IDs, weird things may happen" + ); + } + let param_hashes = param_id_hashes_ptrs_groups .iter() - .map(|&(_, hash, _, _)| hash) + .map(|(_, hash, _, _)| *hash) .collect(); let param_by_hash = param_id_hashes_ptrs_groups .iter() - .map(|&(_, hash, ptr, _)| (hash, ptr)) + .map(|(_, hash, ptr, _)| (*hash, *ptr)) .collect(); let param_group_by_hash = param_id_hashes_ptrs_groups .iter() - .map(|&(_, hash, _, group)| (hash, group.to_string())) + .map(|(_, hash, _, group)| (*hash, group.clone())) .collect(); let param_defaults_normalized = param_id_hashes_ptrs_groups .iter() - .map(|&(_, hash, ptr, _)| (hash, unsafe { ptr.normalized_value() })) + .map(|(_, hash, ptr, _)| (*hash, unsafe { ptr.normalized_value() })) .collect(); let param_id_to_hash = param_id_hashes_ptrs_groups .iter() - .map(|&(id, hash, _, _)| (id.clone(), hash)) + .map(|(id, hash, _, _)| (id.clone(), *hash)) .collect(); let param_ptr_to_hash = param_id_hashes_ptrs_groups .into_iter() diff --git a/src/wrapper/vst3/inner.rs b/src/wrapper/vst3/inner.rs index e20ed061..2b4a0164 100644 --- a/src/wrapper/vst3/inner.rs +++ b/src/wrapper/vst3/inner.rs @@ -2,7 +2,7 @@ use atomic_refcell::AtomicRefCell; use crossbeam::atomic::AtomicCell; use parking_lot::RwLock; use std::cmp::Reverse; -use std::collections::{BinaryHeap, HashMap, VecDeque}; +use std::collections::{BinaryHeap, HashMap, HashSet, VecDeque}; use std::mem::MaybeUninit; use std::sync::atomic::{AtomicBool, AtomicU32, Ordering}; use std::sync::Arc; @@ -137,25 +137,36 @@ impl WrapperInner

{ let editor = plugin.read().editor().map(Arc::from); // This is a mapping from the parameter IDs specified by the plugin to pointers to thsoe - // parameters. Since the object returned by `params()` is pinned, these pointers are safe to - // dereference as long as `wrapper.plugin` is alive - let param_map = plugin.read().params().param_map(); - let param_groups = plugin.read().params().param_groups(); - let param_ids = plugin.read().params().param_ids(); - nih_debug_assert!( - !param_map.contains_key(BYPASS_PARAM_ID), - "The wrapper already adds its own bypass parameter" - ); - - // Only calculate these hashes once, and in the stable order defined by the plugin - let param_id_hashes_ptrs_groups: Vec<_> = param_ids - .iter() - .filter_map(|id| { - let param_ptr = param_map.get(id)?; - let param_group = param_groups.get(id)?; - Some((id, hash_param_id(id), param_ptr, param_group)) + // parameters. These pointers are assumed to be safe to dereference as long as + // `wrapper.plugin` is alive. The plugin API identifiers these parameters by hashes, which + // we'll calculate from the string ID specified by the plugin. These parameters should also + // remain in the same order as the one returned by the plugin. + let param_id_hashes_ptrs_groups: Vec<_> = plugin + .read() + .params() + .param_map() + .into_iter() + .map(|(id, ptr, group)| { + let hash = hash_param_id(&id); + (id, hash, ptr, group) }) .collect(); + if cfg!(debug_assertions) { + let param_map = plugin.read().params().param_map(); + let param_ids: HashSet<_> = param_id_hashes_ptrs_groups + .iter() + .map(|(id, _, _, _)| id.clone()) + .collect(); + nih_debug_assert!( + !param_ids.contains(BYPASS_PARAM_ID), + "The wrapper already adds its own bypass parameter" + ); + nih_debug_assert_eq!( + param_map.len(), + param_ids.len(), + "The plugin has duplicate parameter IDs, weird things may happen" + ); + } let param_hashes = param_id_hashes_ptrs_groups .iter() @@ -163,25 +174,25 @@ impl WrapperInner

{ .collect(); let param_by_hash = param_id_hashes_ptrs_groups .iter() - .map(|&(_, hash, ptr, _)| (hash, *ptr)) + .map(|&(_, hash, ptr, _)| (hash, ptr)) .collect(); let param_units = ParamUnits::from_param_groups( param_id_hashes_ptrs_groups .iter() - .map(|&(_, hash, _, group_name)| (hash, group_name.as_str())), + .map(|(_, hash, _, group_name)| (*hash, group_name.as_str())), ) .expect("Inconsistent parameter groups"); let param_defaults_normalized = param_id_hashes_ptrs_groups .iter() - .map(|&(_, hash, ptr, _)| (hash, unsafe { ptr.normalized_value() })) + .map(|(_, hash, ptr, _)| (*hash, unsafe { ptr.normalized_value() })) .collect(); let param_id_to_hash = param_id_hashes_ptrs_groups .iter() - .map(|&(id, hash, _, _)| (id.clone(), hash)) + .map(|(id, hash, _, _)| (id.clone(), *hash)) .collect(); let param_ptr_to_hash = param_id_hashes_ptrs_groups .into_iter() - .map(|(_, hash, ptr, _)| (*ptr, hash)) + .map(|(_, hash, ptr, _)| (ptr, hash)) .collect(); let wrapper = Self {