exchanged KBM constructor for default Tuning constructor

[baconpaul]

OK! Yeah I see why you need this change (no rawText on the unmapped KeyboardMapping) but this change isn’t quite right. The default KeyboardMapping constructor has note0 as 60 and frequency0 as 32MIDI_FREQ_0 or whatever the constant is, so rather than having the second argument be “tunA69to(440)” it should be “tuneNoteTo( 60, 32 MIDI_NOTE_0 )” (or close to that - sorry I’m on ipad not computer).

That should - in theory - make the test exact again so you can remove the Approx

And this would actually make a difference for synths which use the non-KBM constructor type (which I think sfizz does) in that if you load a short scale (say a scale with period 5) instead of note 60 being 251 you would have tuned note 60 way way down a couple of octaves.

Happy to merge with those changes though. Thanks for fixing the underlying problem!

[baconpaul]

Also the fact that we don’t have a test in the library that fails in this case means our test suite is incomplete. The real test is take one of the short scales from the library (I think we have some 5 and 6 note ones), apply them to the tuning without a mapping, and simply assert that midi note 60 is 32 * MIDI_FREQ_0 (which is 261.2). That should pass without your change; should fail with your change, and should pass once you have it correct. I can bang that test in this morning if you want or you can do it with this diff or I can do it after. Just lemme know!

[TheWaveWarden]

Ah yeah, I forgot that this makes a difference when using scl as well. Thanks.

I just played around with the tests for an hour: It seems tuneNoteTo( 60, MIDI_0_FREQ * 32.0 ), does not produce an identical KBM as KeyboardMapping() does. When diffing them, the resulting differences are:

tuningPitch: 32 vs 32.00001 octaveDegrees: 12 vs 0

This apparently results in a lot of failed tests, due to slight differences, especially when not only replacing in Tuning(), but also Tuning(Scale), which should be done for consistency really. I am now unsure how to proceed:

• fix all those tests manually with an Approx
• somehow make both constructors generate identical classes.
• leave as it was to begin with (everything I want to do can be done already, just use different constructor)

Note also that tuneNoteTo() generates a string, which then gets reparsed into the class members, which seems convoluted.

[baconpaul]

Yeah I am thinking the right answer is to have the kbm default constructor set the raw text to something reasonable rather than do the un parse

The reason I haven’t run into this is I keep a mapping and a state flag both in my synth so don’t use or stream the mapping string if you haven’t set one but that’s ugly state management and your solution is better indeed.

So let me look later today ok? I’ll push something up with a fix this weekend. For now maybe just whack your local copy so you can keep dev on odinngoing and then you can pull when I’ve got it?

[TheWaveWarden]

Yeah we can do that! It is not an urgent matter though - I can do everything I want to with the main branch right now.

My intention was to contribute something at least, not to make it usable in the first place.

[baconpaul]

I have it fixed in my user area and turns out we have another edge case bug I need to fix anyway so all good. Should have an update this weekend! Will post here when I do.

Cool stuff. Looking forward to Odin getting this :)