search

surge-synthesizer / tuning-library

Micro-tuning format parsing and frequency finding as a header-only C+ library
https://surge-synth-team.org/tuning-library/
MIT License
83 stars 15 forks source link

error parsing `kbm` file with `size: 0` #42

Closed modosc closed 3 years ago

modosc commented 3 years ago

scala ships with some example .kbm files that throw Tunings::TuningError: Incomplete KBM file. Unable to get to keys section of file. when parsed. seems like the issue is the zero-size of the mapping?

! 128.kbm
! Key-for-key mapping with Middle C on standard frequency.
! Size:
0
! First MIDI note number to retune:
0
! Last MIDI note number to retune:
127
! Middle note where the first entry of the mapping is mapped to:
0
! Reference note for which frequency is given:
60
! Frequency to tune the above note to (floating point e.g. 440.0):
261.6256
! Scale degree to consider as formal octave:
127
! piano.kbm
! Key-for-key mapping for 88-key piano with A4=440
! Size:
0
! First MIDI note number to retune:
21
! Last MIDI note number to retune:
108
! Middle note where the first entry of the mapping is mapped to:
21
! Reference note for which frequency is given:
69
! Frequency to tune the above note to (floating point e.g. 440.0):
440.0
! Scale degree to consider as formal octave:
88
baconpaul commented 3 years ago

We test size 0 maps https://github.com/surge-synthesizer/tuning-library/blob/main/tests/data/empty-note61.kbm so I wonder what’s different with these. Will look. Thanks!

baconpaul commented 3 years ago

Non zero scale degree seems like one difference

baconpaul commented 3 years ago

Yeah I think those aren’t valid KBM but the error message is wrong. It should say an octave linear map doesn’t have a scale length. http://www.huygens-fokker.org/scala/help.htm#mappings

baconpaul commented 3 years ago

Anyway just guesses. I’ll look soon

baconpaul commented 3 years ago

Huh so OK both of these files parse file for me when I copy this text into a kbm I wonder if they have a line ending that is confusing the parser. The error message would occur if it can't fully read that last line. Can you take exactly the files and .zip them and apply them here?

If you look at the branch 'kbm-parse-42in thebaconpaul` fork of this repo you can see your two files attached and parsed in the tests with no error. So definitely need the original files. Thanks

modosc commented 3 years ago

kbm.zip

i got these files from the scala distribution:

http://www.huygens-fokker.org/scala/downloads.html#Mac

specifically i copied them from /Applications/Scala.app/Contents/Resources/scala-22-mac-osx

it also occurred to me they might be templates and not intended as actual kbm files? i've never used these before so i may be missing something.

baconpaul commented 3 years ago

Hmm. I have no problem parsing either of those files with readKbmFile either with my type in version or the direct copied version. No errors at all.

So how does your program which is parsing them work is my next question I guess?

modosc commented 3 years ago

i'm not doing anything crazy (at least i don't think so?):

  //  Locations::tunings() is a juce::File
  auto file = Locations::tunings()
                  .getChildFile(tuningFile)
                  .getFullPathName()
                  .toStdString();
  scale = Tunings::readSCLFile(file);

   //  Locations::keyboardMappings() is a juce::File
  file = Locations::keyboardMappings()
                  .getChildFile(keyboardMappingFile)
                  .getFullPathName()
                  .toStdString();
  keyboardMapping = Tunings::readKBMFile(file);

  tuning = Tunings::Tuning(scale, keyboardMapping);

i also verified that the files installed from my app's bundle are the same as the files from the scala installation:

$ md5sum /Applications/Scala.app/Contents/Resources/scala-22-mac-osx/128.kbm
9dcdd409cd507044e71a06ea213d35cf  /Applications/Scala.app/Contents/Resources/scala-22-mac-osx/128.kbm
$ md5sum 128.kbm
9dcdd409cd507044e71a06ea213d35cf  128.kbm

is there any other info from my end that would be useful?

baconpaul commented 3 years ago

Huh crazy. That's exactly what I'm doing also. I just pushed an upgrade to the stuff around the lib (including your file as a test case which passes) and adding a CMakeLists.txt which will make life easier

Along the way I added a small utility in the commands/ directory called 'parsecheck'. You can build it with tuning-lib at head with

cmake -Bbuild
cmake --build build --target parsecheck

then do parsecheck /path/to/file1.scl /path/to/file2.kbm /path/to/file3.scl

and it will parse each. Does this work for you with these files?

baconpaul commented 3 years ago 
paul:~/dev/music/tuning-library$ bl/parsecheck /Applications/Scala.app/Contents/Resources/scala-22-mac-osx/*kbm
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/10.kbm PASSED
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/11.kbm PASSED
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/12-19.kbm PASSED
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/12-31.kbm PASSED
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/128.kbm PASSED
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/6.kbm PASSED
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/61.kbm PASSED
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/7.kbm PASSED
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/8.kbm PASSED
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/a440.kbm PASSED
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/axis49.kbm PASSED
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/black.kbm PASSED
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/blackjack.kbm PASSED
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/bp.kbm PASSED
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/breed.kbm PASSED
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/clinear.kbm PASSED
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/dlinear.kbm PASSED
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/erl22.kbm PASSED
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/erl22d.kbm PASSED
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/example.kbm PASSED
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/lowengard.kbm PASSED
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/map_1.kbm PASSED
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/map_2.kbm PASSED
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/map_3.kbm PASSED
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/map_4.kbm PASSED
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/map_5.kbm PASSED
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/miller24.kbm PASSED
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/miller31.kbm PASSED
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/millerlemba24.kbm PASSED
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/miracle24.kbm PASSED
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/mr88cet.kbm FAILED : Invalid line 12. line='57  ! A'. Bad character is '!/33'
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/piano.kbm PASSED
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/reverse_d_12.kbm FAILED : Invalid line 16. line='-12'. Bad character is '-/45'
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/reverse_d_et.kbm FAILED : Invalid line 16. line='-1'. Bad character is '-/45'
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/seth10.kbm PASSED
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/two17.kbm PASSED
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/two19.kbm PASSED
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/two20.kbm PASSED
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/two22.kbm PASSED
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/two22b.kbm PASSED
/Applications/Scala.app/Contents/Resources/scala-22-mac-osx/white.kbm PASSED
baconpaul commented 3 years ago

is your mac set to us english? I wonder if I have a locale bug (even though I test those)

baconpaul commented 3 years ago

(I'll add those inline comment ones to my test suite soon enough and fix it)

modosc commented 3 years ago

ok, i built parsecheck and got the same results as above.

so next guess - is there something wrong with how i'm building tuning-library into my app that could be causing this (c++ version / compiler version / etc)?

baconpaul commented 3 years ago

Fascinating

Right so build tuning library and do the tests fail? If so yeah what c compiler and os are you using?

This case could arise if your stdlib doesn’t deal with endlin / eof properly. Then the last line would never trigger the state transition.

I’m sure we can find it and I appreciate your efforts to help isolate why you see this and I don’t.

baconpaul commented 3 years ago

(To run the tests just use the cmake target run-all-tests)

modosc commented 3 years ago

ok, figured it out and it's all my fault.

you'll get the error i got if you do Tunings::readKBMFile("")

i think you can check the return value here to give a more specific error?

int c = sb->sbumpc();
if (c == -1) // failure here

anyway sorry for the noise, thanks!

baconpaul commented 3 years ago

Oh right we should give a better error if the file doesn’t open indeed! Lemme add that and add it to the tests. Good stuff.

baconpaul commented 3 years ago

So Tunings::readKBMFile( "" ) already reported a file not found, but Tunings::parseKBMData ("" ) gave the error message you saw. I have since improved the error message for both parseKBMData and parseSCLData in the incomplete file case to show how many lines were parsed and how far we got, as well as adding tests to make sure the error is generated and correct in the "" case at least. If you pull to head you should get a clearer result.

Thanks!