Implements rhythmic-music-iterator

Implements rhythmic-music-iterator

[dak, 2012-01-18 13:38:38]

http://codereview.appspot.com/5554048/diff/2001/lily/rhythmic-music-iterator.cc
File lily/rhythmic-music-iterator.cc (right):

http://codereview.appspot.com/5554048/diff/2001/lily/rhythmic-music-iterator....
lily/rhythmic-music-iterator.cc:41: report_event (get_music ());
I suppose that one has to clear "articulations" here (after fetching them), or
they will get treated by both report_event as well as the rhythmic iterator.

Since it would be awkward to actually mess with the Music event, one will likely
not be able to call report_event, but instead call to_event, and then take the
articulations from there.  As a result, one would not be calling report_event in
the loop either.  And one would likely have to descend_to_context (and other
report_event stuff) manually.

[MikeSol, 2012-01-18 13:56:25]

http://codereview.appspot.com/5554048/diff/2001/lily/rhythmic-music-iterator.cc
File lily/rhythmic-music-iterator.cc (right):

http://codereview.appspot.com/5554048/diff/2001/lily/rhythmic-music-iterator....
lily/rhythmic-music-iterator.cc:41: report_event (get_music ());
On 2012/01/18 13:38:39, dak wrote:
> I suppose that one has to clear "articulations" here (after fetching them),
or
> they will get treated by both report_event as well as the rhythmic iterator.

Good call.  A new patch set is up that fixes this.  It adds some extra verbage,
but it makes sure that:
a) Notes are reported without their articulations; and
b) Articulations are reported after notes.

This should prevent the new-fingering-engraver from seeing the articulations. 
Furthermore, given the way that the music stream currently looks, articulations
are placed after notes in an event chord.  In theory this shouldn't matter, but
in case it does, this puts the loop after the first report_event.

[dak, 2012-01-18 14:27:53]

http://codereview.appspot.com/5554048/diff/5001/lily/rhythmic-music-iterator.cc
File lily/rhythmic-music-iterator.cc (right):

http://codereview.appspot.com/5554048/diff/5001/lily/rhythmic-music-iterator....
lily/rhythmic-music-iterator.cc:42: get_music ()->set_property ("articulations",
SCM_EOL);
I am not actually comfortable with clearing articulations in the original music
event rather than its eventized copy.

Are iterators allowed to change their input?

[MikeSol, 2012-01-18 14:36:57]

On 2012/01/18 14:27:53, dak wrote:
>
http://codereview.appspot.com/5554048/diff/5001/lily/rhythmic-music-iterator.cc
> File lily/rhythmic-music-iterator.cc (right):
> 
>
http://codereview.appspot.com/5554048/diff/5001/lily/rhythmic-music-iterator....
> lily/rhythmic-music-iterator.cc:42: get_music ()->set_property
("articulations",
> SCM_EOL);
> I am not actually comfortable with clearing articulations in the original
music
> event rather than its eventized copy.
> 
> Are iterators allowed to change their input?

I'm not sure if they're allowed by design, but I know they're allowed to
proliferate input that wasn't there before.

One solution is to just set a property in the event "ignore-articulations". 
That way, when the new-fingering-engraver gets the event, it can check if this
is set before it touches articulations.

[dak, 2012-01-18 14:53:28]

On 2012/01/18 14:36:57, MikeSol wrote:
> On 2012/01/18 14:27:53, dak wrote:
> > Are iterators allowed to change their input?
> 
> I'm not sure if they're allowed by design, but I know they're allowed to
> proliferate input that wasn't there before.

I don't know what you mean by "proliferate input" as this verb is intransitive. 
If you mean generating new input: sure.  I am worried by it changing the input. 
If it gets iterated in a different context again, information will be gone.

> One solution is to just set a property in the event "ignore-articulations".

Where would be the point?  If I plan to touch up the stream event before
broadcasting it, I can't use the everything-included report_event anyway.  If I
plan to touch up the music event in a manner equivalent to deleting the
articulations, I can just delete the articulations instead.  In either case, the
original music event would behave differently after having once visited the
iterator.

So I guess I'll take your patch and just do a bit of copying of what
report_event does.  It is pretty much equivalent to my messing around with an
additional optional boolean argument to report_event, but less hidden.

Perhaps it would be cleanest to make report_event a virtual function?  Then
rhythmic_iterator could just override it.  Better yet, EventChord could just
override it, and we would not need the rhythmic iterator at all.

[dak, 2012-01-18 15:23:35]

On 2012/01/18 14:53:28, dak wrote:
> Then
> rhythmic_iterator could just override it.  Better yet, EventChord could just
> override it, and we would not need the rhythmic iterator at all.

Well, it is not really "better yet" since the current case is the simple and
straightforward one, so it should be the music iterator default.  So I guess
I'll see whether I can fudge this into the Rhythmic iterator after all.

[mike_apollinemike.com, 2012-01-18 15:28:42]

On Jan 18, 2012, at 3:53 PM, dak@gnu.org wrote:

> Where would be the point?  If I plan to touch up the stream event before
> broadcasting it, I can't use the everything-included report_event
> anyway.  If I plan to touch up the music event in a manner equivalent to
> deleting the articulations, I can just delete the articulations instead.
> In either case, the original music event would behave differently after
> having once visited the iterator.
> 

I don't think there's a risk that one engraver in a context will get a different
version of an event than another engraver in a context.  This would require the
NoteEvent to be reported more than once.  This is why I don't think setting
articulations to SCM_EOL will matter.

Have you had the chance to apply my patch set along with your work to see if it
works?

Cheers,
MS

[dak, 2012-01-18 15:55:35]

On 2012/01/18 15:28:42, mike_apollinemike.com wrote:

> I don't think there's a risk that one engraver in a context will get a
different
> version of an event than another engraver in a context.  This would require
the
> NoteEvent to be reported more than once.

It would merely require using the same music expression more than once without
copying it.  Music expressions are frontend material.  I am not really
comfortable messing with them.  Now it is true that if you put stuff in a music
variable and access this using \var, you get a copy.  And music functions in
general are not expected to worry about stomping over their arguments
destructively because of that.

But I still have a hard time believing that every Music variable is guaranteed
to never have anybody ever look at it again once it has been seen by an iterator
once.

[dak, 2012-01-18 16:11:46]

On 2012/01/18 15:55:35, dak wrote:
> On 2012/01/18 15:28:42, http://mike_apollinemike.com wrote:
> 
> > I don't think there's a risk that one engraver in a context will get a
> different
> > version of an event than another engraver in a context.  This would require
> the
> > NoteEvent to be reported more than once.
> 
> It would merely require using the same music expression more than once without
> copying it.  Music expressions are frontend material.  I am not really
> comfortable messing with them.  Now it is true that if you put stuff in a
music
> variable and access this using \var, you get a copy.  And music functions in
> general are not expected to worry about stomping over their arguments
> destructively because of that.
> 
> But I still have a hard time believing that every Music variable is guaranteed
> to never have anybody ever look at it again once it has been seen by an
iterator
> once.

No need to mess with virtual functions actually since this is all inside of the
rhythmic iterator (one could override the report function non-virtually, but
there is little point since it does almost nothing).  The following puts
together all of the collected stuff.

void
Rhythmic_music_iterator::process (Moment m)
{
  if (last_processed_mom_ < Moment (0))
    {

      descend_to_bottom_context ();

      Stream_event *ev = get_music ()->to_event ();
      SCM arts = ev->get_property ("articulations");
      Context *c = get_outlet ();
      
      if (scm_is_pair (arts))
        {
          ev->set_property ("articulations", SCM_EOL);
          c->event_source ()->broadcast (ev);
          for (; scm_is_pair (arts); arts = scm_cdr (arts))
            c->event_source ()->broadcast (unsmob_stream_event (scm_car
(arts)));
        }
      else
        c->event_source ()->broadcast (ev);

      ev->unprotect ();
    }
  Simple_music_iterator::process (m);
}

[MikeSol, 2012-01-18 16:14:44]

The tuplet-iterator.cc has a process method that uses similar logic as that
above: it does parts of what the report_music function does, because as you
correctly state, the function does very little.

LGTM - if you can, please adopt this patch so that you can coordinate getting it
and others connected to it into the source.