Incorrect parsing for non-power-of-two notes

[jcjohnson]

Thanks for the great package!

I noticed that when parsing charts with 12ths or 24ths, the underlying Fraction representation for Beats are very strange.

Here's a minimal reproducing example where we parse an SM file with a single measure full of 12th notes and print the numerator and denominator of all Note objects:

https://gist.github.com/jcjohnson/603d9b5ab50016429f762cf9a5db2af7

When run, it gives the output:

(0, 1)
(6004799503160661, 18014398509481984)
(6004799503160661, 9007199254740992)
(1, 1)
(6004799503160661, 4503599627370496)
(7505999378950827, 4503599627370496)
(2, 1)
(5254199565265579, 2251799813685248)
(6004799503160661, 2251799813685248)
(3, 1)
(7505999378950827, 2251799813685248)
(8256599316845909, 2251799813685248)

The problem is here: https://github.com/garcia/simfile/blob/master/simfile/notes/__init__.py#L169

You are passing a float to the Fraction constructor rather than a pair of integers. This will behave fine when the fraction is a power of two (since these can be represented exactly as floating-point values) but will give strange behavior when the fraction is not a power of two.

The fix is simple; you just need to use the Fraction constructor that accepts a pair of integers for the numerator and denominator. When run with --apply-patch=1, my test script above monkey-patches NoteData.__iter__ to apply this fix; this then gives a much more normal result when parsing the chart with 12th notes:

(0, 1)
(1, 3)
(2, 3)
(1, 1)
(4, 3)
(5, 3)
(2, 1)
(7, 3)
(8, 3)
(3, 1)
(10, 3)
(11, 3)

[garcia]

thanks a bunch for the detailed issue & patch! I just published a fix for this under 2.0.0-beta.3.

[jcjohnson]

Awesome, thanks for the quick fix!