Fix a unison RR voice manager problem and reduce zone search (#1340)

1. The unison round robin interacted improperly with the voice manager and so created orphaned voices. Fix this bu changing the voice manager to have a transaction boundary around voice creation with a begin-for-count, make-up-to-count and end, which allows internal state to be constructed more evenly in the responder 2. This means the begin count counts the unison voices so the initiate just sees each unison as a different entry 3. This also means we can cache the findZone result from voice count rather than redo it, while still using fixed sized memory vehicles Closes #1192 Closes #1241
surge-synthesizer · Sep 17, 2024 · b3559b4 · b3559b4
1 parent 2f4c13b
commit b3559b4
Show file tree

Hide file tree

Showing 4 changed files with 79 additions and 20 deletions.
diff --git a/libs/sst/sst-voicemanager b/libs/sst/sst-voicemanager
diff --git a/src/configuration.h b/src/configuration.h
@@ -83,6 +83,7 @@ static constexpr bool selection{false};
 static constexpr bool uiStructure{false};
 static constexpr bool groupZoneMutation{false};
 static constexpr bool memoryPool{false};
+static constexpr bool voiceResponder{false};
 } // namespace log
 
 } // namespace scxt

diff --git a/src/engine/engine.h b/src/engine/engine.h
@@ -170,6 +170,7 @@ struct Engine : MoveableOnly<Engine>, SampleRateSupport
     {
         Engine &engine;
         std::array<pathToZone_t, maxVoices> findZoneWorkingBuffer;
+        std::array<std::pair<pathToZone_t, int32_t>, maxVoices> voiceCreationWorkingBuffer;
 
         VoiceManagerResponder(Engine &e) : engine(e) {}
 
@@ -186,14 +187,17 @@ struct Engine : MoveableOnly<Engine>, SampleRateSupport
             }
         }
 
-        int32_t voiceCountForInitializationAction(uint16_t port, uint16_t channel, uint16_t key,
-                                                  int32_t noteId, float velocity);
+        int32_t beginVoiceCreationTransaction(uint16_t port, uint16_t channel, uint16_t key,
+                                              int32_t noteId, float velocity);
 
         int32_t initializeMultipleVoices(
             std::array<voice::Voice *, VMConfig::maxVoiceCount> &voiceInitWorkingBuffer,
             uint16_t port, uint16_t channel, uint16_t key, int32_t noteId, float velocity,
             float retune);
 
+        void endVoiceCreationTransaction(uint16_t port, uint16_t channel, uint16_t key,
+                                         int32_t noteId, float velocity);
+
         void releaseVoice(voice::Voice *v, float velocity);
         void retriggerVoiceWithNewNoteID(voice::Voice *v, int32_t noteid, float velocity)
         {
@@ -216,6 +220,9 @@ struct Engine : MoveableOnly<Engine>, SampleRateSupport
         void allNotesOff() { engine.stopAllSounds(); }
         void setMIDI1CC(voice::Voice *v, int8_t cc, int8_t val);
 
+      private:
+        bool transactionValid{false};
+        int32_t transactionVoiceCount{0};
     } voiceManagerResponder{*this};
     using voiceManager_t = sst::voicemanager::VoiceManager<VMConfig, VoiceManagerResponder>;
     voiceManager_t voiceManager{voiceManagerResponder};

diff --git a/src/engine/engine_voice_responder.cpp b/src/engine/engine_voice_responder.cpp
@@ -30,28 +30,65 @@
 
 namespace scxt::engine
 {
-int32_t Engine::VoiceManagerResponder::voiceCountForInitializationAction(
-    uint16_t port, uint16_t channel, uint16_t key, int32_t noteId, float velocity)
+int32_t Engine::VoiceManagerResponder::beginVoiceCreationTransaction(uint16_t port,
+                                                                     uint16_t channel, uint16_t key,
+                                                                     int32_t noteId, float velocity)
 {
+    SCLOG_IF(voiceResponder, "begin voice transaction " << SCD(port) << SCD(channel) << SCD(key)
+                                                        << SCD(noteId) << SCD(velocity));
+    assert(!transactionValid);
     // TODO: We can optimize this so we don't have to find twice in the future
     auto useKey = engine.midikeyRetuner.remapKeyTo(channel, key);
     auto nts = engine.findZone(channel, useKey, noteId, std::clamp((int)(velocity * 128), 0, 127),
                                findZoneWorkingBuffer);
 
-    return nts;
+    auto voicesCreated{0};
+    for (auto idx = 0; idx < nts; ++idx)
+    {
+        const auto &path = findZoneWorkingBuffer[idx];
+        auto &z = engine.zoneByPath(path);
+        if (z->variantData.variantPlaybackMode == Zone::UNISON)
+        {
+            for (int i = 0; i < maxVariantsPerZone; ++i)
+            {
+                if (z->variantData.variants[i].active)
+                {
+                    voiceCreationWorkingBuffer[voicesCreated] = {path, i};
+                    SCLOG_IF(voiceResponder, "-- Created at " << voicesCreated << " - " << path.part
+                                                              << "/" << path.group << "/"
+                                                              << path.zone << " variant=" << i);
+                    voicesCreated++;
+                }
+            }
+        }
+        else
+        {
+            voiceCreationWorkingBuffer[voicesCreated] = {path, -1};
+            SCLOG_IF(voiceResponder, "-- Created at " << voicesCreated << " - " << path.part << "/"
+                                                      << path.group << "/" << path.zone
+                                                      << "  zone handles variant");
+
+            voicesCreated++;
+        }
+    }
+    transactionValid = true;
+    transactionVoiceCount = voicesCreated;
+    SCLOG_IF(voiceResponder, "beginTransaction returns " << voicesCreated << " voices");
+    return voicesCreated;
 }
 
 int32_t Engine::VoiceManagerResponder::initializeMultipleVoices(
     std::array<voice::Voice *, VMConfig::maxVoiceCount> &voiceInitWorkingBuffer, uint16_t port,
     uint16_t channel, uint16_t key, int32_t noteId, float velocity, float retune)
 {
+    assert(transactionValid);
+    assert(transactionVoiceCount > 0);
     auto useKey = engine.midikeyRetuner.remapKeyTo(channel, key);
-    auto nts = engine.findZone(channel, useKey, noteId, std::clamp((int)(velocity * 128), 0, 127),
-                               findZoneWorkingBuffer);
-
+    auto nts = transactionVoiceCount;
+    SCLOG_IF(voiceResponder, "voice initiation of " << nts << " voices");
     for (auto idx = 0; idx < nts; ++idx)
     {
-        const auto &path = findZoneWorkingBuffer[idx];
+        const auto &[path, variantIndex] = voiceCreationWorkingBuffer[idx];
         auto &z = engine.zoneByPath(path);
         auto nbSampleLoadedInZone = z->getNumSampleLoaded();
 
@@ -66,24 +103,27 @@ int32_t Engine::VoiceManagerResponder::initializeMultipleVoices(
                 v->attack();
             }
             voiceInitWorkingBuffer[idx] = v;
+            SCLOG_IF(voiceResponder, "-- Created single voice for single zone");
         }
         else if (z->variantData.variantPlaybackMode == Zone::UNISON)
         {
-            for (int uv = 0; uv < nbSampleLoadedInZone; ++uv)
+            assert(variantIndex >= 0);
+            z->sampleIndex = variantIndex;
+            auto v = engine.initiateVoice(path);
+            if (v)
             {
-                z->sampleIndex = uv;
-                auto v = engine.initiateVoice(path);
-                if (v)
-                {
-                    v->velocity = velocity;
-                    v->originalMidiKey = key;
-                    v->attack();
-                }
-                voiceInitWorkingBuffer[idx] = v;
+                v->velocity = velocity;
+                v->originalMidiKey = key;
+                v->attack();
             }
+            voiceInitWorkingBuffer[idx] = v;
+            SCLOG_IF(voiceResponder,
+                     "-- Created one of variants for zone (" << variantIndex << ")");
         }
         else
         {
+            assert(variantIndex == -1);
+            SCLOG_IF(voiceResponder, "-- Launching zone and using its RR tactic");
             int nextAvail{0};
             if (nbSampleLoadedInZone == 1)
             {
@@ -157,9 +197,20 @@ int32_t Engine::VoiceManagerResponder::initializeMultipleVoices(
         }
     }
     engine.midiNoteStateCounter++;
+    SCLOG_IF(voiceResponder, "Completed voice initiation");
     return nts;
 }
 
+void Engine::VoiceManagerResponder::endVoiceCreationTransaction(uint16_t port, uint16_t channel,
+                                                                uint16_t key, int32_t noteId,
+                                                                float velocity)
+{
+    SCLOG_IF(voiceResponder, "end voice transaction");
+    assert(transactionValid);
+    transactionValid = false;
+    transactionVoiceCount = 0;
+}
+
 void Engine::VoiceManagerResponder::releaseVoice(voice::Voice *v, float velocity) { v->release(); }
 
 void Engine::VoiceManagerResponder::setVoiceMIDIPitchBend(voice::Voice *v, uint16_t pb14bit)
+40 −0		.github/workflows/build-pr.yml
+17 −0		.github/workflows/code-checks.yml
+1 −0		CMakeLists.txt
+16 −4		include/sst/voicemanager/managers/polymanager.h
+2 −6		include/sst/voicemanager/voicemanager.h
+0 −0		libs/catch2/catch2.hpp
+28 −0		tests/basic_poly.cpp