Re: [frogs] chord-name-engraver plus capo

[Carl Sorensen <c_sorensen@xxxxxxx>]

Great job!  I've put some inline review comments below.


On 8/27/10 7:44 PM, "Wols Lists" <antlists@xxxxxxxxxxxxxxx> wrote:

>  Finally! Thanks Neil - it all worked!
> 
> So here it is, the new engraver. It could probably be tweaked to look a
> bit nicer, but it's there and it works. And I need a column markup
> function to finish it off :-) but here it is for your review.
> 
> The warning needs fixing - I haven't worked out why the compiler doesn't
> like "origin".
> 
> All the changes are guarded by an "if (capo) {}", so it shouldn't change
> *anything* unless the user has deliberately set capoFret in their .ly file.
> 
> The only other file modified is define-context-properties.scm, which
> suppresses warnings rather than is critical. I'm away for the weekend,
> so I'll put some effort into serious commenting later when I get the
> chance. If you can give me any clues where I've guessed wrong it would
> be appreciated. Enjoy :-)
> 
> -------------
> 
> /*
>   This file is part of LilyPond, the GNU music typesetter.
> 
>   Copyright (C) 1998--2010 Jan Nieuwenhuizen <janneke@xxxxxxx>
>   Copyright (C) 2010 Anthony Youngman <anthony@xxxxxxxxxxxxxxx>

Just add your name to the copyright name list, rather than adding a new
date.
> 
>   LilyPond is free software: you can redistribute it and/or modify
>   it under the terms of the GNU General Public License as published by
>   the Free Software Foundation, either version 3 of the License, or
>   (at your option) any later version.
> 
>   LilyPond is distributed in the hope that it will be useful,
>   but WITHOUT ANY WARRANTY; without even the implied warranty of
>   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>   GNU General Public License for more details.
> 
>   You should have received a copy of the GNU General Public License
>   along with LilyPond.  If not, see <http://www.gnu.org/licenses/>.
> */
> 
> #include <iostream>
> 
> #include "chord-name.hh"
> #include "context.hh"
> #include "dimensions.hh"
> #include "engraver.hh"        // required for all engravers ?
> #include "font-interface.hh"
> #include "international.hh"    // required to translate stuff eg error
> messages
> #include "item.hh"
> #include "output-def.hh"
> #include "pitch.hh"
> #include "protected-scm.hh"
> #include "stream-event.hh"
> #include "text-interface.hh"
> #include "warn.hh"
> 
> #include "translator.icc"    // required to define the engraver as a
> translator
> 
> // define the class here
> class Chord_name_engraver : public Engraver // declare the engraver as
> an engraver
> {
>   TRANSLATOR_DECLARATIONS (Chord_name_engraver);
> protected:
> 
>   // functions inherited from translator base class - declare and define
> as required
>   // virtual void initialize();        // set up for each grob as it is
> created
>   // void start_translation_timestep();    // this is called for all
> active grobs at the start of each timeslice ?
>   void process_music ();        // called after all events have been heard
>   // void process_acknowledged();    //    or grobs acknowledged
>   void stop_translation_timestep ();    // called for active grobs at
> the end of each timeslice
>   virtual void finalize ();        // tie up any loose ends when the
> grob finishes
> 
>   // other functions
>   virtual void derived_mark () const;
>   DECLARE_TRANSLATOR_LISTENER (note);    // tells the engraver to
> process notes ?
>   DECLARE_TRANSLATOR_LISTENER (rest);    // tells the engraver to
> process rests ?
> 
> private:    // member variable names end with an underscore
>   Item *chord_name_;
>   vector<Stream_event*> notes_;
> 
>   SCM last_chord_;
>   Stream_event *rest_event_;
> 
>   // ugh - surely there's a better way to do this ...
>   Pitch capo_transpose_[11];    // store the pitches to transpose down by
> };
> 
> Chord_name_engraver::Chord_name_engraver ()
> // As each context is created, the engravers are attached. This
> initialises the engraver.
> // these variables carry context forward from note to note.
> {
>   chord_name_ = 0;
>   last_chord_ = SCM_EOL;
>   rest_event_ = 0;
> 

Why not just define a C++ function

transpose_pitch (int semitones)

  return Pitch (0, - int (semitones / 2), - (semitones mod 2));


to handle the transposition?  Or perhaps even make this part of the
Pitch class -- i.e. add a transposed_semitones function to Pitch, so then
you could just call p->transposed_semitones (capo)?

>   // assume we're starting from C and transposing down // see above -
> this must be a crap way of doing it
>   capo_transpose_[0] = Pitch (0, -1, 0);    // B
>   capo_transpose_[1] = Pitch (0, -1, FLAT_ALTERATION);    // B flat
>   capo_transpose_[2] = Pitch (0, -2, 0);    // A
>   capo_transpose_[3] = Pitch (0, -2, FLAT_ALTERATION);    // A flat
>   capo_transpose_[4] = Pitch (0, -3, 0);    // G
>   capo_transpose_[5] = Pitch (0, -4, SHARP_ALTERATION);    // F sharp
>   capo_transpose_[6] = Pitch (0, -4, 0);    // F
>   capo_transpose_[7] = Pitch (0, -5, 0);    // E
>   capo_transpose_[8] = Pitch (0, -5, FLAT_ALTERATION);    // E flat
>   capo_transpose_[9] = Pitch (0, -6, 0);    // D
>   capo_transpose_[10] = Pitch (0, -7, SHARP_ALTERATION);    // C sharp
> }
> 
> // initialize()
> 
> // start_translation_timestep()
> 
> void
> Chord_name_engraver::process_music ()
> // This is called for each note or rest.
> // It is called as a result of being registered with
> IMPLEMENT_TRANSLATOR_LISTENER
> {
>   SCM markup;        // to store markup in
>   SCM bass = SCM_EOL;    // remember notes to create chord
>   SCM inversion = SCM_EOL;
>   SCM pitches = SCM_EOL;
> 
>   SCM capo_markup;    // and transposed notes for capo chord
>   SCM capo_bass = SCM_EOL;
>   SCM capo_inversion = SCM_EOL;
>   SCM capo_pitches = SCM_EOL;
> 
>   int capo = 0;        // declare capo as both flag and value
> 
>   if (rest_event_)
>     {    // process a rest

IMO, this comment is unnecessary.  rest_event_ is self explanatory

>       SCM no_chord_markup = get_property ("noChordSymbol");
>       if (!Text_interface::is_markup (no_chord_markup))
>         return;
>       markup = no_chord_markup;
>     }
>   else
>     {  // process a note event
>       if (!notes_.size ())
>         return;
> 
>       // get capo fret
>       // This is set by "\set ChordNames.capoFret = #3" in the lily
> source file
>       // declare properties in define-context-properties.scm

Similarly, these comments are unnecessary.  get_property ("capoFret") says
everything that the comments say.

>       SCM capofret = get_property ("capoFret");
Variable name should be capo_fret in C++

>       if (scm_is_number (capofret))
>       {
>     capo = scm_to_int (capofret);
>     if (capo < 0 || capo > 23)
>     {
> // !!! TODO !!! FIX !!!

ly:warning, IIUC.  And your message should mention that capo is set to 0
and continuing.

> //      origin ()->warning (_f ("invalid capo fret; must be 0 to 23,
> found: \"%i\"",
> //                  capo));
>       capo = 0;    // set to 0 to ignore
>     }
>     if (capo >= 12)
>       capo -= 12;    // if you can put the capo above 12th fret, we
> don't care about octaves
>       }
>     
>       Stream_event *inversion_event = 0;
>       for (vsize i = 0; i < notes_.size (); i++)
>       {    // step through each note in turn
>         Stream_event *n = notes_[i];
>         SCM p = n->get_property ("pitch");
>         if (!unsmob_pitch (p))
>           continue;
> 
>         if (n->get_property ("inversion") == SCM_BOOL_T)
>         {
>           inversion_event = n;
>           inversion = p;
The code below should be indented to match inversion
>       if (capo)
>       { // transpose the pitch // note that p here is not p above!

Why not 

Pitch *c_p = unsmob_pitch (p)

in order to avoid the double call to n->get_property and to avoid the
confusion?


>         Pitch *p = unsmob_pitch (n->get_property ("pitch"));    //
> convert the pitch to a C++ class (Pitch)
>         Pitch orig = p->transposed (capo_transpose_[capo-1]);    // Now
> transpose it
>         capo_inversion = orig.smobbed_copy ();            // and convert
> it back to a scheme object
>       }
>     }
>         else if (n->get_property ("bass") == SCM_BOOL_T)
>     {
>           bass = p;
>       if (capo) {

Since you're calling this transpose multiple times, it should probably
be a C++ function

  SCM capo_transpose (SCM scm_pitch, int capo_fret)
     Pitch *cxx_pitch = unsmob_pitch (scm_pitch);
     Pitch capo_pitch = cxx_pitch->transposed_semitones (capo_fret);
     return (capo_pitch.smobbed_copy ();

Then you can just say

    if (capo)
      capo_bass = capo_transpose (p, capo);

>         Pitch *p = unsmob_pitch (n->get_property ("pitch"));
>         Pitch orig = p->transposed (capo_transpose_[capo-1]);
>         capo_bass = orig.smobbed_copy ();
>       }
>     }
>         else
>     {
>           pitches = scm_cons (p, pitches);
>       if (capo) {
>         Pitch *p = unsmob_pitch (n->get_property ("pitch"));
>         Pitch orig = p->transposed (capo_transpose_[capo-1]);
>         capo_pitches = scm_cons ( orig.smobbed_copy (), capo_pitches);
>       }
>     }
>       }
> 
>       if (inversion_event)
>       {
>         SCM oct = inversion_event->get_property ("octavation");
>         if (scm_is_number (oct))
>         {
>           Pitch *p = unsmob_pitch (inversion_event->get_property ("pitch"));
>           int octavation = scm_to_int (oct);
>           Pitch orig = p->transposed (Pitch (-octavation, 0, 0));
> 
>           pitches = scm_cons (orig.smobbed_copy (), pitches);
>     
>       // if capo sort this out later !!!
>     
>         }
>         else
>           programming_error ("inversion does not have original pitch");
>       }
> 
>       pitches = scm_sort_list (pitches, Pitch::less_p_proc);
>       if (capo)
>     capo_pitches = scm_sort_list (capo_pitches, Pitch::less_p_proc);
>   
>       SCM name_proc = get_property ("chordNameFunction");
>       markup = scm_call_4 (name_proc, pitches, bass, inversion,
>           context ()->self_scm ());
>       if (capo)
>     capo_markup = scm_call_4 (name_proc, capo_pitches, capo_bass,
> capo_inversion,
>         context ()->self_scm ());
>     }
>   /*
>     Ugh. // Why's this ughly?
>   */
> 
>   // do we need any more capo stuff here beyond setting the chord name text?
> 
>   SCM chord_as_scm = scm_cons (pitches, scm_cons (bass, inversion));
> 
>   chord_name_ = make_item ("ChordName",
>       rest_event_ ? rest_event_->self_scm () : notes_[0]->self_scm ());
>   if (!capo)
>     chord_name_->set_property ("text", markup);
>   else
>   {
>     SCM capovertical = get_property ("capoVertical");
>     // how do I combine text and markup to get "markup (capo_markup)" !!!
>     SCM paren_proc = ly_lily_module_constant ("parenthesize-markup");
>     SCM line_proc = ly_lily_module_constant ("line-markup");
>     SCM hspace_proc = ly_lily_module_constant ("hspace-markup");
> 
>   SCM final_markup = scm_list_n (line_proc,
>                  scm_list_3 (markup,
>                          scm_list_2 (hspace_proc,
>                              scm_from_int (1)),
>                          scm_list_2 (paren_proc, capo_markup)),
>                  SCM_UNDEFINED);
> 
>     chord_name_->set_property ("text", final_markup);
>   }
> 
>   SCM chord_changes = get_property("chordChanges");
>   if (to_boolean (chord_changes) && scm_is_pair (last_chord_)
>       && ly_is_equal (chord_as_scm, last_chord_))
>     chord_name_->set_property ("begin-of-line-visible", SCM_BOOL_T);
> 
>   last_chord_ = chord_as_scm;
> }
> 
> // process_acknowledged()
> 
> void
> Chord_name_engraver::stop_translation_timestep ()
> {
>   chord_name_ = 0;
>   notes_.clear ();
>   rest_event_ = 0;
> }
> 
> void
> Chord_name_engraver::finalize ()
> {
> }
> 
> void
> Chord_name_engraver::derived_mark () const
> {
>   scm_gc_mark (last_chord_);
> }
> 
> 
> 
> IMPLEMENT_TRANSLATOR_LISTENER (Chord_name_engraver, note);
> void
> Chord_name_engraver::listen_note (Stream_event *ev)
> {
>   notes_.push_back (ev);
> }
> 
> IMPLEMENT_TRANSLATOR_LISTENER (Chord_name_engraver, rest);
> void
> Chord_name_engraver::listen_rest (Stream_event *ev)
> {
>   ASSIGN_EVENT_ONCE(rest_event_, ev);
> }
> 
> 
> /*
>   The READs description is not strictly accurate:
>   which properties are read depend on the chord naming function active.
> */
> ADD_TRANSLATOR (Chord_name_engraver,
>         /* doc */
>         "Catch note and rest events and generate the appropriate
> chordname.",
> 
>         /* create */
>         "ChordName ",
> 
>         /* read */    // list of properties it reads
>         "capoFret "    // added for capo
Don't need the comment on capoFret; its name says it all.

>         "capoVertical "
>         "chordChanges "
>         "chordNameExceptions "
>         "chordNameFunction "
>         "chordNoteNamer "
>         "chordRootNamer "
>         "chordNameExceptions "
>         "majorSevenSymbol "
>                 "noChordSymbol ",
> 
>         /* write */
>         ""
>         );
> 
> 
> ---
> ----
> Join the Frogs!
> 
Thanks,

Carl


---
----
Join the Frogs!