Re: Yet another 2 patches for music streams

[Erik Sandberg]

On 5/22/06, Han-Wen Nienhuys <address@hidden> wrote:

> Also, in general,  I think it should be possible to split patches up
> further, eg. the tremolo stuff seems independent of the
> partcombine/lyriccombine stuff.

A problem is my limited internet access. I can cvs diff very seldom; I
often work several days on lily between the diffs. This makes it
difficult/troublesome to keep patches atomic.

Currently I keep 3 copies of the lily tree, so I can maintain 2
independent diff branches + one unmodified version; this already leads
to problems (the tremolo stuff was first supposed to go into branch 2,
but was accidentally added to branch 1 instead).

> >+         dir_ = RIGHT;
> >+       }
> >+      if (dir == STOP)
> >+       {

They don't represent the same thing. RIGHT means that the right
beam-count is modified, STOP means that an event stops a spanner.

>  >
> > +      SCM tremolo_symbol = ly_symbol2scm ("TremoloSpanEvent");
> >+      SCM start_event_scm = scm_call_2 (ly_lily_module_constant 
> ("make-span-event"), tremolo_symbol, scm_from_int (START));
> >+      unsmob_music (start_event_scm)->set_spot (*origin);
> >+      SCM stop_event_scm = scm_call_2 (ly_lily_module_constant 
> ("make-span-event"), tremolo_symbol, scm_from_int (STOP));
> >+
> >+      Music *start_event = unsmob_music (start_event_scm);
> >+      Music *stop_event = unsmob_music (stop_event_scm);
>
> I think you're leaking memory here, some unprotects are necessary.

The Music objects are created by SCM code, I thought stuff returned
from Scheme was unprotected in general?

>  >+      SCM start_chord = scm_call_1 (ly_lily_module_constant
> ("chordify-event"), start_event_scm);
>  >+      SCM stop_chord = scm_call_1 (ly_lily_module_constant
>  >("chordify-event"), stop_event_scm);
>  >+
>  >+      child_list_ = scm_list_3 (start_chord, body->self_scm (),
>  >stop_chord);+
>
> I think it should be possible to do without the chord_event, and simply
> insert the events themselves. Possibly you will need to make an
> Event_iterator, which is similar to Event_chord_iterator.

ok, I'll look into that.

> >+      child_list_ = scm_list_3 (start_chord, body->self_scm (), stop_chord);+ 
>    }
> >+
> >+  Sequential_iterator::construct_children ();
>
> >+void
> >+Chord_tremolo_iterator::derived_mark () const
> > {
> >-  return child_iter_->try_music (m);
> >+  scm_gc_mark (child_list_);
>
> It's better style to construct the child_list in get_music_list, and not
> store it all, as it's already stored in the base class in the cursor_
> variable.  Then you also don't need a derived_mark() member.
>
> I notice that you've done this with the other iterators too. Can you
> change this, also in time-scaled-music-iterator and percent-repeat-iterator?

ok

> > +          // Wait for a Create_context event, to catch implicitly created 
> voices before it's too late.
> > +          t->events_below ()->add_listener (GET_LISTENER (check_new_context), 
> ly_symbol2scm ("CreateContext"));
> > +          listening_ = true;
> > +        }
>
> why does this happen in find_voice() ?
>
> Isn't it more natural to set the listener in construct_children?

good idea (which btw seems to make a dirty workaround clean).

> > +      // UGH. Only swallow the output property event in the context
> > +      // it was intended for. This is inelegant but not inefficient.
> > +      if (context ()->context_name_symbol () == m->get_property 
> ("context-type"))
> > +        {
> > +          props_.push_back (m);
> > +          return true;
> > +        }
>
> this doesn't work with \alias. Can you use context::is_alias() ?

ok

> > +                               get_outlet ()->context_name_symbol ());
>
> You're modifying the input; that's not allowed.
>
> Can you write a convert-ly rule to change
>
>    \context Foo \applyOutput XXX
>
> ->
>
>    \applyOutput #'Foo XXXX
>
> and change the \applyOutput to set context-type.

ok

> > +/*
> > +* Processes a moment in an iterator, and returns whether any new music was 
> reported.
> > +*/
> > +bool
> > +Part_combine_iterator::try_process (Music_iterator *i, Moment m)
> > +{
> > +  Dispatcher *disp = i->get_outlet ()->event_source ();
> > +
> > +  disp->add_listener (GET_LISTENER (set_busy), ly_symbol2scm ("MusicEvent"));
> > +  busy_ = false;
> > +
> > +  i->process (m);
> > +
> > +  disp->remove_listener (GET_LISTENER (set_busy), ly_symbol2scm 
> ("MusicEvent"));
> > +  return busy_;
> > +}
>
> I don't understand this. Why are you adding and removing listeners all
> the time? Why don't you signal the state of the Part_combine_iterator to
> set_busy through another boolean member?  ie.

Good idea, thanks (I just tried to emulate the previous behaviour as
closely as possible)

> I don't understand. What is this for?

I don't remember right now (I added those lines last summer). It might
be related to problems with grace notes, or somethign like that.
Should I remove hte lines if I can't find a more clear justification?

> > -     (tremolo-type ,integer? "")
> > +     (tremolo-type ,integer? "speed of tremolo, e.g. 16 for c4:16")
>
> hmm. That sucks, we should store the negative log (-4 for 16) rather
> than 16.

why negative? the parameter is used to control the number of tremolo beams.

> > +#define LOWLEVEL_MAKE_SYNTAX(proc, args)     \
> > +  scm_apply_0 (proc, args)
> > +/* Syntactic Sugar. */
> >  #define MAKE_SYNTAX(name, location, ...)     \
> > -  scm_apply_0 (ly_lily_module_constant (name), scm_list_n (make_input 
> (location), __VA_ARGS__, SCM_UNDEFINED));
> > +  LOWLEVEL_MAKE_SYNTAX (ly_lily_module_constant (name), scm_list_n 
> (make_input (location), __VA_ARGS__, SCM_UNDEFINED));
> >
>
> I don't see the point of LOWLEVEL_MAKE_SYNTAX

It was used for music functions first, then I found a different way to
implement music functions. The macro can be used to use arbitrary
function objects iso. symbols to represent functions.

> > -     | ONCE music_property_def {
> > -             Music *m = unsmob_music ($2);
> > -             SCM e = m->get_property ("element");
> > -                unsmob_music (e)->set_property ("once", SCM_BOOL_T);
> > -             $$ = $2;
>
> I'm not sure if I argee with this. We're getting a warning, but to me
> it's a real syntax error. I'm worried that people might expect \once to
> work in places where it doesn't.

I think the proper way is to make it a music macro; is it OK to keep
it a music function until music macros exist? (also, the music
function code attempts to signal errors when \once is used in the
wrong place, so from a user's point of view, little changes)

> >  (define-ly-syntax-loc (repeat type num body alts)
> >    (make-repeat type num body alts))
> > +
> > +(define-ly-syntax-loc (context-specification type id mus ops create-new?)
>
> I believe that according to Scheme coding standards, the ? is only used
> on procedures, so this should just be create-new.

ok. I recall seeing a ? in an existing non-procedure symbol in lily;
I'll remove it the next time I see it.

Erik