saebekassebil / teoria

Javascript taught Music Theory
http://saebekassebil.github.io/teoria
MIT License
1.32k stars 114 forks source link

Evaluating scale degree of enharmonics acquired from TeoriaNote.enharmonics() can fail #57

Closed benmurrell closed 10 years ago

benmurrell commented 10 years ago

Here is a snippet that causes the error

function test() {
    var aNote = teoria.note( 'db3' );
    var aScale = teoria.note( 'f' ).scale( 'major' );

    var enharmonics = aNote.enharmonics();
    for( var i = 0; i < enharmonics.length; i++ ) {
        var note = enharmonics[i];
        if( note.scaleDegree( aScale ) > 0 ) {
            return note;
        }
    }   
}

TeoriaInterval.quality() calls TeoriaInterval.qualityValue(), which is returning 5, which is out of bounds within the kAlterations sub-array that is being accessed. This may be related to https://github.com/saebekassebil/teoria/issues/56 as it looks like the qualityValue function is dependent on the range that TeoriaInterval.coord lies within.

benmurrell commented 10 years ago

I found a workaround that normalizes the coord. It is similar to the comment I left on the other issue I created.

function add(note, interval) {
    var ret = [note[0] + interval[0], note[1] + interval[1]];

    while( ret[1] <= -9 ) {
      ret[1] += 12;
      ret[0] -= 7;
    }

    while( ret[1] >= 16 ) {
      ret[1] -= 12;
      ret[0] -= 7;
    }

    return ret;
  }

I am hesitant to create any commits or submit any pull requests because it's a hack - I understand why what I've done fixes the immediate issue, but I don't know what the other effects might be in the library.

benmurrell commented 10 years ago

I ended up patching sub to do the same normalization also.

saebekassebil commented 10 years ago

This is indeed a bug in the scaleDegree method - but it isn't related to #56, but rather how it's been implemented. I'll take a look at it as soon as possible, thank you for noticing Ben!

saebekassebil commented 10 years ago

Fixed by 517442302d110c6ec2aa231fae96b287e15817f1 and e56694e60062dad062d073d15e110e6d3e01ab58 - Let me know if it works you, and thanks for reporting!

saebekassebil commented 10 years ago

Let me just add, that it had to do with some weird lookups I'm doing to convert a compound interval into a simple. I'll soon look into simplifying some of that code.

benmurrell commented 10 years ago

I pulled in your most recent changes and it works great for me. Thanks!