WIP: Basic support for octave-per-channel mode

[cgibbard]

See "Use MIDI Channel for Octave Shift" on the Tuning menu.

Only works correctly so far when tuning is applied at MIDI input, and has no special treatment for scene masking yet.

Aims to resolve #4151

[cgibbard]

Nice, got some kind of numerical floating point issue with the tests only on Windows (frequencyForNote seems to be giving NaN)... I could try to restrict the range on that test a bit further I suppose, but it seems a bit suspicious. I don't have a Windows machine handy to build/test on.

[mkruselj]

This FR also kinda requires #4657 to be implemented, otherwise MIDI channel usage gets all over the place.

EDIT: Oh, welcome and thank you for your PR! You're welcome to join us on our Discord!

[baconpaul]

@cgibbard cool I will review this am
@mkruselj @cgibbard is Cale on discord snd has been chatting with me in dev and micro tuning

[mkruselj]

Just noticed!

[baconpaul]

First the code looks great. Clean, well written, totally mergeable if we fix up one or two things. Thank you and welcome to the team!!

Let me rerun that windows job otherwise we will have to actually use windows. I know terrible right? But I have a VM. Let me know if you do also. Although further down these comments I think I figure it out so keep reading :)

@mkruselj is correct that we also need to fix the channel masking stuff. In the original issue I made a comment about this. Basically you have to temporarily force this mode out of channel split mode and avoid the dual mode singles. That's this cryptic comment: #4151 (comment)

Basically in 'dual' mode surge pushes channel 2 to scene A only and channel 3 to scene B only. the issue @mkruselj linked fixes this more in general but I proposed in original issue when we are in 'force octave per' mode we just don't do that. I really think it is just at

surge/src/common/SurgeSynthesizer.cpp

Line 325 in 0fcdf44

else if (storage.getPatch().scenemode.val.i == sm_single)

to add a && ! storage->yourNewVariable but requires some testing. That is probably also why channel 2 failed in your test now I think of it - it is not playing a sine and it blew up.

Now finally, now that I look at it, I think we also need to think do we store this setting in the patch or on the DAW Extra State. In my original comments I suggested patch (which you did) but I think that's wrong. I think, like the SCL/KBM, it should be in the daw extra state. @mkruselj if you agree I'll write up some comments on how to do that.

[mkruselj]

Yep, I agree it should be in DAW state, not the patch.

[baconpaul]

And ok yeah that windows failure is real and I bet it is from The channel 2 going to the wrong scene handling. Interesting - I wonder what I have scene 2 set to in the sine patch - so definitely need to fix that.

commenrs on daw extra state in a minute

[baconpaul]

OK @cgibbard yeah I agree with @mkruselj this is a "daw extra state" thing. The 'daw extra state' is basically the things which are streamed in your DAW session (so when you save a reaper or ardour project or whatever) which are not part of the patch. Determining which is which is a perennial problem (not just for surge - other audio devs also have a hard time with it). The reason this setting is a daw extra is that you want to separate the performance (daw) from the sound (patch) for this I think.

So here's the change you would make

1. in SurgeStorage.h there's a DAWExtraStateStorage - right after the tunings just add a bool
2. In SurgeSynthesizer.cpp there's a function populateDAWExtraStateStorage. There you want to copy the value of your state variable onto the storage object.
3. In SurgeSynthesizer.cpp there's a function loadDAWExtraState right below where you want to do the opposite of step 2 - load from the state onto the synth
4. Move your streaming code in SurgeStorage (which streams into the patch) into streaming into the dawExtraState region. At main that's around line 1948 but look at line 2033 to see how we handle MPE and so on - yours is the same. Similarly around line 2511 is the write side.
5. clang-format, fix the other channel mask thing, and if you want rebase and force push, and then if CI works I'd be happy to merge!

Thanks so much

[cgibbard]

Ah, that makes sense, and agrees with what would probably be convenient for me, since this sort of expresses how my keyboard/DAW is communicating with Surge more than it expresses something about a particular patch. I'd rather not have to toggle it every time I loaded a new patch anyway. I'll give it a shot at some point today.

[cgibbard]

Followed along with the instructions and force pushed. At first I thought I was going to be wholesale moving the field from SurgeStorage to the DAWExtraStateStorage, but then steps 2 and 3 wouldn't have made sense, so hopefully I got the interpretation right. Feels a little strange to have the state in two places now, but seems to roughly work at least.

Also, we still have the kind of odd Windows-only numerical issue it seems. I wonder if there's something different about how the floating point code is compiled on Windows. If this has something to do with the second scene not producing audio, shouldn't it also fail the same way on Linux and Mac?

I wonder what would be involved in making this work with tuning after modulation mode. It feels like if we don't fix its behaviour, we should at least disable that mode in the GUI when this is turned on. Presently, this has some effect on tuning after modulation, but whatever it's doing doesn't seem correct.

Perhaps we ought to visually represent the Chan Split mode being disabled as well, when this is turned on, but I'm not sure if the future plans for MIDI modes will render it a bit moot.

[baconpaul]

Oh I wonder if it is % operator on - number returning neg on win.

Try ( chanoff+16 ) % 16 maybe?

we also don’t want that cmake change I don’t think.

I have some time later today to peek on ein if neither work. All look great

[baconpaul]

Yeah that will be it for sure. From stack overflow quoting the spec:

The binary / operator yields the quotient, and the binary % operator yields the remainder from the division of the first expression by the second. If the second operand of / or % is zero the behavior is undefined; otherwise (a/b)*b + a%b is equal to a. If both operands are nonnegative then the remainder is nonnegative; if not, the sign of the remainder is implementation-defined.

so

 for (int chanOff = -2; chanOff < 3; chanOff++)
         { // Only checking reasonable octaves because frequencyForNote actually examines the
           // waveform
             int useChan = (chanOff+16)%16;
             INFO("chanOff is " << chanOff << " yielding " << useChan );
             f1 = frequencyForNote(surge, 60, 2, 0, useChan % 16);
             f2 = frequencyForNote(surge, 60, 2, 0, (useChan + 1) % 16);

and it explains the differences since MSVC will give you a channel of -2 for -2 value as opposed to 14. (Or what not) but gcc and clang won't.

[battaglia01]

Hi everyone - this looks good, but noting that modulo 16 above, just to check, what is the behavior for channel 16? It's supposed to be that ch. 2 is an octave above ch. 1, and ch. 3 is 2 octaves, but also that ch. 16 is an octave below, ch. 15 is two octaves below, ch. 14 is three octaves below, etc, with it wrapping around at channel 8.

[baconpaul]

That modulo is just in a test so shouldn’t impact usage. Do you see otherwise?

[cgibbard]

Also, it does work as you describe. I have a Lumatone and have layouts set up for PianoTeq as well ;)

[battaglia01]

@cgibbard great stuff! What's the best way to play around with this right now? Just compile the main branch?

[mkruselj]

Yes!

[cgibbard]

Yeah, and then under the Tuning menu, make sure it's set to "Apply Tuning at MIDI Input" and "Use MIDI Channel for Octave Shift"