Modify fret calculation algorithm

Modify fret calculation algorithm
Zero fingering creates an open string fret.

Add defaultStrings property to define strings that will be used
 in calculating frets if no string number is specified in a chord.

Add regression tests for both of these cases.

[Carl, 2010-11-25 03:05:05]

Here's a patch to the fret calculator that answers the first two of Steve's
requests, I think.

http://article.gmane.org/gmane.comp.gnu.lilypond.general/60177

It turns a -0 fingering into an open string request, and it creates a context
property defaultStrings that will serve as a request for strings to be used if
the chord doesn't include specific string indications.

Steve and Patrick, you can only test this if you can build the code, because
this includes C++ changes.

Please review.

Thanks,

Carl

[steve.yegge, 2010-11-26 19:39:06]

Hi Carl,

I looked through the code; it looks great.  I built it and tested it fairly
thoroughly, and found no problems with it -- both features work as advertised.

I tried defaultStrings with single-string melodies, thirds, octaves, skip-string
chords and several other configurations.  I didn't find any bugs.  In one case I
did get a rather cryptic error message because I'd inadvertently specified the
strings in reverse order -- but some reason I've been unable to reproduce that
error message.

I also tried out the zero-fingering feature.  It works quite well in conjunction
with setting the minimumFret.  Again, I found no bugs, although my testing
shouldn't be considered exhaustive.

These features will do wonders for cutting down the verbosity in my music files.
 Thanks for implementing this!

[Carl, 2010-11-26 22:41:01]

On 2010/11/26 19:39:06, steve.yegge wrote:
> Hi Carl,
> 
> I looked through the code; it looks great.  I built it and tested it fairly
> thoroughly, and found no problems with it -- both features work as advertised.

Thanks for the testing!

> 
> I tried defaultStrings with single-string melodies, thirds, octaves,
skip-string
> chords and several other configurations.  I didn't find any bugs.  In one case
I
> did get a rather cryptic error message because I'd inadvertently specified the
> strings in reverse order -- but some reason I've been unable to reproduce that
> error message.

Yes, the assumption is that the string specification needs to be in the same
order as the note specification.  When the documentation is done, we'll need to
be sure that is mentioned.  If you specify the notes and the strings in reverse
order, it's likely to be impossible to find frets that are close enough together
to meet both notes.

> 
> I also tried out the zero-fingering feature.  It works quite well in
conjunction
> with setting the minimumFret.  Again, I found no bugs, although my testing
> shouldn't be considered exhaustive.

I think it should work, and we haven't found any bugs yet, so once we get a
style review we should be able to go ahead and push.  If bugs show up later,
we'll fix them.

> 
> These features will do wonders for cutting down the verbosity in my music
files.
>  Thanks for implementing this!

You're welcome.  Thanks to all who contributed ideas.

Now for the big request -- since I've made things easier for your music files,
would you be willing to write documentation for these two features?  It should
go in the Default Tablatures subsection of Notation Reference 2.4.1 Common
Notation for Fretted Strings.  The source resides in
Documentation/notation/fretted-strings.itely.

The policies for working on documentation are found in chapter 4 of the
Contributors' Guide.

Thanks,

Carl

[c_sorensen, 2010-11-26 22:46:43]

On 11/26/10 3:41 PM, "Carl.D.Sorensen@gmail.com" <Carl.D.Sorensen@gmail.com>
wrote:

> On 2010/11/26 19:39:06, steve.yegge wrote:
>>   Thanks for implementing this!
> 
> You're welcome.  Thanks to all who contributed ideas.
> 
> Now for the big request -- since I've made things easier for your music
> files, would you be willing to write documentation for these two
> features?  It should go in the Default Tablatures subsection of Notation
> Reference 2.4.1 Common Notation for Fretted Strings.  The source resides
> in Documentation/notation/fretted-strings.itely.
> 
> The policies for working on documentation are found in chapter 4 of the
> Contributors' Guide.

One last thing.  The \set defaultStrings command is a tweak, so it won't go
in the actual text of Documentation/notation/fretted-strings.itely.  It
needs to be a snippet with a texinfo description that will be stored in
Documentation/snippets/new.  The file will need to then be included as a
@lilypondfile{} reference in the selected snippets section of Default
tablatures.

Thanks,

Carl

[pls, 2010-11-27 00:25:52]

Am 26.11.2010 um 23:41 schrieb Carl.D.Sorensen@gmail.com:

> On 2010/11/26 19:39:06, steve.yegge wrote:
>> Hi Carl,
>
>> I looked through the code; it looks great.  I built it and tested it
> fairly
>> thoroughly, and found no problems with it -- both features work as
> advertised.
>
> Thanks for the testing!
>
>
>> I tried defaultStrings with single-string melodies, thirds, octaves,
> skip-string
>> chords and several other configurations.  I didn't find any bugs.  In
> one case I
>> did get a rather cryptic error message because I'd inadvertently
> specified the
>> strings in reverse order -- but some reason I've been unable to
> reproduce that
>> error message.
>
> Yes, the assumption is that the string specification needs to be in  
> the
> same order as the note specification.  When the documentation is done,
> we'll need to be sure that is mentioned.  If you specify the notes and
> the strings in reverse order, it's likely to be impossible to find  
> frets
> that are close enough together to meet both notes.
>
>
>> I also tried out the zero-fingering feature.  It works quite well in
> conjunction
>> with setting the minimumFret.  Again, I found no bugs, although my
> testing
>> shouldn't be considered exhaustive.
>
> I think it should work, and we haven't found any bugs yet, so once we
> get a style review we should be able to go ahead and push.  If bugs  
> show
> up later, we'll fix them.
>
>
>> These features will do wonders for cutting down the verbosity in my
> music files.
>>   Thanks for implementing this!
>
> You're welcome.  Thanks to all who contributed ideas.
>
> Now for the big request -- since I've made things easier for your  
> music
> files, would you be willing to write documentation for these two
> features?  It should go in the Default Tablatures subsection of  
> Notation
> Reference 2.4.1 Common Notation for Fretted Strings.  The source  
> resides
> in Documentation/notation/fretted-strings.itely.
I'll do it as soon as possible. But first of all I need to reinstall  
lilybuntu on my mac ox 10.4 and test your patch. At the moment I  
can't build LilyPond...

Thanks,

patrick
>
> The policies for working on documentation are found in chapter 4 of  
> the
> Contributors' Guide.
>
> Thanks,
>
> Carl
>
>
>
>
> http://codereview.appspot.com/3320041/

[c_sorensen, 2010-11-27 00:34:52]

On 11/26/10 5:25 PM, "Patrick Schmidt" <p.l.schmidt@gmx.de> wrote:
> Am 26.11.2010 um 23:41 schrieb Carl.D.Sorensen@gmail.com:
>> On 2010/11/26 19:39:06, steve.yegge wrote:
>>>   Thanks for implementing this!
>> 
>> You're welcome.  Thanks to all who contributed ideas.
>> 
>> Now for the big request -- since I've made things easier for your
>> music
>> files, would you be willing to write documentation for these two
>> features?  
> I'll do it as soon as possible. But first of all I need to reinstall
> lilybuntu on my mac ox 10.4 and test your patch. At the moment I
> can't build LilyPond...

Actually, I was inviting Steve to do it, since can already build it...

Thanks,

Carl

[Neil Puttock, 2010-11-27 00:39:21]

http://codereview.appspot.com/3320041/diff/4001/input/regression/tablature-de...
File input/regression/tablature-default-strings.ly (right):

http://codereview.appspot.com/3320041/diff/4001/input/regression/tablature-de...
input/regression/tablature-default-strings.ly:4: texidoc="
texidoc = "

http://codereview.appspot.com/3320041/diff/4001/input/regression/tablature-de...
input/regression/tablature-default-strings.ly:5: Context property defaultStrings
defines desired strings
@code{defaultStrings}

http://codereview.appspot.com/3320041/diff/4001/input/regression/tablature-ze...
File input/regression/tablature-zero-finger.ly (right):

http://codereview.appspot.com/3320041/diff/4001/input/regression/tablature-ze...
input/regression/tablature-zero-finger.ly:4: texidoc="
texidoc = "

http://codereview.appspot.com/3320041/diff/4001/input/regression/tablature-ze...
input/regression/tablature-zero-finger.ly:11: <d-0 a'-2 d-3 f-1>1
I can't see any difference in output with or without the zero.

An invalid zero fingering causes a crash:

\new TabVoice \relative c' {
  <f-0>
}

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

http://codereview.appspot.com/3320041/diff/4001/scm/translation-functions.scm...
scm/translation-functions.scm:292: (strings-used (if (and (zero?  (entry-count
defined-strings))
(zero? (

http://codereview.appspot.com/3320041/diff/4001/scm/translation-functions.scm...
scm/translation-functions.scm:361: (define (finger-number event)
unused

http://codereview.appspot.com/3320041/diff/4001/scm/translation-functions.scm...
scm/translation-functions.scm:413: (begin
redundant begin

http://codereview.appspot.com/3320041/diff/4001/scm/translation-functions.scm...
scm/translation-functions.scm:417: ))
move to previous line

[steve.yegge, 2010-11-27 02:28:37]

On Fri, Nov 26, 2010 at 2:41 PM, <Carl.D.Sorensen@gmail.com> wrote:

>
> You're welcome.  Thanks to all who contributed ideas.
>
> Now for the big request -- since I've made things easier for your music
> files, would you be willing to write documentation for these two
> features?  It should go in the Default Tablatures subsection of Notation
> Reference 2.4.1 Common Notation for Fretted Strings.  The source resides
> in Documentation/notation/fretted-strings.itely.
>
> The policies for working on documentation are found in chapter 4 of the
> Contributors' Guide.
>

Yes, it's the least I can do.  I need to fiddle with it to get a doc build
first, but I'll work on it right away.

-steve


>
> Thanks,
>
> Carl
>
>
>
>
>
> http://codereview.appspot.com/3320041/
>

[c_sorensen, 2010-11-27 04:10:47]

On 11/26/10 5:25 PM, "Patrick Schmidt" <p.l.schmidt@gmx.de> wrote:
> Am 26.11.2010 um 23:41 schrieb Carl.D.Sorensen@gmail.com:
> 
>> On 2010/11/26 19:39:06, steve.yegge wrote:
>>> I also tried out the zero-fingering feature.  It works quite well in
>> conjunction
>>> with setting the minimumFret.  Again, I found no bugs, although my
>> testing
>>> shouldn't be considered exhaustive.
>> 
>> I think it should work, and we haven't found any bugs yet, so once we
>> get a style review we should be able to go ahead and push.  If bugs
>> show
>> up later, we'll fix them.
>> 

Neil found a bug in the code.  What happens when someone requests an open
string that isn't present in the tuning?

I've got the code fixed to issue a warning.  After issuing the warning, we
have two choices:  

1 -- leave the improper pitch out of the tablature (which will leave a 0
fingering in the Staff and no corresponding pitch in the tablature)

2 -- calculate that pitch as part of the tablature, will will leave a 0
fingering in the Staff and a non-zero fret in the tablature.

Which would be preferable?

Thanks,

Carl

[Carl, 2010-11-27 04:17:55]

I've responded to all of Neil's comments.

Thanks for the review, Neil!

Carl

http://codereview.appspot.com/3320041/diff/4001/input/regression/tablature-de...
File input/regression/tablature-default-strings.ly (right):

http://codereview.appspot.com/3320041/diff/4001/input/regression/tablature-de...
input/regression/tablature-default-strings.ly:4: texidoc="
On 2010/11/27 00:39:21, Neil Puttock wrote:
> texidoc = "

Done.

http://codereview.appspot.com/3320041/diff/4001/input/regression/tablature-de...
input/regression/tablature-default-strings.ly:5: Context property defaultStrings
defines desired strings
On 2010/11/27 00:39:21, Neil Puttock wrote:
> @code{defaultStrings}

Done.

http://codereview.appspot.com/3320041/diff/4001/input/regression/tablature-ze...
File input/regression/tablature-zero-finger.ly (right):

http://codereview.appspot.com/3320041/diff/4001/input/regression/tablature-ze...
input/regression/tablature-zero-finger.ly:4: texidoc="
On 2010/11/27 00:39:21, Neil Puttock wrote:
> texidoc = "

Done.

http://codereview.appspot.com/3320041/diff/4001/input/regression/tablature-ze...
input/regression/tablature-zero-finger.ly:11: <d-0 a'-2 d-3 f-1>1
On 2010/11/27 00:39:21, Neil Puttock wrote:
> I can't see any difference in output with or without the zero.
>  
I've changed this to a chord that will put the D on the 5th string without the
zero, but on the 4th string with the zero.

> An invalid zero fingering causes a crash:
> 
> \new TabVoice \relative c' {
>   <f-0>

This has been fixed to issue a warning and fit the note to some other string. 
Another alternative is to issue a warning and leave the note out of the
tablature.

I've got a question out as to which behavior is preferable.

I've also added a regtest for this behavior.

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

http://codereview.appspot.com/3320041/diff/4001/scm/translation-functions.scm...
scm/translation-functions.scm:292: (strings-used (if (and (zero?  (entry-count
defined-strings))
On 2010/11/27 00:39:21, Neil Puttock wrote:
> (zero? (

Done.

[t.daniels_treda.co.uk, 2010-11-27 08:22:42]

Carl Sorensen wrote Saturday, November 27, 2010 4:10 AM

> Neil found a bug in the code.  What happens when 
> someone requests an open string that isn't present
> in the tuning?

> I've got the code fixed to issue a warning.  After
> issuing the warning, we have two choices:  
>
> 1 -- leave the improper pitch out of the tablature 
> (which will leave a 0 fingering in the Staff and no 
> corresponding pitch in the tablature)
>
> 2 -- calculate that pitch as part of the tablature, 
> will will leave a 0 fingering in the Staff and a 
> non-zero fret in the tablature.
>
> Which would be preferable?

Isn't the 0 fingering in the Staff equally incorrect?

As it is impossible to determine whether the pitch
or the fingering is wrong this should really be 
classed as a syntax error - the combination is invalid.

Trevor

[c_sorensen, 2010-11-27 13:36:59]

On 11/27/10 1:22 AM, "Trevor Daniels" <t.daniels@treda.co.uk> wrote:

> 
> 
> Carl Sorensen wrote Saturday, November 27, 2010 4:10 AM
> 
>> Neil found a bug in the code.  What happens when
>> someone requests an open string that isn't present
>> in the tuning?
> 
>> I've got the code fixed to issue a warning.  After
>> issuing the warning, we have two choices:
>> 
>> 1 -- leave the improper pitch out of the tablature
>> (which will leave a 0 fingering in the Staff and no
>> corresponding pitch in the tablature)
>> 
>> 2 -- calculate that pitch as part of the tablature,
>> will will leave a 0 fingering in the Staff and a
>> non-zero fret in the tablature.
>> 
>> Which would be preferable?
> 
> Isn't the 0 fingering in the Staff equally incorrect?

Yes, it is.  But it's only advisory.

> 
> As it is impossible to determine whether the pitch
> or the fingering is wrong this should really be
> classed as a syntax error - the combination is invalid.

No, it's not a syntax error, any more than

\time 4/4 c4 c c |

is a syntax error.

I'm fine to throw an error instead of a warning, so that the output will say
the file failed.  But it's a music error, not a syntax error.

Thanks,

Carl

[Neil Puttock, 2010-12-08 00:03:25]

Carl, there are two code nitpicks I mentioned before which are still present:

`finger-number' is defined but not used in translation-functions.scm.

`pad-list' has a redundant `begin'.

Cheers,
Neil

[steve.yegge, 2010-12-08 01:01:48]

Incidentally I'm still working on documenting this feature.
I've had to reinstall virtually every piece of software on my machine,
but I'm converging on a working LilyBuntu build on Snow Leopard.

-steve

On Tue, Dec 7, 2010 at 4:03 PM, <n.puttock@gmail.com> wrote:

> Carl, there are two code nitpicks I mentioned before which are still
> present:
>
> `finger-number' is defined but not used in translation-functions.scm.
>
> `pad-list' has a redundant `begin'.
>
> Cheers,
> Neil
>
> http://codereview.appspot.com/3320041/
>

[c_sorensen, 2010-12-09 01:59:52]

Thanks, Neil.  They're gone now.

Carl



On 12/7/10 5:03 PM, "n.puttock@gmail.com" <n.puttock@gmail.com> wrote:

> Carl, there are two code nitpicks I mentioned before which are still
> present:
> 
> `finger-number' is defined but not used in translation-functions.scm.
> 
> `pad-list' has a redundant `begin'.
> 
> Cheers,
> Neil
> 
> http://codereview.appspot.com/3320041/