allow loading multi-component vst3 effects#32908
allow loading multi-component vst3 effects#32908kryksyh wants to merge 1 commit intomusescore:masterfrom
Conversation
kryksyh
force-pushed
the
multicomponent_vst3_effects
branch
from
568b4cd to
1ad5ad7
Compare
📝 WalkthroughWalkthroughThe changes enable handling of multiple audio effects per plugin path throughout the plugin discovery and registration pipeline. The plugin scanner now associates each path with multiple audio resource IDs instead of a single ID. The metadata reader collects all audio-effect classes from a plugin and generates unique identifiers for each using class names or derived labels to handle multi-component bundles. The plugin instance loader then selects the appropriate effect by matching the resource ID against both class name and identifier attributes. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/framework/audioplugins/internal/registeraudiopluginsscenario.cpp`:
- Around line 66-73: The current loop in registeraudiopluginsscenario.cpp
incorrectly treats any seen bundle path as fully reconciled and erases pathToIds
entry, which misses component-level churn; instead, when a path from
scannerRegister()->scanners()->scanPlugins() is encountered, fetch the current
plugin list/metadata for that path via the scanner
(IAudioPluginsScanner::scanPlugins()/vstpluginsscanner.cpp), diff it against the
existing entry in pathToIds (do not immediately erase the map entry), add any
newly discovered plugin IDs to result.newPluginPaths or result.newPluginIds and
record removed IDs into result.removedPluginIds, and update the stored metadata
in pathToIds to reflect the new per-plugin state; keep the pathToIds entry and
only remove it if the bundle is truly gone.
In `@src/framework/vst/internal/vstpluginmetareader.cpp`:
- Around line 73-85: The code currently persists a weak display label via
classInfo.name() into id for multiComponent bundles, which can collide across
bundles; instead persist a stable bundle-scoped token (or the class key)
separate from the human label. Change the assignment in the multiComponent
branch to use a stable identifier such as classInfo.ID().toString() (or combine
a bundle identifier + classInfo.name()) as the persisted AudioResourceId, and
keep classInfo.name() only for a displayLabel variable; ensure usedIds still
checks uniqueness against the persisted id and update downstream consumers
(e.g., VstPluginInstance(const AudioResourceId&) /
modulesRepo()->pluginModule(m_resourceId)) to expect the bundle-scoped token
rather than the bare class name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d061ceb6-3352-4c58-8678-9915c921ed83
📒 Files selected for processing (3)
src/framework/audioplugins/internal/registeraudiopluginsscenario.cppsrc/framework/vst/internal/vstplugininstance.cppsrc/framework/vst/internal/vstpluginmetareader.cpp
| for (const auto& scanner : scannerRegister()->scanners()) { | ||
| for (const auto& path : scanner->scanPlugins()) { | ||
| if (auto it = registered.find(path); it != registered.end()) { | ||
| registered.erase(it); | ||
| if (auto it = pathToIds.find(path); it != pathToIds.end()) { | ||
| pathToIds.erase(it); | ||
| } else { | ||
| result.newPluginPaths.push_back(path); | ||
| } | ||
| } |
There was a problem hiding this comment.
Rescans can't detect component churn inside an existing bundle.
Line 68 removes the whole entry as soon as the bundle path is seen, but IAudioPluginsScanner::scanPlugins() and src/framework/vst/internal/vstpluginsscanner.cpp only report paths. If a registered bundle at the same path changes from [A, B] to [A, C], this code treats it as unchanged, so B stays registered and C is never added. Matched paths need a metadata refresh/diff instead of being considered fully reconciled by path presence alone.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/framework/audioplugins/internal/registeraudiopluginsscenario.cpp` around
lines 66 - 73, The current loop in registeraudiopluginsscenario.cpp incorrectly
treats any seen bundle path as fully reconciled and erases pathToIds entry,
which misses component-level churn; instead, when a path from
scannerRegister()->scanners()->scanPlugins() is encountered, fetch the current
plugin list/metadata for that path via the scanner
(IAudioPluginsScanner::scanPlugins()/vstpluginsscanner.cpp), diff it against the
existing entry in pathToIds (do not immediately erase the map entry), add any
newly discovered plugin IDs to result.newPluginPaths or result.newPluginIds and
record removed IDs into result.removedPluginIds, and update the stored metadata
in pathToIds to reflect the new per-plugin state; keep the pathToIds entry and
only remove it if the bundle is truly gone.
| //! NOTE: For multi-component bundles, the resource ID is the component name or UID. | ||
| //! Match by name or UID against the factory's audio effect components. | ||
| for (const ClassInfo& classInfo : factory.classInfos()) { | ||
| if (classInfo.category() != kVstAudioEffectClass) { | ||
| continue; | ||
| } | ||
|
|
||
| m_pluginProvider = std::make_unique<VstPluginProvider>(factory, classInfo); | ||
| m_classInfo = classInfo; | ||
| break; | ||
| if (classInfo.name() == m_resourceId || classInfo.ID().toString() == m_resourceId) { | ||
| m_pluginProvider = std::make_unique<VstPluginProvider>(factory, classInfo); | ||
| m_classInfo = classInfo; | ||
| break; | ||
| } |
There was a problem hiding this comment.
This selector no longer matches every ID that readMeta() can emit.
src/framework/vst/internal/vstpluginmetareader.cpp still produces meta.id = baseName for single-component bundles and baseName + " N" when a multi-component class has no name. Lines 119-122 only match classInfo.name() or classInfo.ID().toString(), so those resources can never create a provider and load() falls through to "Unable to load vst plugin provider".
🧰 Tools
🪛 Cppcheck (2.20.0)
[style] 121-121: The function 'createModule' is never used.
(unusedFunction)
| std::string id; | ||
| if (multiComponent) { | ||
| id = classInfo.name(); | ||
| if (id.empty()) { | ||
| id = baseName + " " + std::to_string(i + 1); | ||
| } | ||
| if (muse::contains(usedIds, id)) { | ||
| id = classInfo.ID().toString(); | ||
| } | ||
| usedIds.insert(id); | ||
| } else { | ||
| id = baseName; | ||
| } |
There was a problem hiding this comment.
classInfo.name() is too weak to be the persisted AudioResourceId.
For multi-component bundles, Line 75 turns the lookup key into the bare class name unless there is an intra-bundle duplicate. Two different VST bundles can both expose IDs like Compressor or Chorus Stereo, but the rest of the pipeline resolves plugins by resourceId alone (VstPluginInstance(const AudioResourceId&) → modulesRepo()->pluginModule(m_resourceId)). That makes cross-bundle collisions ambiguous. Please keep a bundle-scoped/stable token in the stored ID, or persist the class key separately from the display label.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/framework/vst/internal/vstpluginmetareader.cpp` around lines 73 - 85, The
code currently persists a weak display label via classInfo.name() into id for
multiComponent bundles, which can collide across bundles; instead persist a
stable bundle-scoped token (or the class key) separate from the human label.
Change the assignment in the multiComponent branch to use a stable identifier
such as classInfo.ID().toString() (or combine a bundle identifier +
classInfo.name()) as the persisted AudioResourceId, and keep classInfo.name()
only for a displayLabel variable; ensure usedIds still checks uniqueness against
the persisted id and update downstream consumers (e.g., VstPluginInstance(const
AudioResourceId&) / modulesRepo()->pluginModule(m_resourceId)) to expect the
bundle-scoped token rather than the bare class name.
1 participant
Some rare VST3 effects contain more than one effect within a single bundle. Before this change, only the first effect could be used.
Example effect pack: Linux Studio Plugins (lsp-plugins):
MSS 4.6.5:
This PR:
