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

Made performKeyToDegree much closer to inverse of performDegreeToKey #1164

Closed
wants to merge 3 commits into from

Conversation

triss
Copy link
Contributor

@triss triss commented Aug 6, 2014

See issue #1163

@triss
Copy link
Contributor Author

triss commented Aug 6, 2014

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.

@crucialfelix
Copy link
Member

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
link to github repos.

On Wed, Aug 6, 2014 at 4:02 PM, Tristan Strange notifications@github.com
wrote:

I've made the patch now....

Unfortunately I've just noticed it has issues with some scales,
specifically Scale.ajam which has 24 note per octave I think.

Seems to work perfectly for twelve notes per octave though. I can't fathom
why it's an issue when the number is bigger.

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.


Reply to this email directly or view it on GitHub
#1164 (comment)
.

@telephon
Copy link
Member

any news on this? Do you have a simple test file?

@crucialfelix crucialfelix added this to the 3.8 milestone Mar 6, 2016
@crucialfelix
Copy link
Member

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)

@jamshark70
Copy link
Contributor

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|
Copy link
Member

Choose a reason for hiding this comment

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

comma?

@vivid-synth vivid-synth added the comp: sclang sclang C++ implementation (primitives, etc.). for changes to class lib use "comp: class library" label Jan 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp: sclang sclang C++ implementation (primitives, etc.). for changes to class lib use "comp: class library" waiting for testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants