vst: send note articulations to VST3 instruments as keyswitches - #183
vst: send note articulations to VST3 instruments as keyswitches#183manolo wants to merge 3 commits into
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/mpe/articulationstringutils.h`:
- Around line 31-38: Replace static with inline on the namespace-scope
ARTICULATION_FAMILY_NAMES and ARTICULATION_TYPE_NAMES definitions in
articulationstringutils.h, preserving const and their existing initializers.
Confirm the project uses C++17 or newer so these inline variables provide one
shared definition across translation units.
In `@framework/vst/internal/synth/vstsequencer.cpp`:
- Around line 165-208: Update the prevState lookup in the keyswitch handling
block to use upper_bound with arrangementCtx.actualTimestamp before applying
std::prev, ensuring the most recent state at or before the timestamp is used for
chords and span articulations. Preserve the existing non-chord behavior and
keyswitch emission logic; optionally add a regression test in
vstsequencertest.cpp for multiple same-timestamp notes sharing an articulation.
🪄 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 Plus
Run ID: 686cb25a-cd0b-4151-89fc-ac67339b881d
📒 Files selected for processing (18)
framework/audio/engine/internal/audiocontext.cppframework/audio/engine/internal/audiofactory.cppframework/audio/engine/internal/audiofactory.hframework/audio/engine/internal/iaudiofactory.hframework/audio/engine/internal/nodes/eventaudionode.cppframework/audio/engine/internal/nodes/eventaudionode.hframework/audio/engine/isynthesizer.hframework/mpe/CMakeLists.txtframework/mpe/articulationstringutils.hframework/mpe/internal/articulationprofilesrepository.cppframework/mpe/qml/Muse/Mpe/articulationpatternitem.cppframework/vst/CMakeLists.txtframework/vst/internal/synth/vstsequencer.cppframework/vst/internal/synth/vstsequencer.hframework/vst/internal/synth/vstsynthesiser.cppframework/vst/internal/synth/vstsynthesiser.hframework/vst/tests/CMakeLists.txtframework/vst/tests/vstsequencertest.cpp
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/mpe/articulationstringutils.h`:
- Around line 31-38: Replace static with inline on the namespace-scope
ARTICULATION_FAMILY_NAMES and ARTICULATION_TYPE_NAMES definitions in
articulationstringutils.h, preserving const and their existing initializers.
Confirm the project uses C++17 or newer so these inline variables provide one
shared definition across translation units.
In `@framework/vst/internal/synth/vstsequencer.cpp`:
- Around line 165-208: Update the prevState lookup in the keyswitch handling
block to use upper_bound with arrangementCtx.actualTimestamp before applying
std::prev, ensuring the most recent state at or before the timestamp is used for
chords and span articulations. Preserve the existing non-chord behavior and
keyswitch emission logic; optionally add a regression test in
vstsequencertest.cpp for multiple same-timestamp notes sharing an articulation.
🪄 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 Plus
Run ID: 686cb25a-cd0b-4151-89fc-ac67339b881d
📒 Files selected for processing (18)
framework/audio/engine/internal/audiocontext.cppframework/audio/engine/internal/audiofactory.cppframework/audio/engine/internal/audiofactory.hframework/audio/engine/internal/iaudiofactory.hframework/audio/engine/internal/nodes/eventaudionode.cppframework/audio/engine/internal/nodes/eventaudionode.hframework/audio/engine/isynthesizer.hframework/mpe/CMakeLists.txtframework/mpe/articulationstringutils.hframework/mpe/internal/articulationprofilesrepository.cppframework/mpe/qml/Muse/Mpe/articulationpatternitem.cppframework/vst/CMakeLists.txtframework/vst/internal/synth/vstsequencer.cppframework/vst/internal/synth/vstsequencer.hframework/vst/internal/synth/vstsynthesiser.cppframework/vst/internal/synth/vstsynthesiser.hframework/vst/tests/CMakeLists.txtframework/vst/tests/vstsequencertest.cpp
🛑 Comments failed to post (1)
framework/mpe/articulationstringutils.h (1)
31-38: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Use
inlineinstead ofstaticfor namespace-scope maps in this public header.
ARTICULATION_FAMILY_NAMESandARTICULATION_TYPE_NAMESare declaredstatic constat namespace scope. This gives each map internal linkage. Every translation unit that includes this header gets its own private copy of the map. This header moved frominternal/to a public path specifically so more modules (VST3 keyswitch discovery, QML pattern items, the profile repository) can include it. Each new consumer duplicates the ~90-entryARTICULATION_TYPE_NAMESmap in its own object file.Declare the maps
inline constinstead. In C++17,inlinevariables at namespace scope get external linkage and a single shared definition across all translation units.♻️ Proposed fix
-static const std::unordered_map<ArticulationFamily, QString> ARTICULATION_FAMILY_NAMES { +inline const std::unordered_map<ArticulationFamily, QString> ARTICULATION_FAMILY_NAMES { { ArticulationFamily::Undefined, "Undefined" }, ... }; -static const std::unordered_map<ArticulationType, QString> ARTICULATION_TYPE_NAMES { +inline const std::unordered_map<ArticulationType, QString> ARTICULATION_TYPE_NAMES { { ArticulationType::Undefined, "Undefined" }, ... };Please confirm the project targets C++17 or newer, so
inlinevariables are available.Also applies to: 40-166
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/mpe/articulationstringutils.h` around lines 31 - 38, Replace static with inline on the namespace-scope ARTICULATION_FAMILY_NAMES and ARTICULATION_TYPE_NAMES definitions in articulationstringutils.h, preserving const and their existing initializers. Confirm the project uses C++17 or newer so these inline variables provide one shared definition across translation units.
|
Warning Review limit reached
Next review available in: 26 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThe audio event-source path now carries host track names into synthesizers. VST3 synthesizers forward supported names through channel context. VST3 keyswitch profiles are discovered from plugin controllers and passed to the sequencer. The sequencer emits latched and ranged keyswitch events for mapped articulations. Public MPE articulation string mappings use the public header path. New VST sequencer tests cover tremolo, legato, ranged modifiers, and chord keyswitch events. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1a89667 to
903cc44
Compare
Move articulationstringutils.h out of internal/ so other modules can reuse the articulation name table.
903cc44 to
876e441
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/vst/internal/synth/vstsequencer.cpp`:
- Around line 247-252: Update VstSequencer::addKeyswitchSpanEvent to avoid
adding duplicate keyswitch note-ons when the same timestamp and keyswitchPitch
already exist in destination, matching the deduplication behavior of the primary
keyswitch path. Preserve retriggers when either the timestamp or pitch differs.
- Around line 173-190: Update the primary-articulation selection logic around
keyswitchFor and the rank calculation so every mapped articulation has a
deterministic priority, including explicit ordering among non-span, non-Standard
types such as pizzicato and staccato. Ensure selection is independent of the
hash-map iteration order, and add a test that inserts the same articulations in
different orders and verifies the identical keyswitch is chosen.
- Around line 203-207: Update the event ordering around
sortNoteOnEventsByPitch() to assign keyswitch note-ons an explicit higher
priority than musical note-ons, rather than relying on MIDI pitch. Ensure every
keyswitch sharing a timestamp is emitted before the played note, including the
related logic at the additional keyswitch event site, and add a test where the
keyswitch pitch is above the musical note.
- Around line 168-170: Update the keyswitch selection in the arrangement-start
handling around keyswitchFor so it remains optional instead of defaulting to
pitch 0. Emit the keyswitch event only when a matching articulation or Standard
mapping exists, and skip emission when neither is advertised.
🪄 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 Plus
Run ID: c0d7d13a-a641-468a-bd93-67231a393ade
📒 Files selected for processing (18)
framework/audio/engine/internal/audiocontext.cppframework/audio/engine/internal/audiofactory.cppframework/audio/engine/internal/audiofactory.hframework/audio/engine/internal/iaudiofactory.hframework/audio/engine/internal/nodes/eventaudionode.cppframework/audio/engine/internal/nodes/eventaudionode.hframework/audio/engine/isynthesizer.hframework/mpe/CMakeLists.txtframework/mpe/articulationstringutils.hframework/mpe/internal/articulationprofilesrepository.cppframework/mpe/qml/Muse/Mpe/articulationpatternitem.cppframework/vst/CMakeLists.txtframework/vst/internal/synth/vstsequencer.cppframework/vst/internal/synth/vstsequencer.hframework/vst/internal/synth/vstsynthesiser.cppframework/vst/internal/synth/vstsynthesiser.hframework/vst/tests/CMakeLists.txtframework/vst/tests/vstsequencertest.cpp
876e441 to
4bd7f3e
Compare
Query the plugin's IKeyswitchController and, when it advertises keyswitches, send the articulation as a keyswitch note: latched per articulation, re-sent to retrigger a span (tremolo), and forwarded as a range span for ranged modifiers (legato). Adds unit tests for the sequencer.
…ntext Thread the track name to the synthesizer and, for VST instruments, push it as the VST3 channel context name, so a plugin can auto-select its sound from the staff name.
4bd7f3e to
2fb594a
Compare
1 participant
Resolves: musescore/MuseScore#34385
MuseScore knows each note's articulation but does not tell third party VST3 instruments about it. When a plugin advertises keyswitches through
IKeyswitchController, this queries them at load and, straight from the notation, sends the matching keyswitch note to select pizzicato, tremolo, mute, harmonic, legato and so on. It also forwards the host track name as VST3 channel context, so a plugin can auto select its sound from the staff name. Both are no-ops for plugins that do not support them, and neither affects the FluidSynth or MuseSampler backends.The goal is to let a third party VST3 instrument render articulations directly from the score, with no hacks or workarounds. The consumer this was built and tested against is Plectro, a free VST3 / AU plucked string instrument I maintain: https://github.com/manolo/vst-plectro