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

LFO #338

Merged
merged 11 commits into from
Aug 13, 2020
Merged

LFO #338

merged 11 commits into from
Aug 13, 2020

Conversation

jpcima
Copy link
Collaborator

@jpcima jpcima commented Jul 28, 2020

This adds SFZv2 LFO on top of the mod matrix PR.

There are the basic targets for now (amplitude, pitch, etc)

Simple usage example

<region>
sample=*sine
lfo1_freq=1.0
//lfo1_amplitude=100
lfo1_pitch=100

@jpcima jpcima force-pushed the mm+lfo branch 3 times, most recently from b928712 to 012aadb Compare July 28, 2020 18:29
src/sfizz/LFO.cpp Show resolved Hide resolved
src/sfizz/LFO.cpp Outdated Show resolved Hide resolved
src/sfizz/LFO.cpp Show resolved Hide resolved
src/sfizz/Region.cpp Show resolved Hide resolved
src/sfizz/Voice.cpp Show resolved Hide resolved
@paulfd
Copy link
Member

paulfd commented Aug 11, 2020

I added some tests, trying to automate them also on travis. It may fail, bear with me :)

@jpcima
Copy link
Collaborator Author

jpcima commented Aug 12, 2020

On reference data points, may we possibly flac them up to reduce their size?

Copy link
Member

@paulfd paulfd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if you're certain about the phase I guess we need to amend sfzformat.com, and if my solution for the reference sampling point is OK, I'm good with the rest 🙂

@jpcima
Copy link
Collaborator Author

jpcima commented Aug 12, 2020

Last commit introduces a terrible hack for Garritan instrument compatibility.

  • if lfoN_phase is 0<=x<=1, it's considered with the ARIA meaning which is 0-1 normalized
  • otherwise, it's taken as degrees, as standard SFZ suggests
  • if the user adds trailing text to the value, this forces a particular choice of unit, eg. 0.5d → 0.5 degrees

@paulfd
Copy link
Member

paulfd commented Aug 12, 2020

This seems like a reasonable solution to a messy problem!

@alcomposer
Copy link
Collaborator

alcomposer commented Aug 12, 2020

lfoN_phase_onccX would then follow the SFZv2 standard, or use both as well?

@jpcima
Copy link
Collaborator Author

jpcima commented Aug 13, 2020

I have done some changes as follows to help make the work more mergeable

  • control all of plot_lfo settings by arguments
  • squashed the test commit and moved on top
  • postponed the ARIA compatibility work, kept in a side branch; there is discussion to have on this topic, as every proposed solution has its advantages and inconvenients, and I'm not interested to take the decision here, just advance LFO for coming work

On the tests, I wish to have them preferably included in the regular test suite, I will add some work towards this direction.

The reference files have been visually validated

Tweaks for the LFO regression test
@jpcima
Copy link
Collaborator Author

jpcima commented Aug 13, 2020

Regression tests are moved into the main test suite. Good for merge.
Do we keep these numpy files?

@paulfd
Copy link
Member

paulfd commented Aug 13, 2020

I like the plotting one but it can go somewhere else. I'm not used to whipping out gnuplots on the fly :)

We still need something like it to validate changes visually and possibly update the reference.

@jpcima jpcima merged commit 0934172 into sfztools:develop Aug 13, 2020
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