Skip to content
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

Closed
wants to merge 16 commits into from

Conversation

PythonBlue
Copy link
Contributor

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.

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
@paulfd
Copy link
Member

paulfd commented Oct 16, 2023

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?

@PythonBlue
Copy link
Contributor Author

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?).

@redtide
Copy link
Member

redtide commented Oct 16, 2023

Maybe better here than on Discord:
some opcodes might be missing on sfzformat: the opcode list on sfz.tools uses the "same" data as sfzformat (a copy updated time to time), so if an opcode is missing is just not included in the sfzformat database. Other than that might be that some opcodes marked as unsupported are already implemented, just not keep updated in the sfz.tools database.
So if someone find some error please report in the appropriated website issue tracker, sfzformat or sfztools.

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.
@paulfd
Copy link
Member

paulfd commented Dec 26, 2023

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.
@PythonBlue
Copy link
Contributor Author

PythonBlue commented Dec 26, 2023

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;
Copy link
Member

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.

Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants