ozankasikci / rust-music-theory

A music theory guide written in Rust.
MIT License
626 stars 28 forks source link

Make chord representation consistent #2

Open majg0 opened 4 years ago

majg0 commented 4 years ago

This is tricky! :P I really appreciate your hard work on this crate!

In https://github.com/ozankasikci/rust-music-theory/blob/86591a15c119c41a681d1cb1150f1ccc64ae4343/src/chord/chord.rs#L92, the default chord is erroneously missing its intervals.

The easiest way to fix this is to use ::new and call it a day, but imo that keeps the design open for a similar problem happening elsewhere.

Another viable approach: What if we make Chord a trait instead, and make several concrete representations for it? For example a vector of intervals could be considered a chord, albeit without a root pitch class. Chord quality and whether it's a triad/seventh /something exotic could be inferred from the intervals.

We could still construct chords from quality, string, etc but my point is we should ideally only represent it with one unique and flexible data point, because right now it's possible for the Chord struct to contain invalid data, where the fields mismatch each other and go out of sync.

If we consider performance, I'd personally vouch for being able to compute each thing from each other thing, e.g. string representation from intervals, quality from string, etc.

What do you think?

ozankasikci commented 4 years ago

@martingronlund thank you.

I agree that a chord should never have an empty list of intervals. However i'm not sure how would making the Chord a trait prevent that? The implementation of a Chord trait could still consist an empty vector of intervals, unless it's explicitly checked.

What we can and will do instead is, returning Result<Self> from Chord::new. This is the idiomatic way to handle invalid data in Rust.

Does that make sense to you?

majg0 commented 4 years ago

Not really. The point I was trying to make is that there is redundant data, which means that the struct can become invalid at any time if we're unlucky or not careful enough. I guess you want to cache some fields for performance, but if those go unaccessed it might just be a hog of unnecessary resources. Imo better to compute on demand and be in control of caching.

ozankasikci commented 4 years ago

Hi Martin, can you please explain what you mean with some concrete code examples? because i'm not sure if i'm following what you mean here.

majg0 commented 4 years ago

I'll try :)

Let's say we want to represent a C major chord.

My point is there are many ways to fully represent just "C major" - that very string being yet another example of how to fully encode all information we need (assuming western music with equal temperament etc etc, which is fair).

The main point I want to make, and the core concept I think is important here, is that we should avoid mixing several representations of the same conceptual data in the core library structs. E.g. we should avoid storing all of { root note (C) + intervals (M3,P5) + notes (C,E,G) } in a chord struct, because it contains redundant data. There are several reasons for me to think its inadvisable to mix representations;

My main proposal is simple: All redundant data could be computed on demand.

This would be reducing construction overhead and memory consumption, making state more easily understandable and easy to reason about, while giving API users more control.

Now... In my previous comments I couldn't settle on how to best represent a chord fundamentally and I thought that, in some sense, the string "C major" is just as good as {C + major quality}, so I proposed using a trait to gather all those representations of a chord into one. However, I think I was wrong: "C major" is a worse representation than {C + quality}.

I'd like to propose

  1. struct Chord { root: Note, quality: u16 }
  2. Move the old fields which I removed, into functions. Users can cache on demand instead.

Then using the sixteen bits as flags for whether the interval of the corresponding distance is "on". E.g. in C major, there is a M3 and P5, which are 4 and 7 semitones away from C respectively, which translates to e.g. 1 << 3 == 8 and 1 << 6 == 64 so 8 + 64 == 72. Alternatively, not assuming the root is always present like I just did, reserving bit 1 for whether it is: 1 + 1 << 4 + 1 << 7 == 145. I think this second approach is nicer because of additional flexibility together with that the numbers for bitshifts line up with the semitone count.

Phew. 👍 haha

ozankasikci commented 4 years ago

Thanks for elaborating, Martin! :)

Now i know your reasoning and what you meant. This sounds like a more flexible way of representing chords. But of course would require some work, and design decisions. Also would dramatically change the API that is exposed.

I'm not against the idea of having this dynamic way of representing a chord. Might implement this sometime in future. Atm i don't have the time for it and the road map is already defined a long time ago.

I appreciate your time and effort for proposing this!

donRumata03 commented 2 years ago

This idea looks pretty much the same to the core concept of normalized data bases: if there's a redundancy in you data representation, it will finally go out of synchronization.