-
-
Notifications
You must be signed in to change notification settings - Fork 58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update Region.cpp to support per-voice CC's #1173
Conversation
… the extended CC's with filter cutoff: the CC checks within the changed file assume unconditionally that the modulators are per-cycle controllers (Controller) whereas most extended CC's are per-voice (PerVoiceController), so added conditional statements to reflect that.
Fix applied to the CC processing: too many CC values were specified to be per-voice originally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, thanks a lot. Are there any examples of this and the desired behavior? Especially something we could test would be amazing...
src/sfizz/Region.cpp
Outdated
@@ -1692,6 +1692,10 @@ bool sfz::Region::processGenericCc(const Opcode& opcode, OpcodeSpec<float> spec, | |||
auto it = std::find_if(connections.begin(), connections.end(), | |||
[ccNumber, &target](const Connection& x) -> bool | |||
{ | |||
if ((ccNumber > 130 && ccNumber < 138) || ccNumber == 140 || ccNumber == 141) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we keep it like this maybe it would make sense to add an isPerVoiceController
function to the extended CC list to replace both this check and the switch below in this method. However we can plan to do that after.
@@ -1692,6 +1692,10 @@ bool sfz::Region::processGenericCc(const Opcode& opcode, OpcodeSpec<float> spec, | |||
auto it = std::find_if(connections.begin(), connections.end(), | |||
[ccNumber, &target](const Connection& x) -> bool | |||
{ | |||
if ((ccNumber > 130 && ccNumber < 138) || ccNumber == 140 || ccNumber == 141) | |||
return x.source.id() == ModId::PerVoiceController && | |||
x.source.parameters().cc == ccNumber && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we check the region id here too in this case? If not, I'd suggest just modifying the initial check to x.source.id() == ModId::PerVoiceController || x.source.id() == ModId::Controller
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the critiques, Paul. Insofar as this code and the one with the corresponding suggestion, good thought about the region checking, I suppose. Beyond that technicality with this particular one, I'll let you be the judge over how to format the syntax exactly: I fully admit code optimization is not my forte.
Sorry for my delayed response, btw.
src/sfizz/Region.cpp
Outdated
@@ -1851,7 +1855,12 @@ sfz::Region::Connection& sfz::Region::getOrCreateConnection(const ModKey& source | |||
sfz::Region::Connection* sfz::Region::getConnectionFromCC(int sourceCC, const ModKey& target) | |||
{ | |||
for (sfz::Region::Connection& conn : connections) { | |||
if (conn.source.id() == sfz::ModId::Controller && conn.target == target) { | |||
if (conn.source.id() == sfz::ModId::PerVoiceController && conn.target == target) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, it feels like this could be a single if
statement.
a382e43
to
c164057
Compare
Figured out my issue of why I had to deviate so much with filter cutoff CC curves: it turns out the file I changed originally assumed all CC's are per-cycle controllers, whereas most of the extended CC's, including the ones I was working with, are per-voice controllers, so I added conditional statements for if it's one of those extended CC numbers. Should positively affect other targets that make use of complex CC modulation as well.