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

Synths winwood_lead, bass_foundation, and bass_highend #2628

Merged
merged 2 commits into from
Apr 27, 2021
Merged

Synths winwood_lead, bass_foundation, and bass_highend #2628

merged 2 commits into from
Apr 27, 2021

Conversation

level-xx
Copy link
Contributor

Synths winwood_lead, bass_foundation, and bass_highend, adapted from "Steal This Sound", each with raw and compiled synthdef files, and an updated synthinfo.rb.

Copy link
Collaborator

@ethancrawford ethancrawford left a comment

Choose a reason for hiding this comment

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

Hey hey. Looking good so far! there are a few comments that would be good to address, but I think this is fairly close! 👍

},
:lfo_rate =>
{
:doc => "Width of the low-frequency oscillator (LFO) which determines how fast base tones oscillate around their base frequencies",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This docstring was presumably meant to be updated?
Also, just to keep in mind for the new docstring, most of the time when we've talked about opts that are measured in units of time, we've done so in beats. (I say most because I think there are a few exceptions here and there).
@samaaron - do we want to talk about rate in an lfo_rate docstring in terms of beats per second rather than Hz?
Perhaps something like:

Suggested change
:doc => "Width of the low-frequency oscillator (LFO) which determines how fast base tones oscillate around their base frequencies",
:doc => "Rate in beats per second that the low-frequency oscillator (LFO) oscillates between its low and high values",

?

Copy link
Contributor Author

@level-xx level-xx Jan 14, 2021

Choose a reason for hiding this comment

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

Oops, sure, that was supposed to be "rate", of course. As to the unit: The synthdef code actually uses Hz. I won't be very passionate about it, but the idea was that Hz would be the right thing here. As far as I can see we have no configurable LFO in other synths and fx yet, so there is no precedence. SP synths and fx deal with 2 types of frequencies up to now: the note and the beat. I suppose, neither of them really fits. If you consider we should go with either note or beats, then I think beats will infact be the better choice.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@samaaron - would value your input on this one when you are able.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this depends on whether this rate is more closely tied to the sound made by the synth, or the rhythm of the surrounding piece. I.e. if you were to change the tempo of the music without changing the notes, would you expect this rate to increase with the tempo, or to stay constant?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@samaaron - would value your input on this one when you are able.

As mentioned before in this thread, this all looks great but I'm currently 100% focussed on getting the new release out. I'm happy to get back into this conversation then :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@emlyn: I'd say it should stay constant. The effect of the LFO is similar to those in a vibraphone or a hammond organ.

end

def doc
"A lead synth inspired by the Winwood songs from the early 80s. Adapted for Sonic Pi from [Steal This Sound](https://raw.githubusercontent.com/supercollider/supercollider/develop/examples/demonstrations/stealthissound.scd). Published there under [GPL v3](https://www.gnu.org/licenses/gpl-3.0.en.html), so re-published under the same terms. The source code is available [on the Sonic Pi GitHub repository](https://github.com/sonic-pi-net/sonic-pi). Date of modification: 10.01.2021"
Copy link
Collaborator

@ethancrawford ethancrawford Jan 13, 2021

Choose a reason for hiding this comment

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

If I were to be really picky, is an average user going to need to know directly from the GUI's synth docs exactly what license it is and how or where to find the source code? We could leave notes like that in the sclang file maybe 🤷‍♂️ (perhaps it doesn't really matter if we leave it in, but I thought this was a reasonable question to ask 😅 🤷‍♂️)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was wondering about that, too, but I have tried to comply with GPL v3, and that license explicitly requires that:

"5 d) If the work has interactive user interfaces, each must display Appropriate Legal Notices; however, if the Program has interactive interfaces that do not display Appropriate Legal Notices, your work need not make them do so."

Copy link
Collaborator

Choose a reason for hiding this comment

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

@samaaron - shall we leave the license and source code link in the synth doc here given the above?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We already have an explicit user interface component that shows that the app is distributed under a GPL license which is built into the GUI under a section called "License". I'm assuming that this is sufficient and we don't have to explicitly document it in the built-in documentation - although I do like having a link to the original designs so people can see the provenance.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@level-xx would you be ok removing all the license stuff from the docs, keeping a URL to the original project and then adding an entry to the License.md file covering these synths?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, that's the best way. I was unaware of the license component. Just to avoid creating a mess ... I have noticed you have changed the base branch from main to dev. So, I would pull, merge my previous changes into dev, make the new license changes, and push to dev, correct?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@level-xx @samaaron I suggest using GPL for the synthdefs and MIT for the synth metadata and documentation as Sam suggested in his recent comment on this PR. Regarding the target of this PR - you can just make a new PR with all the adjusted code ready to go, and targeted at dev 🙂

:lfo_width =>
{
:doc => "Width of the low-frequency oscillator (LFO) which determines how wide base tones oscillate around their base frequencies",
:modulatable => true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does lfo_width: need validations? (same with other non-default opts in these synths)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, yes, thanks, both lfo_rateand lfo_width should be non-negative:

:validations => [v_positive(:lfo_rate)],

and

:validations => [v_positive(:lfo_width)],

in their corresponding sections.

Copy link
Collaborator

@ethancrawford ethancrawford Jan 18, 2021

Choose a reason for hiding this comment

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

Agreed for :lfo_width, but I think maybe :lfo_rate should be greater than zero?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just checked that in SuperCollider: :lfo_rate is fed into an LFTrioscillator as the frequency argument. Zero is OK and doesn't break it. It gives a flat line, of course. For practical reasons, I'd allow zero. This way, for example, the user can slide down the rate to zero without causing a validation error.

@samaaron
Copy link
Collaborator

Looks ace, but right now I'm 100% focussed on finishing off v3.3 and getting it out of the door (I was hoping to have had it released by now, but some last minute MIDI niggles still need to be sorted).

As soon as I've done that, I can come back here and look in to merging this into the code base :-)

Thanks once again!

@samaaron samaaron changed the base branch from main to dev February 12, 2021 17:00
@samaaron samaaron merged commit 0c1ea59 into sonic-pi-net:dev Apr 27, 2021
@samaaron
Copy link
Collaborator

Merging this now - I can tidy up any niggling issues later.

@level-xx - thank-you so much for this - it's going to make a fabulous addition to the next version :-)

@samaaron
Copy link
Collaborator

Apologies - I hastily merged this without checking more thoroughly to see these comments.

I think this would be a great addition, but having it as GPL doesn't match our current licensing as MIT. I think we therefore need to be very careful here to ensure things are proper and correct.

I think it makes sense to make sure that the synthdefs are licsensed as GPL but the synth metadata and documentation (Ruby stuff) is MIT.

If you could clarify this, and address @ethancrawford's points above - I'd be happy to merge in a new PR with this work :-)

@level-xx
Copy link
Contributor Author

@samaaron -- No worry, actually I have had job change and since have not been able to make the changes that you suggested in the merge request, so my fault anyway. I'll still be busy for some weeks now, but I sure want to fix the license issues and re-submit the synths for a future release. I am also working on a tonewheel synth, and I hope to get it all done in summer.

@samaaron
Copy link
Collaborator

@level-xx that's great - and congrats on the job change. No rush from me, but happy to get things rolling just as soon as you have the time and energy :-)

@samaaron
Copy link
Collaborator

samaaron commented Oct 13, 2021

Hi there @level-xx, I just had a play with the new synths - I think they're going to be a lot of fun.

One thing I noticed is that they don't appear to behave in a similar way to the existing synths with respect to the ADSR envelope. For example, with the built in synths I can do:

play :e1, release: 0.1

which will play the note E1 for 0.1 beats. The attack, decay, and sustain therefore default to 0 by default.

This is unfortunately not the case here:

use_synth :winwood_lead
play :e1, release: 0.1, amp: 5

It might just be a case of tweaking the defaults in the synthinfo.rb file. However, it might be possible that some slight tweaks to the design of the synth are necessary. For example, this makes no sound at all:

use_synth :bass_foundation
play :e1, release: 0.3, amp: 5, sustain: 0

I suspect :bass_foundation doesn't even respond to release?

Do you think it would be possible to tweak things to match the default behaviour?

@ethancrawford
Copy link
Collaborator

ethancrawford commented Oct 13, 2021

I suspect :bass_foundation doesn't even respond to release?

I think it does, but my guess is that it's because the SynthDefs have hardcoded filter envelopes? Perhaps there's a possibility they could either be made proportional to the main amplitude envelopes, and/or configurable with opts of their own? (such as we do with the :tb303 synth etc)? thoughts @level-xx?

@level-xx
Copy link
Contributor Author

It does take the standard ADSR params, for both length and level. Back to the testbed :) I'll check that and get back here asap.

@level-xx
Copy link
Contributor Author

Hi @samaaron and @ethancrawford, I've just taken a look and it, and it seems alright, i. e. I don't think there's anything technical wrong. I believe it's just the default values.

... The attack, decay, and sustain therefore default to 0 by default.

This is unfortunately not the case here:

use_synth :winwood_lead
play :e1, release: 0.1, amp: 5

I assume, this

use_synth :winwood_lead
play :e1, attack: 0, sustain: 0, decay: 0, release: 0.1, amp: 5

does create the sound you expect. This is so, because winwood_lead does currently not have 0 by default for attack, decay, sustain.

... For example, this makes no sound at all:

use_synth :bass_foundation
play :e1, release: 0.3, amp: 5, sustain: 0

Well, it actually makes some undesired kick sound :) The reason here is also in the defaults. I have noticed now, that the existing synths have the sustain_level set to 1, while the bass synths have 0.

use_synth :bass_foundation
play :e1, release: 0.3, amp: 5, sustain: 0, sustain_level: 1

does behave as desired. So I think, setting the defaults correctly in the 3 of them should fix things up. That would be setting attack, decay, sustain to 0, as well as setting sustain_level and attack_level to 1, while decay_level is -1, apparently as flag for »equal to sustain_level«. That will require some testing, as -1 will currently throw an option value of range error. I'll make a new PR with the updated synthdefs and docs until the weekend.

BTW: The build on Ubuntu 20.04 went smooth, but I did get some warnings. Would it be good to post them here somewhere?

@level-xx
Copy link
Contributor Author

Submiited new PR last night.

@ethancrawford
Copy link
Collaborator

@level-xx - up for adding a few small extra changes relating to this? Your synths are now being released with version 4.0, not version 3.3 as previously added - also, you could add new synth items to https://github.com/sonic-pi-net/sonic-pi/blob/dev/CHANGELOG.md (in the same style as previous others) if you like 🙂

@ethancrawford
Copy link
Collaborator

Also, I think here you refer to lfo_rate as being the 'width' of the LFO - that was perhaps unintentional?

:doc => "Width of the low-frequency oscillator (LFO) which determines how fast base tones oscillate around their base frequencies",

@level-xx
Copy link
Contributor Author

Hi, just submitted the PR. Thanks for spotting this!

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.

None yet

4 participants