-
Notifications
You must be signed in to change notification settings - Fork 761
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
Made performKeyToDegree much closer to inverse of performDegreeToKey #1164
Conversation
I've made the patch now.... Unfortunately I've just noticed it has issues with some scales, specifically Scale.ajam. Seems to work perfectly with the scales I'm more familiar with though. Scale.minor, major, dorian and all the other modes. Would you mind showing me how to throw together a Unit test for this using your framework? I'm not likely to get the chance to fiddle with your unit testing library for a week or so. |
this example should be pretty easy to understand: https://github.com/supercollider-quarks/CommonTests/blob/master/TestArray.sc you just subclass UnitTest and write test_methods { } that repository is out of date, its just an example because its easy to On Wed, Aug 6, 2014 at 4:02 PM, Tristan Strange notifications@github.com
|
any news on this? Do you have a simple test file? |
It sounds like this changes behavior, and this is a very musical method that is often used. We can't change it without unit tests that confirm that we won't have herds of angry musicians telling us that we embarrassed themselves in public (because they updated the night before a gig) |
If this changes behavior, I'd share Felix's concern. I'd suggest taking the time to draw up a reasonably complete feature spec before merging anything. "Make it behave more like an inverse operation" is awfully vague and invites dispute... but if we have a spec, then there's something written down to explain to current and future users why it's this way and not another way. SC development almost never begins with gathering requirements and speccing features... fine until something breaks. It would be nice to do without the temporary array. This is basically a math op, which should avoid unnecessary load on the garbage collector. |
@@ -594,7 +594,7 @@ SequenceableCollection : Collection { | |||
^if(accidental == 0) { baseKey } { baseKey + (accidental * (stepsPerOctave / 12.0)) } | |||
} | |||
|
|||
performKeyToDegree { | degree, stepsPerOctave = 12 | | |||
performKeyToDegree { |key stepsPerOctave = 12| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comma?
See issue #1163