surge-synthesizer / stochas

The Stochas Sequencer
https://stochas.org
GNU General Public License v3.0
406 stars 35 forks source link

Negative transport value causes hang-up #27

Closed mfep closed 3 years ago

mfep commented 3 years ago

OS: Windows 10 x64 DAW: Bitwig Studio 3.2.7 edit: Stochas version: 1.3.HEAD.a2346d3

Description of bug / reproduction

Expected behavior

Please let me know if more information is needed. Thank you for developing awesome software!

rudeog commented 3 years ago

Will take a look

rudeog commented 3 years ago

I am not able to reproduce this. Started bitwig (demo mode as I don't have full version). Added stochas followed by an instrument synth. Added notes to stochas. Added another instrument channel with no plugins on it. Armed this second channel for record. Set pre-roll to 4. Set transport to beginning of timeline. Hit record and play. It did 4 bar pre-roll and then worked as normal. Am I missing anything?

mfep commented 3 years ago

After a quick investigation, I discovered that the problem only occurs, when there is a note on the first step in Stochas. Could you confirm whether you could reproduce the phenomenon with that?

baconpaul commented 3 years ago

Yeah I see the same - a note in position 0 and a preroll of 2 measures in bw (I'm still at 3.1.2 on this mac) and I get a hangup in the pre-roll. No note in position 0, no problem.

baconpaul commented 3 years ago

@rudeog I spent a bit of time this morning taking a peek, mostly because I wanted to explore the Stochas codebase a bit more and nothing like a bug dive to get you there.

Anyway the problem is that in the preroll transport on the very first time through, you end up with a negative step_position. This solves itself right away but on StochaEngine.cpp in playPositionChange we do

 position = step_position % numsteps;

and then later

 mDependencySource[position].reset();

which on mac hangs and then recovers but should probably crash.

Modulo of negative numbers always makes my head hurt and is weird and language dependent. But if just push negative steps round it works. Here's a patch. Want me to PR it? What while is a bit gross but will rarely trigger. Alternately we can do the right math which I think is (if steps < 0 ) mod = steps + mod but I am never sure.

diff --git a/src/StochaEngine.cpp b/src/StochaEngine.cpp
index ce3adbc..944b6f8 100644
--- a/src/StochaEngine.cpp
+++ b/src/StochaEngine.cpp
@@ -217,6 +217,13 @@ StochaEngine::playPositionChange(int samples_per_step, // samples per sequencer
    // this is the length of a step in samples
    int step_size = (int)((double)samples_per_step*dds);

+
+   // In some pre-roll situations step_position can be negative. Mod of negatives is tricky so just whack it
+   while( step_position < 0 )
+   {
+       step_position += numsteps;
+   }
+
    // which step are we on in this sequence
    position = step_position % numsteps;
rudeog commented 3 years ago

Paul the issue exhibited itself differently to me. In my situation, it actually got into the random generation with a negative value and ended up in a semi-infinite loop: // generate intermediate values while (ctr) { ... in here...

While your patch will work, if you look at the top of that playPositionChange function there is a jassert(step_position >=0) which is getting hit, so I believe we should ensure that step_position is >=0 before it even hits this function...

baconpaul commented 3 years ago

Yup right fix upstream indeed. I wasn't quite sure where to put a fix (like I said I was doing this to learn the codebase) which is why I dnd't send a PR.

But clearly "we assume step_position is positive and it's not in the bitwig preroll" is the answer here! So lets go hunt down those assumptions!

Neat.

rudeog commented 3 years ago

I guess the easy fix here would be to just short circuit in processBlock if ppqposition < 0 however if I do that it ends up missing the first note at the beginning of the first measure (fine on subsequent measures). So not sure...

baconpaul commented 3 years ago

Yeah I think the thing you need to do is just wrap up into the positive regime. So something like 'while position < 0 position += layer-length' but I couldn't figure out the right version of that higher up.

rudeog commented 3 years ago

Would that result in playback starting during the pre-roll?

baconpaul commented 3 years ago

Oh yeah. Right so just bail if pos < 0

rudeog commented 3 years ago

Yeah, but I think it's not as simple: Need to determine whether the zero position falls somewhere within the current sample block otherwise we can end up missing that first beat.

baconpaul commented 3 years ago

Oh I see you round up the last block before 0 to 0 in case you cross over the block. Clever solution.

mfep commented 3 years ago

👏👏👏