-
-
Notifications
You must be signed in to change notification settings - Fork 57
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
CC Curve support for standard EG's #1194
Conversation
Fix applied to the CC processing: too many CC values were specified to be per-voice originally.
Merged changes in the fork and the main repo
For this particular pull request, I'm happy to say I managed to pull off curves for CC-based modulation of ADSR envelope times. Fully tested with amp EG's before committing, and theoretically pitch and filter EG's should work the same.
Replaced references to all resources for EG stages with just the curves, and some other fixes for easy merging
Forgot to remove references in my full branch
Forgot to implement changes for the Region.cpp file, and also implemented the changes for the Tests for the sake of the whole project building properly.
Fixed SynthMessaging pointing to the new envelope stages.
Incorrect default value for ccSustain fixed
That's a great feature thanks, and the implem looks good at a glance. I'll expand the existing tests to account for this capability. Is this actually in the spec? |
All of these opcodes are already implemented in ARIA, though your question raises an interesting point that only some of them are listed at all on the sfizz website's opcode page (the one for ampeg attack is missing, for instance?). |
Maybe better here than on Discord: |
While testing a problem I had with an earlier build, that turned out to be an SFZ parsing issue, I decided to apply some code changes to further enforce that custom curves will be read.
Just noticed the SFZ parser wasn't programmed to read some of them in this branch. Fixed with this commit.
Hey @PythonBlue, is this branch in good shape? I can probably fix the tests and look it over shortly if so! |
Last minute commit before possible merging into main repo. Fixed a type specification issue for the new CC variables that would cause tests to fail, and likely issues with negative values as well.
Thanks for the reminder about this PR. I THINK I just fixed the tests issue, though I guess we'll find out in a few minutes, but either way, it should be better this way anyway because I realized part of the issue was how the CC Sustain and Start vaues were made. |
CCMap<float> ccStart; | ||
CCMap<float> ccSustain; | ||
CCMap<ModifierCurvePair<float>> ccSustain { ModifierCurvePair<float>{ Default::egPercent, Default::curveCC } }; | ||
//CCMap<float> ccAttack; |
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.
These could probably go away.
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 you're referring to what I commented out, fair point. I just kept it in case revertng was desired.
Implemented a crude approach to supporting CC curves by adding the CurveSet resources to the EG's.
The following opcodes should now be supported with this branch:
ampeg_attack_onccN
ampeg_decay_onccN
ampeg_delay_onccN
ampeg_hold_onccN
ampeg_release_onccN
ampeg_sustain_onccN
fileg_attack_onccN
fileg_decay_onccN
fileg_delay_onccN
fileg_hold_onccN
fileg_elease_onccN
fileg_sustain_onccN
pitcheg_attack_onccN
pitcheg_decay_onccN
pitcheg_delay_onccN
pitcheg_hold_onccN
pitcheg_release_onccN
pitcheg_sustain_onccN
Also, apologies for all the constant re-requests: main issue was my failing to realize I copied all of the files I should have for the branch.