Fix determine-frets so that it preserves note order

Fix determine-frets so that it preserves note order

[Carl, 2011-05-09 14:17:16]

In response to Mike's request, I've fixed the problem in determine-frets that
sometimes changed the order of the notes in the chord.

This keeps the glissando between the same notes in the TabStaff as in the Staff.

I've made enough changes that I'd like to get a review of the changes.

Thanks,

Carl

[Neil Puttock, 2011-05-09 18:34:44]

Hi Carl,

LGTM, apart from some indentation issues.

One question for you though: do you think the refactoring is an improvement on
the current code?  You could for example achieve the same result with the
following:

    (sort string-fret-fingering-tuples
	  (lambda (a b) (> (car a) (car b)))))
    ;; end of determine-frets-and-strings

Cheers,
Neil

[c_sorensen, 2011-05-09 18:45:06]

On 5/9/11 12:34 PM, "n.puttock@gmail.com" <n.puttock@gmail.com> wrote:

> Hi Carl,
> 
> LGTM, apart from some indentation issues.
> 
> One question for you though: do you think the refactoring is an
> improvement on the current code?  You could for example achieve the same
> result with the following:
> 
>      (sort string-fret-fingering-tuples
>           (lambda (a b) (> (car a) (car b)))))
>      ;; end of determine-frets-and-strings
> 

There's no guarantee that the notes will be in decreasing string number
order.

That's the general case, but it's not required.

In absolute mode I can write a C major chord as

<c' e' g'>

or

<g' e' c'>

and as far as I know, we have no requirement that we write chords with the
lowest note first.

Originally, (before the harmonic became part of the tab note head) we could
be order-agnostic, because each head had its own information necessary to
place the glyph.  But now we aren't order agnostic because of glissandos on
each note, and because the harmonic needs to be associated with the proper
note.

Additionally, there are instruments (at least one, the ukulele) where
strings do not increase in pitch, so pitch order (as chords are generally
entered in relative mode) doesn't necessarily match string order (which is
what the sort call you proposed creates).

For these reasons, I think something like what I've done is better than just
the sort code you proposed.

But I'm open to having the discussion.  Thanks for the feedback!

Carl



> Cheers,
> Neil
> 
> 
> 
> http://codereview.appspot.com/4518045/

[Neil Puttock, 2011-05-09 18:53:21]

On 9 May 2011 19:44, Carl Sorensen <c_sorensen@byu.edu> wrote:

> There's no guarantee that the notes will be in decreasing string number
> order.
>
> That's the general case, but it's not required.
>
> In absolute mode I can write a C major chord as
>
> <c' e' g'>
>
> or
>
> <g' e' c'>
>
> and as far as I know, we have no requirement that we write chords with the
> lowest note first.

Ah, good point.  I hadn't thought of that.  :)

In that case, it definitely LGTM.

Cheers,
Neil

[Neil Puttock, 2011-05-09 18:53:48]

http://codereview.appspot.com/4518045/diff/1/scm/translation-functions.scm
File scm/translation-functions.scm (right):

http://codereview.appspot.com/4518045/diff/1/scm/translation-functions.scm#ne...
scm/translation-functions.scm:280: (define free-strings (map 1+ (iota (length
tuning))))
could use srfi-1 iota here:

(iota (length tuning) 1)

[Keith, 2011-05-26 03:26:32]

http://codereview.appspot.com/4518045/diff/1/scm/translation-functions.scm
File scm/translation-functions.scm (right):

http://codereview.appspot.com/4518045/diff/1/scm/translation-functions.scm#ne...
scm/translation-functions.scm:351: (note-pitch note) string))
            (car pitch-entry) string))
`make check` was crashing on an unbound variable 'note'.  Given line 345 above
the fix was so obvious that I just pushed it.

[Carl, 2011-05-26 03:36:08]

On 2011/05/26 03:26:32, Keith wrote:

> `make check` was crashing on an unbound variable 'note'.  Given line 345 above
> the fix was so obvious that I just pushed it.

Thanks for the fix.  I had found that fix as well, and was getting ready to push
it.  But there's still another problem.  Right now 'ignore has the same effect
as 'recalculate, and it shouldn't. 

So this patch fixes compiling, but not the regtest.

Thanks,

Carl