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

Teach piano synth how to play fractional notes #2041

Merged
merged 7 commits into from May 17, 2019

Conversation

Projects
None yet
3 participants
@emlyn
Copy link
Contributor

commented Mar 22, 2019

The MdaPiano ugen used by Sonic Pi's piano synth, although it only accepts integer notes, actually has a separate tune parameter that allows it to play fractional notes.

I've rewritten the piano synthdef in SuperCollider's language, hopefully copying all the functionality of the existing one, and also wiring up the tune parameter to the fractional part of the note (thanks for your help @samaaron & @ethancrawford).

Let me know if anything is not correct, as I'm still a Supercollider noob.

One thing in particular I wasn't sure about is the vel parameter. In the current piano synth it scales the input range by 4 ([0-1] -> [0-4]), then multiplies it by 127, giving it a final range of 0-508 (here), although the docs say it should be 0-127. Is this a bug, or have I misunderstood what lin-lin does (why is it done in 2 steps - linlin followed by multiplication)?

@ethancrawford

This comment has been minimized.

Copy link
Contributor

commented on etc/synthdefs/designs/piano.scd in 15d042a Mar 22, 2019

Does the decay_level = -1 actually assign -1 to decay_level, rather than compare? Not sure.
One thing that I seemed to have bumped up against when using Select (unless I have just been mistaken somehow) was that I couldn't use == in a comparison here either as it produced an error where it expected a numerical value but was seeing a Boolean. However, using < 0 seemed to work fine. 🤷‍♂️

This comment has been minimized.

Copy link
Contributor Author

replied Mar 22, 2019

Good catch, I'll replace it with < 0.

@ethancrawford

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2019

That vel scaling seems like a bug to me 🤷‍♂️ what does it sound like with the initial scaling taken out?

@ethancrawford

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2019

One other thing to note - for the control parameters that are defined at the start of the SynthDef (between the |) you have corresponding local variables in the body of the synth (declared with var and ending with an underscore). I don't know whether there's a specific convention for this, but I haven't usually added these matching local variables - I've just used the control parameters directly in any expressions in the SynthDef body, as it saves on a small amount of typing and avoids creating seemingly unnecessary variables. I guess it's maybe down to personal preference 🤷‍♂️

@emlyn emlyn force-pushed the emlyn:piano-tuning branch from 0abea0c to e3232af Mar 22, 2019

emlyn added some commits Mar 22, 2019

@emlyn

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

If I just remove the factor of 4 it goes very quiet, but if I then scale the default back from 0.2 to 0.8 then it sounds identical to before. The docs say 100 is the default, and 0.8 * 127 ~= 100, so I've gone with that for now.

In the Overtone version there's the let block that reuses the names of the parameters, and I was trying to replicate that, but SuperCollider won't let me reassign a parameter, nor reuse the name as a local variable, hence the underscores to get as similar a name as possible.
I was worried that putting everything into one big expression would get unreadable, but I think i went a bit too far the other way, so I've cleaned it up and reduced the number of vars.

@ethancrawford

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2019

That's interesting... I have things like this in mine:

  {
    |note = 52,
   ...

and later, in the synth body:

    note = VarLag.kr(note, note_slide, note_slide_curve, note_slide_shape);

and that works fine 🤷‍♂️

@ethancrawford

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2019

Also, I think the number of intermediate variables before the last change was fine, I was just not sure the underscores were necessary is all 🤷‍♂️

@emlyn

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

Oh, I misunderstood, when you said

I've just used the control parameters directly in any expressions in the SynthDef body, as it saves on a small amount of typing and avoids creating seemingly unnecessary variables

I thought you were suggesting to use fewer intermediate variables.
And I think I've worked out what was going on when I couldn't reassign parameters before: I hadn't realised that all local vars had to be declared at the top before any other statements, and I'd put the assignment before a var declaration.

So now that I can reassign parameters I don't need so many vars, and no underscores :-) I think it's looking better now.

@emlyn

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

Just realised I had pushed a commit from before I'd finished tidying things up, so added the rest.

@samaaron

This comment has been minimized.

Copy link
Owner

commented Mar 25, 2019

This all looks fab. I'll take a look in detail when I get a moment. Thank-you so much for your effort with this!

@emlyn

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2019

I was just playing with this and found out that this synth (or any others I've tried loading via load_synthdefs) doesn't recognise notes specified as symbols (e.g. play :e4 prints synth :testPiano, {note: 0.0} in the log).

I'm not sure if this is something that needs to be handled in the synth, or if it's a Sonic Pi bug? I would have thought it makes sense for Sonic Pi to do the translation itself so that it doesn't have to be done in every synth, but maybe there's a reason it doesn't/can't.

@ethancrawford

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

Maybe the automatic note translation is a currently un-handled feature for external synths, but I'd have to dig into the code a little to confirm that.

@samaaron

This comment has been minimized.

Copy link
Owner

commented Apr 1, 2019

Sonic Pi specifically does no fancy translation for your custom synths as it can't know in advance if your 'note:' option is in MIDI or Hz.

The built-in synths pass through a system of mapping functions and checking functions which are defined as part of the metadata for each synth. This is currently not open enough. In the future it would be amazing to consider a synth format which included the synth binary, design and metadata in one zip file which could be easily shared.

One benefit from this approach could be to also tie in the synth opts from the metadata to the autocompletion system.

@ethancrawford

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

Yeah, thought so. @emlyn @samaaron the packaged synths are on my todo list 🙂

@emlyn

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2019

Ah that explains it. I was a bit confused for a while when I thought this synth had stopped working, until I realised I was using note symbols, but when testing it initially I had been using (fractional) midi notes.
Packaged synths seem like a great idea, that would make external synths much nicer to work with.

@samaaron samaaron merged commit 5a15b84 into samaaron:master May 17, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@samaaron

This comment has been minimized.

Copy link
Owner

commented May 17, 2019

Fab stuff, thanks!

@emlyn emlyn deleted the emlyn:piano-tuning branch May 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.