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

Replace keyToDegree2 with keyToDegree #11

Open
totalgee opened this issue Jan 11, 2020 · 1 comment
Open

Replace keyToDegree2 with keyToDegree #11

totalgee opened this issue Jan 11, 2020 · 1 comment

Comments

@totalgee
Copy link

In the code, there are various comments about replacing keyToDegree2 by keyToDegree (and removing performKeyToDegree2).

The comment says:

// TODO won't be required in next SC release see pull request #1164

So I was thinking about making a PR on this repo to remove this (six years have passed ;-), but as far as I can tell, nothing has changed in SC related to this...your SC submissions from 2014 (1163 and 1164) were unfortunately never integrated into SC.

I know it's related to using offsets of 0.1 vs 0.5 for flattened and sharpened degrees:

ChordSymbol.asDegrees(\Cm7)    // (uses keyToDegree2; same as:)
ChordSymbol.asNotes(\Cm7).collect(_.keyToDegree2(Scale.major))
// --> [ 0.0, 1.1, 4.0, 5.1 ]
ChordSymbol.asNotes(\Cm7).keyToDegree(Scale.major)
// --> [ 0.0, 1.5, 4.0, 5.5 ]

And if we were to use the existing keyToDegree, then the resolution of those degrees (by the note Event) would return bad notes...

n = [0, 3, 7, 10];
Pbind(\degree, Pseq(n.collect(_.keyToDegree2(Scale.major)))).asStream.all(Event.default).do(_.use{"deg: % note: %".format(~degree, ~note.value).postln});
// deg: 0.0 note: 0.0
// deg: 1.1 note: 3.0
// deg: 4.0 note: 7.0
// deg: 5.1 note: 10.0

Pbind(\degree, Pseq(n.keyToDegree(Scale.major))).asStream.all(Event.default).do(_.use{"degree: % note: %".format(~degree, ~note.value).postln});
// degree: 0.0 note: 0.0
// degree: 1.5 note: -1.0
// degree: 4.0 note: 7.0
// degree: 5.5 note: 6.0

Maybe the comments should just be updated to state why keyToDegree2 is needed, and no longer suggest that it will be replaced by keyToDegree imminently (it seems it won't be!).

@triss
Copy link
Owner

triss commented Jan 12, 2020

This sounds like a good move. It sounds like people really aren't keen to change the implementation.

Sadly, I don't find much time for SC at the moment.

Please feel free to submit a PR that updates the comments.

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

No branches or pull requests

2 participants