allow polyaftertouch extended CC (130) to respect note num

[essej]

This is especially useful when used as trigger with on_lo/hicc130 opcodes, so that the key range can be used select appropriate regions independently. This is primarily useful with e-drums which send polyAT for cymbal chokes, and with this change it's now possible to choke multiple independent notes based on polyAT. (sforzando/ARIA handled it correctly already, now sfizz does too).

The implementation also opens up the possibility that other extended CCs could pass needed extra value if necessary (haven't checked if any others need it yet...)

[paulfd]

Thanks for the MR! I'd like to add a some form of testing for this, do you have the typical use case that triggered you to make this change?

[essej]

Yes, I mentioned the use case in my initial comment here... correctly dealing with multiple cymbal chokes sent with poly-aftertouch from e-drum kits. E-drum modules send polyAT of value 127 with the cymbal note num when squeezed, followed by value 0. Currently in SFZ the only way to trigger region (and do offby muting) based on that is using on_locc130/on_hicc130 opcode to trigger on the polyAT extended CC. Before this PR, the implementation was not able to pass on the note num information for a polyAT message to the triggering logic, so that ANY polyAT message would trigger a region setup with the on_locc130... regardless of its lokey/hikey, meaning you couldn't use separate independent choke-able cymbals.

Here is an example:

<global>
loop_mode=one_shot

// crash1 cymbal
<group>
key=55
group=5 off_by=6
off_mode=normal
ampeg_release=0.6
<region> sample=crash1_OH_P_1.wav

// crash1 choke triggered on polyAT
<group>
key=55 group=6 
locc130=127 hicc130=127 
lopolyaft=127 hipolyaft=127
on_locc130=127 on_hicc130=127  
trigger=release  rt_decay=10
<region> sample=crash1Choke_OH_F_1.wav

// crash2 cymbal
<group>
key=57
group=7 off_by=8
off_mode=normal
ampeg_release=0.6
<region> sample=crash2_OH_P_1.wav

// crash2 choke triggered on polyAT
<group>
key=57 group=8 
locc130=127 hicc130=127 
lopolyaft=127 hipolyaft=127
on_locc130=127 on_hicc130=127  
trigger=release  rt_decay=10
<region> sample=crash2Choke_OH_F_1.wav

Passing test would be:

Send NoteOn 55 127
Send NoteOff 55
Send NoteOn 57 127
Send NoteOff 57
Send PolyAT 55 127 
Send PolyAT 55 0 

It should only choke the ringing cymbal region associated with the note num 55 (and not 57)

Perhaps I can figure out how the test system works and add a new one for this?

[paulfd]

Hey, sorry for how long it takes me each time. It's hard finding chunks of time where I can get back into it.

So re: tests, basically they use Catch, and there are some helpers functions lying around that can help test what you wish. The best is to copy some existing ones. You need to configure with -DSFIZZ_TESTS=ON and then you can run them with e.g. ctest -j16 --output-on-failure --test-dir build.

I like to test the parser extensively, mostly checking either than the correct values are set in the regions properly, or using the OSC messaging to check the values as read. I also have a bunch of tests for choking, polyphony and the likes, which are more or less "easy" to test, full of weird corner cases, and impossible to cover manually. Other than these, I test what I can, if only primitives rather than the output of the synth itself. I had some branches lying around trying to test the output but it's quite complex to do in C++ so I didn't finish them. I had half a mind to do Python bindings just to have access to more comprehensive array processing logic to do this.

Anyway, for this particular PR I felt we could test both the choking behavior you described, as well as triggering on a given polyAT message. For triggering, there's a RegionTriggers test file which can be suited, although I also have some triggering tests in other files. The test looks like this:

TEST_CASE("[Triggers] Respect poly-aftertouch note values when triggering on cc130")
{
    Synth synth;
    synth.loadSfzString(fs::current_path() / "tests/TestFiles/poly_at_trigger.sfz", R"(
        <region> key=55 on_locc130=127 on_hicc130=127 sample=*saw
        <region> key=57 on_locc130=127 on_hicc130=127 sample=*sine
    )");

    synth.polyAftertouch(0, 54, 127);
    REQUIRE(playingSamples(synth) == std::vector<std::string> { });
    synth.polyAftertouch(0, 56, 127);
    REQUIRE(playingSamples(synth) == std::vector<std::string> { });
    synth.polyAftertouch(0, 58, 127);
    REQUIRE(playingSamples(synth) == std::vector<std::string> { });
    synth.polyAftertouch(0, 55, 127);
    REQUIRE(playingSamples(synth) == std::vector<std::string> { "*saw" });
    synth.polyAftertouch(10, 57, 127);
    REQUIRE(playingSamples(synth) == std::vector<std::string> { "*saw", "*sine" });
}

The playingSamples(synth) method sends back a vector of strings with the sample names. I use generators for simplicity. The rest is more or less explanatory.

For the other tests, there are different places it could be in. I have a lot of choking tests in PolyphonyT so I decided to put it here. Here on top of sending the messages, you usually need to do a renderBlock call so voices can be offed and cleaned up when needed

TEST_CASE("[Polyphony] Choking on poly-aftertouch respects the note number")
{
    sfz::Synth synth;
    sfz::AudioBuffer<float> buffer { 2, static_cast<unsigned>(synth.getSamplesPerBlock()) };
    synth.loadSfzString(fs::current_path() / "tests/TestFiles/polyat_choke.sfz", R"(
        <region> key=55 group=1 off_by=2 sample=*saw
        <region> key=55 group=2 on_locc130=127 on_hicc130=127 trigger=release polyphony=1 sample=*sine
    )");
    synth.noteOn(0, 55, 63 );
    synth.renderBlock(buffer);
    REQUIRE( playingSamples(synth) == std::vector<std::string> { "*saw" } );
    synth.hdPolyAftertouch(0, 54, 1.0f);
    synth.renderBlock(buffer);
    REQUIRE( playingSamples(synth) == std::vector<std::string> { "*saw" } );
    synth.hdPolyAftertouch(0, 57, 1.0f);
    synth.renderBlock(buffer);
    REQUIRE( playingSamples(synth) == std::vector<std::string> { "*saw" } );
    synth.hdPolyAftertouch(0, 55, 1.0f);
    synth.renderBlock(buffer);
    REQUIRE( playingSamples(synth) == std::vector<std::string> { "*sine"} );
    synth.hdPolyAftertouch(0, 55, 0.0f);
    synth.renderBlock(buffer);
    REQUIRE( playingSamples(synth) == std::vector<std::string> { "*sine"} );
}

I started from your example, only keeping one region and it's corresponding off region, removing less useful opcodes for this particular test (at least so I thought).

From the tests I realized a couple issues:

• Setting CC triggers actually do not prevent triggering on notes, I kinda forgot about this. So if you remove the trigger=release from the example, the "cymbal" (*saw in the test) gets choked immediately because they're triggered by the same note. On top of this, the sfzformat website states

In ARIA if on_locc/on_hicc and trigger=release or trigger=release_key is used, the on_locc/on_hicc opcode will be effectively disregarded, and the region will behave like a normal release or release_key region.

so I'm curious if i) your example works in Sforzando? and ii) what the MIDI messages look like for this choking behavior?
It seems sfizz departs from sforzando in this behavior.

• When a voice gets choked, it sends back a note-off signal to trigger possible choking sounds. In your example, I believe this would mean the choking sample gets played twice (simultaneously so all you'd hear is a difference in volume). Could you possibly check if this is the case? I needed to add polyphony=1 to get the behavior I expected.
• To avoid problems with some controllers that send multiple CC messages with the same value (IIRC it was some sustain pedal) I had a check that the value for the CC was different. This check caused a problem for the desired behavior if for example you already received a polyAT message on another note, which caused the registered CC130 state to be already 127 when the "correct" polyAT message was received. I changed this check in Layer.cpp to be skipped if the CC is actually polyAT; I'm not super happy with this change, but I'm not sure I have another possibility without changing some of the core MidiState logic to make cc130 very special in this regard. I might need to do this down the line.

[essej]

For the other tests, there are different places it could be in. I have a lot of choking tests in PolyphonyT so I decided to put it here. Here on top of sending the messages, you usually need to do a renderBlock call so voices can be offed and cleaned up when needed

TEST_CASE("[Polyphony] Choking on poly-aftertouch respects the note number")
{
    sfz::Synth synth;
    sfz::AudioBuffer<float> buffer { 2, static_cast<unsigned>(synth.getSamplesPerBlock()) };
    synth.loadSfzString(fs::current_path() / "tests/TestFiles/polyat_choke.sfz", R"(
        <region> key=55 group=1 off_by=2 sample=*saw
        <region> key=55 group=2 on_locc130=127 on_hicc130=127 trigger=release polyphony=1 sample=*sine
    )");
    synth.noteOn(0, 55, 63 );
    synth.renderBlock(buffer);
    REQUIRE( playingSamples(synth) == std::vector<std::string> { "*saw" } );
    synth.hdPolyAftertouch(0, 54, 1.0f);
    synth.renderBlock(buffer);
    REQUIRE( playingSamples(synth) == std::vector<std::string> { "*saw" } );
    synth.hdPolyAftertouch(0, 57, 1.0f);
    synth.renderBlock(buffer);
    REQUIRE( playingSamples(synth) == std::vector<std::string> { "*saw" } );
    synth.hdPolyAftertouch(0, 55, 1.0f);
    synth.renderBlock(buffer);
    REQUIRE( playingSamples(synth) == std::vector<std::string> { "*sine"} );
    synth.hdPolyAftertouch(0, 55, 0.0f);
    synth.renderBlock(buffer);
    REQUIRE( playingSamples(synth) == std::vector<std::string> { "*sine"} );
}

I started from your example, only keeping one region and it's corresponding off region, removing less useful opcodes for this particular test (at least so I thought).

From the tests I realized a couple issues:

• Setting CC triggers actually do not prevent triggering on notes, I kinda forgot about this. So if you remove the trigger=release from the example, the "cymbal" (*saw in the test) gets choked immediately because they're triggered by the same note. On top of this, the sfzformat website states

In ARIA if on_locc/on_hicc and trigger=release or trigger=release_key is used, the on_locc/on_hicc opcode will be effectively disregarded, and the region will behave like a normal release or release_key region.

so I'm curious if i) your example works in Sforzando? and ii) what the MIDI messages look like for this choking behavior? It seems sfizz departs from sforzando in this behavior.

Yes, my example (with a minor change removing the lo/hicc130 opcodes) works as expected in sforzando with midi messages like this (the regions are all loop_mode=one_shot in my example so the early note-offs are basically ignored):

Send NoteOn 55 127
Send NoteOff 55
Send NoteOn 57 127
Send NoteOff 57
Send PolyAT 55 127 
Send PolyAT 55 0 

Where 55 is choked, while 57 keeps ringing

<global>
loop_mode=one_shot

<control>
default_path=Salamander/OH/

// crash1 cymbal
<group>
key=55
group=5 off_by=6
off_mode=normal
ampeg_release=0.6
<region> sample=crash1_OH_P_1.wav

// crash1 choke triggered on polyAT
<group>
key=55 group=6 
lopolyaft=127 hipolyaft=127
on_locc130=127 on_hicc130=127  
trigger=release  rt_decay=10
<region> sample=crash1Choke_OH_F_1.wav

// crash2 cymbal
<group>
key=57
group=7 off_by=8
off_mode=normal
ampeg_release=0.6
<region> sample=crash2_OH_P_1.wav

// crash2 choke triggered on polyAT
<group>
key=57 group=8 
lopolyaft=127 hipolyaft=127
on_locc130=127 on_hicc130=127  
trigger=release  rt_decay=10
<region> sample=crash2Choke_OH_F_1.wav

• When a voice gets choked, it sends back a note-off signal to trigger possible choking sounds. In your example, I believe this would mean the choking sample gets played twice (simultaneously so all you'd hear is a difference in volume). Could you possibly check if this is the case? I needed to add polyphony=1 to get the behavior I expected.

In real-life use (for choking cymbals with e-drums) the lopolyaft=127 hipolyaft=127 are critical to making the desired behavior work right. In sforzando, at least, the above is not causing the choke to play twice simultaneously (verified by looking at voice count)... but I do see that happening in this sfizz branch. I can't explain why it works in sforzando, actually...

• To avoid problems with some controllers that send multiple CC messages with the same value (IIRC it was some sustain pedal) I had a check that the value for the CC was different. This check caused a problem for the desired behavior if for example you already received a polyAT message on another note, which caused the registered CC130 state to be already 127 when the "correct" polyAT message was received. I changed this check in Layer.cpp to be skipped if the CC is actually polyAT; I'm not super happy with this change, but I'm not sure I have another possibility without changing some of the core MidiState logic to make cc130 very special in this regard. I might need to do this down the line.

With actual e-drums, the act of squeezing the cymbal sends a polyAT 127 value, and releasing the squeeze sends a polyAT value 0 (some Alesis kits are dumb and just send the 127, like mine, but I have worked around that with midi event injection elsewhere in the chain, and it isn't sfizz's problem... as it causes issues with all other drum instruments too). I don't know if your fix was actually necessary, at least for my purposes... but I'll have to check. Down the line I think my big per-note expression feature may solve this too...

[paulfd]

In real-life use (for choking cymbals with e-drums) the lopolyaft=127 hipolyaft=127 are critical to making the desired behavior work right. In sforzando, at least, the above is not causing the choke to play twice simultaneously (verified by looking at voice count)... but I do see that happening in this sfizz branch. I can't explain why it works in sforzando, actually...

Interesting. I have the following lines in the "choking" logic:

if (event.type == TriggerEventType::NoteOn)
    noteOffDispatch(delay, event.number, event.value);

I wonder if I'm actually right there or if I'm missing something sforzando does. Basically, a couple possibilities come to mind:

• You're not supposed to send this "fake" note-off if the "choking" voice is triggered by a CC instead of a note signal. Basically, change the above lines to include a test like ... && chokingEventType != CC.
• You're not supposed to send this fake note-off if the original note is already "off" in the midi state, so adding ... && && resources_.getMidiState().isNotePressed(event.number)

Both of these pass all my existing choking tests, so I'd not break anything I know by including either of them. I'm just not sure which one is true. I need @kinwie 😆

[kinwie]

Hi Paul, and essej,

I'm also kinda lost in time about this feature details, so I'm slowly catching up here :)

First thing I would like to explain is about the use of trigger=release.
Setting CC triggers actually do not prevent triggering on notes, I kinda forgot about this. So if you remove the trigger=release from the example, the "cymbal" (*saw in the test) gets choked immediately because they're triggered by the same note. On top of this, the [sfzformat website](https://sfzformat.com/opcodes/on_loccN) states

The release trigger was used in that example for utilizing the rt_decay function only, It is true that the choking sample will play when it receive the on_locc130 and on_hicc130 opcodes command regardless the need of trigger=release.
But since the choke sample for drum cymbal is usually a tail sample only and has volume decreasing depending on how long the previous cymbal was hit then grabbed (in real acoustic drum playing). So, with such limitation of current opcodes we have for now, only rt_decay can serve the volume decreasing needed in sfz programming, thus, trigger=release was added there.
Those examples works in sforzando and the rt_decay is functioning as well, but of course it might be not perfect yet to get this feature implemented well in sfizz, so i would like to find any holes within this.

I haven't test the latest sfizz build to make more details checks for this. I would like have this build release, but still, I'm at Win7 32 OS :). Please let me know if the release is available. I'll keep an eye for this. Thanks, Paul

[paulfd]

Hey @kinwie, sorry for now I have no solution to build sfizz vst for Win7... Maybe if I have time I may spin up another plugin that uses something compatible :(

For now I went with solution 1, the test is still there so if experience proves us wrong we can change it.

[Sylv1o]

Hi,

I've done some tests today and it works quite good now. The polyAT respond to its corresponding note number.
The only problem I still see is that the mute tail sample can be triggered more than once... I explain :
You hit your cymbal, then you choke it. The cymbal sound is stopped and the mute tail sample is triggered, with a velocity that depend on the RT_decay value, that's perfect.
But if you choke it again, the mute tail sample is triggered again (if still inside the RT_decay value)... It should not. The cymbal sound is not playing anymore, we should not hear any mute tail sound when grabbing the cymbal...

The mute tail sample should only be triggered while the cymbal sample is still played. And be careful, it's different than "while note is held" cause with Edrum, the noteOff is always sent about 100ms after the noteOn.

This is the test snippet :

<global> loop_mode=one_shot pitch_keytrack=0

<Master>  
<region>  
key=69
off_by=1
sample=CymbalTest-004.wav

<region>
group=1 
key=69	

locc130=127 hicc130=127 
on_locc130=127 on_hicc130=127	
trigger=release rt_decay=10	
sample=CymbalChokeTest-005.wav