Finish createModel override, fix Ildaeil race condition on load

Signed-off-by: falkTX <falktx@falktx.com>

include/helpers.hpp

@@ -32,67 +32,88 @@
 
 #include <unordered_map>
 
+#include "DistrhoUtils.hpp"
+
 namespace rack {
 
-struct CardinalPluginModelHelper {
-    virtual ~CardinalPluginModelHelper() {}
-    virtual void createCachedModuleWidget(rack::engine::Module* m) = 0;
-    virtual void clearCachedModuleWidget(rack::engine::Module* m) = 0;
+struct CardinalPluginModelHelper : plugin::Model {
+    virtual app::ModuleWidget* createModuleWidgetFromEngineLoad(engine::Module* m) = 0;
+    virtual void removeCachedModuleWidget(engine::Module* m) = 0;
 };
 
 template <class TModule, class TModuleWidget>
-struct CardinalPluginModel : plugin::Model, CardinalPluginModelHelper
+struct CardinalPluginModel : CardinalPluginModelHelper
 {
-    std::unordered_map<rack::engine::Module*, app::ModuleWidget*> widgets;
+    std::unordered_map<engine::Module*, TModuleWidget*> widgets;
+    std::unordered_map<engine::Module*, bool> widgetNeedsDeletion;
 
-    rack::engine::Module* createModule() override
+    engine::Module* createModule() override
     {
         engine::Module* const m = new TModule;
         m->model = this;
         return m;
     }
 
-    app::ModuleWidget* createModuleWidget(rack::engine::Module* const m) override
+    app::ModuleWidget* createModuleWidget(engine::Module* const m) override
     {
-        TModule* tm = NULL;
-        if (m) {
-            assert(m->model == this);
+        TModule* tm = nullptr;
+        if (m)
+        {
+            DISTRHO_SAFE_ASSERT_RETURN(m->model == this, nullptr);
             if (widgets.find(m) != widgets.end())
+            {
+                widgetNeedsDeletion[m] = false;
                 return widgets[m];
+            }
             tm = dynamic_cast<TModule*>(m);
         }
-        app::ModuleWidget* mw = new TModuleWidget(tm);
-        mw->setModel(this);
-        return mw;
+        app::ModuleWidget* const tmw = new TModuleWidget(tm);
+        tmw->setModel(this);
+        return tmw;
     }
 
-    void createCachedModuleWidget(rack::engine::Module* const m) override
+    app::ModuleWidget* createModuleWidgetFromEngineLoad(engine::Module* const m) override
     {
-        assert(m != nullptr); if (m == nullptr) return;
-        assert(m->model == this); if (m->model != this) return;
+        DISTRHO_SAFE_ASSERT_RETURN(m != nullptr, nullptr);
+        DISTRHO_SAFE_ASSERT_RETURN(m->model == this, nullptr);
+
         TModule* const tm = dynamic_cast<TModule*>(m);
-        TModuleWidget* const mw = new TModuleWidget(tm);
-        mw->setModel(this);
-        widgets[m] = mw;
+        DISTRHO_SAFE_ASSERT_RETURN(tm != nullptr, nullptr);
+
+        TModuleWidget* const tmw = new TModuleWidget(tm);
+        tmw->setModel(this);
+
+        widgets[m] = tmw;
+        widgetNeedsDeletion[m] = true;
+        return tmw;
     }
 
-    void clearCachedModuleWidget(rack::engine::Module* const m) override
+    void removeCachedModuleWidget(engine::Module* const m) override
     {
-        assert(m != nullptr); if (m == nullptr) return;
-        assert(m->model == this); if (m->model != this) return;
+        DISTRHO_SAFE_ASSERT_RETURN(m != nullptr,);
+        DISTRHO_SAFE_ASSERT_RETURN(m->model == this,);
+
+        if (widgets.find(m) == widgets.end())
+            return;
+
+        if (widgetNeedsDeletion[m])
+            delete widgets[m];
+
         widgets.erase(m);
+        widgetNeedsDeletion.erase(m);
     }
 };
 
 template <class TModule, class TModuleWidget>
 CardinalPluginModel<TModule, TModuleWidget>* createModel(std::string slug)
 {
-	CardinalPluginModel<TModule, TModuleWidget>* const o = new CardinalPluginModel<TModule, TModuleWidget>();
-	o->slug = slug;
-	return o;
+    CardinalPluginModel<TModule, TModuleWidget>* const o = new CardinalPluginModel<TModule, TModuleWidget>();
+    o->slug = slug;
+    return o;
 }
 
 }
 
 #define createModel createModelOldVCV
 #include_next "helpers.hpp"
+#undef createModel

plugins/Cardinal/src/Ildaeil.cpp

@@ -208,6 +208,8 @@ static intptr_t host_dispatcher(NativeHostHandle handle, NativeHostDispatcherOpc
 
 // --------------------------------------------------------------------------------------------------------------------
 
+static Mutex sPluginInfoLoadMutex;
+
 struct IldaeilModule : Module {
     enum ParamIds {
         NUM_PARAMS
@@ -243,7 +245,6 @@ struct IldaeilModule : Module {
     mutable NativeTimeInfo fCarlaTimeInfo;
 
     void* fUI = nullptr;
-    Mutex fPluginLoadMutex;
 
     float audioDataIn1[BUFFER_SIZE];
     float audioDataIn2[BUFFER_SIZE];
@@ -417,7 +418,7 @@ struct IldaeilModule : Module {
         CarlaEngine* const engine = carla_get_engine_from_handle(fCarlaHostHandle);
 
         water::XmlDocument xml(projectState);
-        const MutexLocker cml(fPluginLoadMutex);
+        const MutexLocker cml(sPluginInfoLoadMutex);
         engine->loadProjectInternal(xml, true);
     }
 
@@ -641,12 +642,10 @@ struct IldaeilWidget : ImGuiWidget, IdleCallback, Thread {
 
     bool idleCallbackActive = false;
     IldaeilModule* const module;
-    Mutex& fPluginLoadMutex;
 
     IldaeilWidget(IldaeilModule* const m)
         : ImGuiWidget(),
-          module(m),
-          fPluginLoadMutex(m->fPluginLoadMutex)
+          module(m)
     {
         if (module->fCarlaHostHandle == nullptr)
         {
@@ -836,7 +835,7 @@ struct IldaeilWidget : ImGuiWidget, IdleCallback, Thread {
 
         carla_set_engine_option(handle, ENGINE_OPTION_PREFER_PLUGIN_BRIDGES, fPluginWillRunInBridgeMode, nullptr);
 
-        const MutexLocker cml(fPluginLoadMutex);
+        const MutexLocker cml(sPluginInfoLoadMutex);
 
         if (carla_add_plugin(handle, BINARY_NATIVE, fPluginType, nullptr, nullptr,
                              label, 0, 0x0, PLUGIN_OPTIONS_NULL))
@@ -1018,7 +1017,7 @@ struct IldaeilWidget : ImGuiWidget, IdleCallback, Thread {
         if (path != nullptr)
             carla_set_engine_option(module->fCarlaHostHandle, ENGINE_OPTION_PLUGIN_PATH, pluginType, path);
 
-        const MutexLocker cml(fPluginLoadMutex);
+        const MutexLocker cml(sPluginInfoLoadMutex);
 
         if (const uint count = carla_get_cached_plugin_count(pluginType, path))
         {
@@ -1432,12 +1431,12 @@ struct IldaeilModuleWidget : ModuleWidget {
         addOutput(createOutput<PJ301MPort>(Vec(3, 54 + 60), module, IldaeilModule::OUTPUT1));
         addOutput(createOutput<PJ301MPort>(Vec(3, 54 + 90), module, IldaeilModule::OUTPUT2));
 
-        addOutput(createInput<PJ301MPort>(Vec(3, 54 + 135), module, IldaeilModule::PITCH_INPUT));
-        addOutput(createInput<PJ301MPort>(Vec(3, 54 + 165), module, IldaeilModule::GATE_INPUT));
-        addOutput(createInput<PJ301MPort>(Vec(3, 54 + 195), module, IldaeilModule::VEL_INPUT));
-        addOutput(createInput<PJ301MPort>(Vec(3, 54 + 225), module, IldaeilModule::AFT_INPUT));
-        addOutput(createInput<PJ301MPort>(Vec(3, 54 + 255), module, IldaeilModule::PW_INPUT));
-        addOutput(createInput<PJ301MPort>(Vec(3, 54 + 285), module, IldaeilModule::MW_INPUT));
+        addInput(createInput<PJ301MPort>(Vec(3, 54 + 135), module, IldaeilModule::PITCH_INPUT));
+        addInput(createInput<PJ301MPort>(Vec(3, 54 + 165), module, IldaeilModule::GATE_INPUT));
+        addInput(createInput<PJ301MPort>(Vec(3, 54 + 195), module, IldaeilModule::VEL_INPUT));
+        addInput(createInput<PJ301MPort>(Vec(3, 54 + 225), module, IldaeilModule::AFT_INPUT));
+        addInput(createInput<PJ301MPort>(Vec(3, 54 + 255), module, IldaeilModule::PW_INPUT));
+        addInput(createInput<PJ301MPort>(Vec(3, 54 + 285), module, IldaeilModule::MW_INPUT));
     }
 
     DISTRHO_DECLARE_NON_COPYABLE_WITH_LEAK_DETECTOR(IldaeilModuleWidget)

plugins/Makefile

@@ -570,11 +570,14 @@ BASE_FLAGS += -I../src/Rack/dep/osdialog
 BASE_FLAGS += -I../src/Rack/dep/oui-blendish
 BASE_FLAGS += -I../src/Rack/dep/pffft
 
+ifeq ($(DEBUG),true)
+BASE_FLAGS += -UDEBUG
+endif
+
 ifeq ($(HEADLESS),true)
 BASE_FLAGS += -DHEADLESS
 endif
 
-
 ifeq ($(WASM),true)
 BASE_FLAGS += -DNANOVG_GLES2=1
 BASE_FLAGS += -msse -msse2 -msse3 -msimd128
@@ -611,7 +614,10 @@ BASE_FLAGS += -Wno-unused-variable
 # also from plugins
 
 BASE_FLAGS += -Wno-deprecated-declarations
+
+ifeq ($(MACOS),true)
 BASE_FLAGS += -Wno-unknown-warning-option
+endif
 
 # --------------------------------------------------------------
 # Build targets

src/Makefile

@@ -50,6 +50,10 @@ BASE_FLAGS += -IRack/dep/osdialog
 BASE_FLAGS += -IRack/dep/oui-blendish
 BASE_FLAGS += -IRack/dep/pffft
 
+ifeq ($(DEBUG),true)
+BASE_FLAGS += -UDEBUG
+endif
+
 ifeq ($(HEADLESS),true)
 BASE_FLAGS += -DHEADLESS
 endif

src/override/Engine.cpp

@@ -558,6 +558,10 @@ void Engine::removeModule_NoLock(Module* module) {
 	// Check that the module actually exists
 	auto it = std::find(internal->modules.begin(), internal->modules.end(), module);
 	DISTRHO_SAFE_ASSERT_RETURN(it != internal->modules.end(),);
+	// Remove from widgets cache
+	CardinalPluginModelHelper* const helper = dynamic_cast<CardinalPluginModelHelper*>(module->model);
+	DISTRHO_SAFE_ASSERT_RETURN(helper != nullptr,);
+	helper->removeCachedModuleWidget(module);
 	// Dispatch RemoveEvent
 	Module::RemoveEvent eRemove;
 	module->onRemove(eRemove);
@@ -586,9 +590,6 @@ void Engine::removeModule_NoLock(Module* module) {
 			m->rightExpander.module = NULL;
 		}
 	}
-	// Remove from widgets cache
-	if (auto* const helper = reinterpret_cast<CardinalPluginModelHelper*>(module->model))
-		helper->clearCachedModuleWidget(module);
 	// Remove module
 	internal->modulesCache.erase(module->id);
 	internal->modules.erase(it);
@@ -989,10 +990,11 @@ void Engine::fromJson(json_t* rootJ) {
 		DISTRHO_SAFE_ASSERT_CONTINUE(module != nullptr);
 
 		// Create the widget too, needed by a few modules
-		auto* const helper = reinterpret_cast<CardinalPluginModelHelper*>(model);
+		CardinalPluginModelHelper* const helper = dynamic_cast<CardinalPluginModelHelper*>(model);
 		DISTRHO_SAFE_ASSERT_CONTINUE(helper != nullptr);
 
-		helper->createCachedModuleWidget(module);
+		app::ModuleWidget* const moduleWidget = helper->createModuleWidgetFromEngineLoad(module);
+		DISTRHO_SAFE_ASSERT_CONTINUE(moduleWidget != nullptr);
 
 		try {
 			// This doesn't need a lock because the Module is not added to the Engine yet.
@@ -1009,6 +1011,7 @@ void Engine::fromJson(json_t* rootJ) {
 		catch (Exception& e) {
 			WARN("Cannot load module: %s", e.what());
 			// APP->patch->log(e.what());
+			helper->removeCachedModuleWidget(module);
 			delete module;
 			continue;
 		}