Move editor handling to its own struct

This makes the API much, much nicer (especially consuming the egui
wrapper), and it also avoids having to lock the plugin instance which is
obviously very bad if the plugin is also supposed to be processing audio
on another thread.

Cargo.lock

@@ -300,6 +300,7 @@ dependencies = [
 [[package]]
 name = "egui-baseview"
 version = "0.0.0"
+source = "git+https://github.com/robbert-vdh/egui-baseview.git?branch=fix/update-dependencies#161c831ded1289eb63d6cd530ac4574e6d8f351a"
 dependencies = [
  "baseview",
  "copypasta",
@@ -533,6 +534,7 @@ dependencies = [
  "egui",
  "egui-baseview",
  "nih_plug",
+ "parking_lot",
 ]
 
 [[package]]

nih_plug_egui/Cargo.toml

@@ -15,3 +15,4 @@ crossbeam = "0.8"
 egui = "0.16"
 # Upstream doesn't work with the current baseview and egui versions
 egui-baseview = { git = "https://github.com/robbert-vdh/egui-baseview.git", branch = "fix/update-dependencies" }
+parking_lot = "0.12"

nih_plug_egui/src/lib.rs

@@ -21,7 +21,8 @@
 use baseview::{Size, WindowHandle, WindowOpenOptions, WindowScalePolicy};
 use egui::CtxRef;
 use egui_baseview::{EguiWindow, RenderSettings, Settings};
-use nih_plug::{Editor, EditorWindowHandle};
+use nih_plug::{Editor, ParamSetter, ParentWindowHandle};
+use parking_lot::RwLock;
 use std::sync::Arc;
 
 /// Re-export for convenience.
@@ -30,22 +31,49 @@ pub use egui;
 
 /// Create an [Editor] instance using an [::egui] GUI. Using the state is optional, but it can be
 /// useful for keeping track of some temporary GUI-only settings. See the `gui_gain` example for
-/// more information on how to use this. The size passed to this function is the GUI's intiial size,
-/// and this is kept in sync whenever the GUI gets resized. You should return the same size value in
-/// your plugin' [nih_plug::Plugin::editor_size()] implementation.
+/// more information on how to use this. The size passed to this function is the GUI's intitial
+/// size, and this is kept in sync whenever the GUI gets resized. If you want this size to be
+/// persisted when restoring a plugin instance, then you can store it in a `#[persist]` field on
+/// your parameters struct.
 //
 // TODO: DPI scaling, this needs to be implemented on the framework level
 pub fn create_egui_editor<T, U>(
-    parent: EditorWindowHandle,
     size: Arc<AtomicCell<(u32, u32)>>,
     initial_state: T,
-    mut update: U,
+    update: U,
 ) -> Option<Box<dyn Editor>>
 where
-    T: 'static + Send,
-    U: FnMut(&CtxRef, &mut T) + 'static + Send,
+    T: 'static + Send + Sync,
+    U: Fn(&CtxRef, &ParamSetter, &mut T) + 'static + Send + Sync,
 {
-    let (width, height) = size.load();
+    Some(Box::new(EguiEditor {
+        size,
+        state: Arc::new(RwLock::new(initial_state)),
+        update: Arc::new(update),
+    }))
+}
+
+/// An [Editor] implementation that calls an egui draw loop.
+struct EguiEditor<T> {
+    size: Arc<AtomicCell<(u32, u32)>>,
+    /// The plugin's state. This is kept in between editor openenings.
+    state: Arc<RwLock<T>>,
+    update: Arc<dyn Fn(&CtxRef, &ParamSetter, &mut T) + 'static + Send + Sync>,
+}
+
+impl<T> Editor for EguiEditor<T>
+where
+    T: 'static + Send + Sync,
+{
+    fn spawn(
+        &self,
+        parent: ParentWindowHandle,
+        context: Arc<dyn nih_plug::GuiContext>,
+    ) -> Box<dyn std::any::Any> {
+        let update = self.update.clone();
+        let state = self.state.clone();
+
+        let (width, height) = self.size.load();
         let window = EguiWindow::open_parented(
             &parent,
             Settings {
@@ -73,45 +101,40 @@ where
                     ..Default::default()
                 },
             },
-        initial_state,
+            state,
             |_, _, _| {},
             move |egui_ctx, queue, state| {
+                let setter = ParamSetter::new(context.as_ref());
+
                 // For now, just always redraw. Most plugin GUIs have meters, and those almost always
                 // need a redraw. Later we can try to be a bit more sophisticated about this. Without
                 // this we would also have a blank GUI when it gets first opened because most DAWs open
                 // their GUI while the window is still unmapped.
                 // TODO: Are there other useful parts of this queue we could pass to thep lugin?
                 queue.request_repaint();
-            update(egui_ctx, state);
+                (update)(egui_ctx, &setter, &mut state.write());
             },
         );
 
-    // There's no error handling here, so let's just pray it worked
-    if window.is_open() {
-        Some(Box::new(EguiEditor { window, size }))
-    } else {
-        None
+        Box::new(EguiEditorHandle { window })
     }
-}
 
-/// An [Editor] implementation that calls an egui draw loop.
-pub struct EguiEditor {
-    window: WindowHandle,
-    size: Arc<AtomicCell<(u32, u32)>>,
-}
-
-/// The window handle enum stored within 'WindowHandle' contains raw pointers. Is there a way around
-/// having this requirement?
-unsafe impl Send for EguiEditor {}
-unsafe impl Sync for EguiEditor {}
-
-impl Editor for EguiEditor {
     fn size(&self) -> (u32, u32) {
         self.size.load()
     }
 }
 
-impl Drop for EguiEditor {
+/// The window handle used for [EguiEditor].
+struct EguiEditorHandle {
+    window: WindowHandle,
+}
+
+/// The window handle enum stored within 'WindowHandle' contains raw pointers. Is there a way around
+/// having this requirement?
+unsafe impl Send for EguiEditorHandle {}
+unsafe impl Sync for EguiEditorHandle {}
+
+impl Drop for EguiEditorHandle {
     fn drop(&mut self) {
         // XXX: This should automatically happen when the handle gets dropped, but apparently not
         self.window.close();

plugins/examples/gain-gui/src/lib.rs

@@ -19,8 +19,8 @@ extern crate nih_plug;
 
 use atomic_float::AtomicF32;
 use nih_plug::{
-    formatters, util, Buffer, BufferConfig, BusConfig, Editor, EditorWindowHandle, GuiContext,
-    ParamSetter, Plugin, ProcessContext, ProcessStatus, Vst3Plugin,
+    formatters, util, Buffer, BufferConfig, BusConfig, Editor, Plugin, ProcessContext,
+    ProcessStatus, Vst3Plugin,
 };
 use nih_plug::{FloatParam, Param, Params, Range, Smoother, SmoothingStyle};
 use nih_plug_egui::{create_egui_editor, egui, AtomicCell};
@@ -97,20 +97,13 @@ impl Plugin for Gain {
         self.params.as_ref()
     }
 
-    fn create_editor(
-        &self,
-        parent: EditorWindowHandle,
-        context: Arc<dyn GuiContext>,
-    ) -> Option<Box<dyn Editor>> {
+    fn editor(&self) -> Option<Box<dyn Editor>> {
         let params = self.params.clone();
         let peak_meter = self.peak_meter.clone();
         create_egui_editor(
-            parent,
             self.editor_size.clone(),
             (),
-            move |egui_ctx, _state| {
-                let setter = ParamSetter::new(context.as_ref());
-
+            move |egui_ctx, setter, _state| {
                 egui::CentralPanel::default().show(egui_ctx, |ui| {
                     ui.allocate_space(egui::Vec2::splat(3.0));
                     ui.label("Gain");
@@ -153,10 +146,6 @@ impl Plugin for Gain {
         )
     }
 
-    fn editor_size(&self) -> Option<(u32, u32)> {
-        Some(self.editor_size.load())
-    }
-
     fn accepts_bus_config(&self, config: &BusConfig) -> bool {
         // This works with any symmetrical IO layout
         config.num_input_channels == config.num_output_channels && config.num_input_channels > 0

src/lib.rs

@@ -34,7 +34,7 @@ pub use param::range::Range;
 pub use param::smoothing::{Smoother, SmoothingStyle};
 pub use param::{BoolParam, FloatParam, IntParam, Param};
 pub use plugin::{
-    BufferConfig, BusConfig, Editor, EditorWindowHandle, NoteEvent, Plugin, ProcessStatus,
+    BufferConfig, BusConfig, Editor, NoteEvent, ParentWindowHandle, Plugin, ProcessStatus,
     Vst3Plugin,
 };
 

src/plugin.rs

@@ -15,6 +15,7 @@
 // along with this program.  If not, see <https://www.gnu.org/licenses/>.
 
 use raw_window_handle::{HasRawWindowHandle, RawWindowHandle};
+use std::any::Any;
 use std::pin::Pin;
 use std::sync::Arc;
 
@@ -66,31 +67,12 @@ pub trait Plugin: Default + Send + Sync + 'static {
     /// plugin receives an update.
     fn params(&self) -> Pin<&dyn Params>;
 
-    /// Create an editor for this plugin and embed it in the parent window. A plugin editor will
-    /// likely want to interact with the plugin's parameters, so the idea is that you take a
-    /// reference to your [Params] object in your editor as well as the [GuiContext] that's passed
-    /// to this function. You can then read the parameter values directly from your [Params] object,
-    /// and modifying the values can be done using the functions on [GuiContext::setter()]. When you
-    /// change a parameter value that way it will be broadcasted to the host and also updated in
-    /// your [Params] struct.
-    //
-    // TODO: Think of how this would work with the event loop. On Linux the wrapper must provide a
-    //       timer using VST3's `IRunLoop` interface, but on Window and macOS the window would
-    //       normally register its own timer. Right now we just ignore this because it would
-    //       otherwise be basically impossible to have this still be GUI-framework agnostic. Any
-    //       callback that deos involve actual GUI operations will still be spooled to the IRunLoop
-    //       instance.
-    fn create_editor(
-        &self,
-        parent: EditorWindowHandle,
-        context: Arc<dyn GuiContext>,
-    ) -> Option<Box<dyn Editor>> {
-        None
-    }
-
-    /// Return the current size of the plugin's editor, if it has one. This is also used to check
-    /// whether the plugin has an editor without creating one.
-    fn editor_size(&self) -> Option<(u32, u32)> {
+    /// The plugin's editor, if it has one. The actual editor instance is created in
+    /// [Editor::spawn]. A plugin editor likely wants to interact with the plugin's parameters and
+    /// other shared data, so you'll need to move [Arc] pointing to any data you want to access into
+    /// the editor. You can later modify the parameters through the [GuiContext] and [ParamSetter]
+    /// after the editor GUI has been created.
+    fn editor(&self) -> Option<Box<dyn Editor>> {
         None
     }
 
@@ -146,8 +128,26 @@ pub trait Vst3Plugin: Plugin {
     const VST3_CATEGORIES: &'static str;
 }
 
-/// An editor for a [Plugin]. This object gets dropped when the host closes the editor.
-pub trait Editor {
+/// An editor for a [Plugin].
+pub trait Editor: Send + Sync {
+    /// Create an instance of the plugin's editor and embed it in the parent window. As explained in
+    /// [Plugin::editor], you can then read the parameter values directly from your [Params] object,
+    /// and modifying the values can be done using the functions on the [ParamSetter]. When you
+    /// change a parameter value that way it will be broadcasted to the host and also updated in
+    /// your [Params] struct.
+    ///
+    /// This function should return a handle to the editor, which will be dropped when the editor
+    /// gets closed. Implement the [Drop] trait on the returned handle if you need to explicitly
+    /// handle the editor's closing behavior.
+    //
+    // TODO: Think of how this would work with the event loop. On Linux the wrapper must provide a
+    //       timer using VST3's `IRunLoop` interface, but on Window and macOS the window would
+    //       normally register its own timer. Right now we just ignore this because it would
+    //       otherwise be basically impossible to have this still be GUI-framework agnostic. Any
+    //       callback that deos involve actual GUI operations will still be spooled to the IRunLoop
+    //       instance.
+    fn spawn(&self, parent: ParentWindowHandle, context: Arc<dyn GuiContext>) -> Box<dyn Any>;
+
     /// Return the (currnent) size of the editor in pixels as a `(width, height)` pair.
     fn size(&self) -> (u32, u32);
 
@@ -160,11 +160,11 @@ pub trait Editor {
 }
 
 /// A raw window handle for platform and GUI framework agnostic editors.
-pub struct EditorWindowHandle {
+pub struct ParentWindowHandle {
     pub handle: RawWindowHandle,
 }
 
-unsafe impl HasRawWindowHandle for EditorWindowHandle {
+unsafe impl HasRawWindowHandle for ParentWindowHandle {
     fn raw_window_handle(&self) -> RawWindowHandle {
         self.handle
     }

src/wrapper/vst3.rs

@@ -22,6 +22,7 @@ use crossbeam::atomic::AtomicCell;
 use lazy_static::lazy_static;
 use parking_lot::{RwLock, RwLockWriteGuard};
 use raw_window_handle::RawWindowHandle;
+use std::any::Any;
 use std::cmp;
 use std::collections::{HashMap, VecDeque};
 use std::ffi::{c_void, CStr};
@@ -51,7 +52,7 @@ use crate::plugin::{
 };
 use crate::wrapper::state::{ParamValue, State};
 use crate::wrapper::util::{hash_param_id, process_wrapper, strlcpy, u16strlcpy};
-use crate::EditorWindowHandle;
+use crate::ParentWindowHandle;
 
 // Alias needed for the VST3 attribute macro
 use vst3_sys as vst3_com;
@@ -104,7 +105,11 @@ macro_rules! check_null_ptr_msg {
 /// its own struct.
 struct WrapperInner<P: Plugin> {
     /// The wrapped plugin instance.
-    plugin: Box<RwLock<P>>,
+    plugin: RwLock<P>,
+    /// The plugin's editor, if it has one. This object does not do anything on its own, but we need
+    /// to instantiate this in advance so we don't need to lock the entire [Plugin] object when
+    /// creating an editor.
+    editor: Option<Arc<dyn Editor>>,
 
     /// The host's `IComponentHandler` instance, if passed through
     /// `IEditController::set_component_handler`.
@@ -173,7 +178,8 @@ pub(crate) struct Wrapper<P: Plugin> {
 #[VST3(implements(IPlugView))]
 struct WrapperView<P: Plugin> {
     inner: Arc<WrapperInner<P>>,
-    editor: RwLock<Option<Box<dyn Editor>>>,
+    editor: Arc<dyn Editor>,
+    editor_handle: RwLock<Option<Box<dyn Any>>>,
 }
 
 /// A [ProcessContext] implementation for the wrapper. This is a separate object so it can hold on
@@ -298,8 +304,12 @@ impl<P: Plugin> WrapperInner<P> {
     //      confused by all of these vtables
     #[allow(unused_unsafe)]
     pub fn new() -> Arc<Self> {
+        let plugin = RwLock::new(P::default());
+        let editor = plugin.read().editor().map(Arc::from);
+
         let mut wrapper = Self {
-            plugin: Box::new(RwLock::default()),
+            plugin,
+            editor,
 
             component_handler: RwLock::new(None),
 
@@ -425,8 +435,8 @@ impl<P: Plugin> Wrapper<P> {
 }
 
 impl<P: Plugin> WrapperView<P> {
-    pub fn new(inner: Arc<WrapperInner<P>>) -> Box<Self> {
-        Self::allocate(inner, RwLock::new(None))
+    pub fn new(inner: Arc<WrapperInner<P>>, editor: Arc<dyn Editor>) -> Box<Self> {
+        Self::allocate(inner, editor, RwLock::new(None))
     }
 }
 
@@ -977,10 +987,9 @@ impl<P: Plugin> IEditController for Wrapper<P> {
     unsafe fn create_view(&self, _name: vst3_sys::base::FIDString) -> *mut c_void {
         // Without specialization this is the least redundant way to check if the plugin has an
         // editor. The default implementation returns a None here.
-        match self.inner.plugin.read().editor_size() {
-            Some(_) => {
-                Box::into_raw(WrapperView::<P>::new(self.inner.clone())) as *mut vst3_sys::c_void
-            }
+        match &self.inner.editor {
+            Some(editor) => Box::into_raw(WrapperView::new(self.inner.clone(), editor.clone()))
+                as *mut vst3_sys::c_void,
             None => ptr::null_mut(),
         }
     }
@@ -1275,7 +1284,7 @@ impl<P: Plugin> IAudioProcessor for Wrapper<P> {
                 }
             }
 
-            let mut plugin = self.inner.plugin.write();
+            let plugin = &mut *self.inner.plugin.data_ptr();
             let mut context = self.inner.make_process_context();
             match plugin.process(&mut output_buffer, &mut context) {
                 ProcessStatus::Error(err) => {
@@ -1336,8 +1345,8 @@ impl<P: Plugin> IPlugView for WrapperView<P> {
     }
 
     unsafe fn attached(&self, parent: *mut c_void, type_: vst3_sys::base::FIDString) -> tresult {
-        let mut editor = self.editor.write();
-        if editor.is_none() {
+        let mut editor_handle = self.editor_handle.write();
+        if editor_handle.is_none() {
             let type_ = CStr::from_ptr(type_);
             let handle = match type_.to_str() {
                 #[cfg(all(target_family = "unix", not(target_os = "macos")))]
@@ -1364,15 +1373,10 @@ impl<P: Plugin> IPlugView for WrapperView<P> {
                 }
             };
 
-            // FIXME: On second thought, this needs some reworking. Needing a read lock on the
-            //        plugin's object means that the process call (which requires a write lock) will
-            //        be blocked. The better API will be to move the create function to the `Editor`
-            //        struct, so we can already fetch that during initialization.
-            *editor = self
-                .inner
-                .plugin
-                .read()
-                .create_editor(EditorWindowHandle { handle }, self.inner.clone());
+            *editor_handle = Some(
+                self.editor
+                    .spawn(ParentWindowHandle { handle }, self.inner.clone()),
+            );
             kResultOk
         } else {
             kResultFalse
@@ -1380,9 +1384,9 @@ impl<P: Plugin> IPlugView for WrapperView<P> {
     }
 
     unsafe fn removed(&self) -> tresult {
-        let mut editor = self.editor.write();
-        if editor.is_some() {
-            *editor = None;
+        let mut editor_handle = self.editor_handle.write();
+        if editor_handle.is_some() {
+            *editor_handle = None;
             kResultOk
         } else {
             kResultFalse
@@ -1416,20 +1420,9 @@ impl<P: Plugin> IPlugView for WrapperView<P> {
     unsafe fn get_size(&self, size: *mut vst3_sys::gui::ViewRect) -> tresult {
         check_null_ptr!(size);
 
-        // If the editor is already open, then take the size from the editor in case the plugin
-        // updates one but not the other
-        let (width, height) = match self.editor.read().as_ref() {
-            Some(editor) => editor.size(),
-            None => self
-                .inner
-                .plugin
-                .read()
-                .editor_size()
-                .expect("Wait, this returned a Some just now!?"),
-        };
-
         *size = mem::zeroed();
 
+        let (width, height) = self.editor.size();
         let size = &mut *size;
         size.left = 0;
         size.right = width as i32;