music-notation-swift / music-notation-core

Music Notation Library in Swift (Deprecated. See `music-notation` instead)
MIT License
115 stars 16 forks source link

CompoundTuplet incorrect initialization #134

Closed massimobio closed 5 years ago

massimobio commented 6 years ago
let triplet = try? Tuplet(3, .eighth, notes: [eighthNote, eighthNote, eighthNote])
let compoundTuplet = try? Tuplet(5, .eighth, notes: [eighthNote, eighthNote, triplet!, eighthNote])
print(compoundTuplet!)

prints this: 6[1/8a1, 1/8a1, 3[1/8a1, 1/8a1, 1/8a1], 1/8a1] Which is incorrect.

There is a related test related to this in TupletTests.testInitSuccessForComplexCompound()

sprzenus commented 5 years ago

@massimobio What should be the result of this initializer in this case then?

drumnkyle commented 5 years ago

If I’m looking at it right, I believe it is just that the print out is wrong. I think the initialization seems correct but the print out should probably be a 5 instead of a 6. Because it is a tuplet of 5

sprzenus commented 5 years ago

If print is wrong, flatIndexes has wrong value.

drumnkyle commented 5 years ago

Hmm. I’d be surprised if that’s the case. I thought the tests should cover this and I don’t think the tests for this functionality are broken. Can you verify that this is still happening or do you need me to try it out?

sprzenus commented 5 years ago

I don't see any tests on noteCount or flatIndexes in TupletTests on master branch. Are you sure it is tested? If no, maybe it would be good to test it.

The function mentioned here is added to the travis branch. I would prefer adding the missing tests to another branch.

drumnkyle commented 5 years ago

Hmm. Yeah another PR would be fine. Let me try to look at it and see what’s going on first. I’ll comment back.

sprzenus commented 5 years ago

I'm not sure if it isn't just an understanding problem. You create tuplet with a duration of 5 eighths which is ok for 3 eighths and the triplet. And you get 6 notes. It seems to be correct

drumnkyle commented 5 years ago

Thanks @sprzenus. You're right. This is a misunderstanding. We are printing the note count. It would probably be nice to print the ratio, but we can make a separate issue to do that later if we want. I will close this issue then.