Following the few commits I did for #22, I noticed a few things related to the Set struct :
-
as IntervalSet is stored on an u32, in modes : the intervals which are popped right will come back from the 31th bit, which won't make those come back into the 0..12 semitones range (in the case of a scale defined on an octave), but there might be other uses where the octave limitation doesn't apply
also I think maybe it would make more sense if this modes function was part of the ScaleIntervals instead of the generic IntervalSet ?
(same rotate problem would occur on ScaleIntervals's u16 storage)
-
I was wondering why MidiSet hasn't been defined as a Set<MidiNote, u128>, which could avoid the low/high split, and maybe simplify the implementation for future things (like the awful totally unreadable split I did there!)
maybe it was to avoid u128 being not cffi compatible ? apparently it is since 1.78,
but since Dioxus is part of the crate, I not so sure about wasm ffi which is likely to get involved then...
-
closely related to the previous one, was also wondering why MidiSet doesn't have the Copy derive, since it's only 2 u64 ?
Would be ready to implement those changes myself if needed, just didn't want to change those with a PR coming out of nowhere without asking first !
thanks for this crate btw
I've tried a few other music theory crates before finding this one, but they didn't seem as much versatile
Following the few commits I did for #22, I noticed a few things related to the
Setstruct :as
IntervalSetis stored on anu32, inmodes: the intervals which are popped right will come back from the 31th bit, which won't make those come back into the 0..12 semitones range (in the case of a scale defined on an octave), but there might be other uses where the octave limitation doesn't applyalso I think maybe it would make more sense if this
modesfunction was part of theScaleIntervalsinstead of the genericIntervalSet?(same rotate problem would occur on
ScaleIntervals'su16storage)I was wondering why
MidiSethasn't been defined as aSet<MidiNote, u128>, which could avoid the low/high split, and maybe simplify the implementation for future things (like the awful totally unreadablesplitI did there!)maybe it was to avoid u128 being not cffi compatible ? apparently it is since 1.78,
but since Dioxus is part of the crate, I not so sure about wasm ffi which is likely to get involved then...
closely related to the previous one, was also wondering why
MidiSetdoesn't have the Copy derive, since it's only 2 u64 ?Would be ready to implement those changes myself if needed, just didn't want to change those with a PR coming out of nowhere without asking first !
thanks for this crate btw
I've tried a few other music theory crates before finding this one, but they didn't seem as much versatile