[GSoC] Implement cross-voice dynamic spanners

[GSoC] Implement cross-voice dynamic spanners

Update (8/22): multiple/cross-voice spanner management has been moved into the Spanner_engraver class, and is presently working. The code/commits can also be found on my GitHub fork https://github.com/starrynte/lilypond/compare/master...gsoc-2016-spanners

Commits:
Set "spanner-share-context" event property with \=.
  (spanners-init.ly, define-music-properties.scm)

Create multiple engraver instances to handle multiple spanners.
  (*.hh, grob.cc, spanner-engraver.cc, spanner.cc, define-context-properties.scm, define-grob-properties.scm)

Customize unterminated spanner warnings for each grob.
  (spanner.cc, translator-group.cc, define-grob-properties.scm)

Implement cross-voice spanners in dynamic engravers.
  (dynamic-align-engraver.cc, dynamic-engraver.cc)

Add regression tests.


Examples:
<< { c\=Score.hello\< d e f } \\ { e f g\=Score.hello\! a } >>
\new Staff { << { c d e\=Staff.hello\< f } >> << { g\> f\!\=Staff.hello\! e d } >> }
\new Staff { c d e\=1\< f\=2\< g f\=1\! e\=2\! d }

[Nathan Chou, 2016-07-23 02:31:56]

Made some changes to Spanner_engraver, and added reg tests

[dak, 2016-07-25 10:01:52]

I've started going through with the code review here but at some point decided
that I was missing the elephant in the room.  So don't really bother with the
code-related commment.

The main problem I see with the current code is that it is ad-hoc and basically
unmaintainable.  Spanner_engraver is a class that really serves no clear purpose
apart from carrying a few helper functions.  The collection of helper functions
does not create a coherent design.  All the actual management of multiple
spanners still happens in the spanner classes themselves.

One indicator of a lack of coherence is that you did not touch Slur_engraver
(which has a working multiple-spanner implementation of its own, without the
multiple-context part).  Having a working generic multiple-spanner
implementation should have _simplified_ the code in Slur_engraver by moving the
stuff dealing with multiple events out.

So the question is how would we _want_ multiple-spanner management to be
reflected in the code?  It's complex and mostly orthogonal to the actual work
(except when multiple spanners or likely rather their grobs have to notice each
other in order to avoid collisions), so the answer is "as little as possible". 
The ideal implementation would be to change ASSIGN_EVENT_ONCE to
ASSIGN_EVENT_SPANNER_ID and not change anything else.  However, the amount of
trickery that would need to get done then would be considerable.

However, the principle underlying this somewhat unrealistic example is still
valid: the bulk of the code should only cater for a single spanner.  If we can
arrive there, we will also be in a good position to let users create Scheme
engravers that can deal with multiple spanners without having to delve into
multiple-spanner management themselves.

So let's get back to a good API here: the ASSIGN_EVENT_ONCE proposal would be
too tricky to pull off, but your idea of using a Spanner_engraver base class has
merit.  What should this Spanner_engraver do?  Well, if the instance is supposed
to deal only with a subset of the arriving events, it is not going to be a true
Engraver but merely something behaving like one.  It will have a pointer to the
actual Engraver class (assuming that it needs it) and implement most of the
function calls from an Engraver class by reflecting them to the actual engraver.

The add_acknowledger/listener functions will probably get a companion
add_multi_acknowledger/listener that will only admit events/grobs matching the
respective spanner id.  That's the main filtering that appears to be necessary. 
So basically Spanner_engraver would entertain additional acknowledger/listener
arrays routed through a hash table routed via spanner_id.

So that's the basic hand-wavy sketch of how to abstract the multiple-spanner
management into a class of its own _without_ significantly complicating the
spanner classes themselves.

Basically the key metric would be that lily/slur-engraver.cc should become
simpler to work on and with than it is now by having the multiple-spanner
management funneled off into a separate class and retaining only management for
a single spanner.

If the additional code/classes does not manage to _simplify_ reading
lily/slur-engraver.cc once it makes it in there, it is going to make writing
maintainable multi-spanner code harder, not easier.

If that abstraction is successful, adding the multiple-context management on top
of the multiple-spanner-id management should require almost no additional code
in slur-engraver.cc itself rather than in the spanner management class.

So the question is how much of a chance we have to get where reusing the same
multiple-id/multiple-context code for both dynamic spanners and for
Slur_engraver (in a way where the ad-hoc multi-spanner management of
Slur_engraver is mostly removed from the Slur_engraver class itself) becomes
feasible.

That's basically my take on this, focused more on the matter of how LilyPond can
better be extended and maintained in future rather than on whether the proposed
code will work for the intended purpose correctly right now.  I trust that you
tested it to do that, but for it to be of best use in future, its operation
should be better separated from the code it affects.

https://codereview.appspot.com/304160043/diff/20001/input/regression/dynamics...
File input/regression/dynamics-alignment-autobreak.ly (right):

https://codereview.appspot.com/304160043/diff/20001/input/regression/dynamics...
input/regression/dynamics-alignment-autobreak.ly:19: <<
This code is not really seminal to the topic of this regtest.  You should rather
create one or more separate regtests for the new functionality and have a
matching description.

The same for the other regtests.

https://codereview.appspot.com/304160043/diff/20001/lily/dynamic-align-engrav...
File lily/dynamic-align-engraver.cc (right):

https://codereview.appspot.com/304160043/diff/20001/lily/dynamic-align-engrav...
lily/dynamic-align-engraver.cc:124: started_.insert (started_.begin (), info);
Inserting at the beginning of the vector is an O(n) operation, leading to an
O(n^2) overall behavior.  Wouldn't it make more sense to push into a separate
structure here like it was originally done?  What is the problem you are trying
to solve by using only one structure?

[Nathan Chou, 2016-07-25 10:47:33]

Thank you very much for taking the time to review this!

On 2016/07/25 10:01:52, dak wrote:
> The main problem I see with the current code is that it is ad-hoc and
basically
> unmaintainable.  Spanner_engraver is a class that really serves no clear
purpose
> apart from carrying a few helper functions.  The collection of helper
functions
> does not create a coherent design.  All the actual management of multiple
> spanners still happens in the spanner classes themselves.
...
> So the question is how would we _want_ multiple-spanner management to be
> reflected in the code?  It's complex and mostly orthogonal to the actual work
> (except when multiple spanners or likely rather their grobs have to notice
each
> other in order to avoid collisions), so the answer is "as little as possible".

> The ideal implementation would be to change ASSIGN_EVENT_ONCE to
> ASSIGN_EVENT_SPANNER_ID and not change anything else.  However, the amount of
> trickery that would need to get done then would be considerable.
> 
> However, the principle underlying this somewhat unrealistic example is still
> valid: the bulk of the code should only cater for a single spanner.  If we can
> arrive there, we will also be in a good position to let users create Scheme
> engravers that can deal with multiple spanners without having to delve into
> multiple-spanner management themselves.

I agree. Handling multiple spanners in Spanner_engraver seems like a good idea;
I'll try to revise this patch to do so (with the help of your suggestions).

>
https://codereview.appspot.com/304160043/diff/20001/input/regression/dynamics...
> input/regression/dynamics-alignment-autobreak.ly:19: <<
> This code is not really seminal to the topic of this regtest.  You should
rather
> create one or more separate regtests for the new functionality and have a
> matching description.
> 
> The same for the other regtests.

Will do.

[Nathan Chou, 2016-08-13 10:13:50]

Hi,

I am almost done moving multiple/cross-voice spanner code out of specific
engravers into the Spanner_engraver class. I just want to make sure this
approach is on the right track before finishing up and submitting the patch:

New instances of the engraver are created to handle
spanners/events/announcements with particular id's. The instance initially
created with the context will maintain a map/list of the additional child
instances that are created.

Instead of the listen_ or acknowledge_ functions in the engraver, a special
function is called in Spanner_engraver when listening or acknowledging.
This function then calls the listen_ or acknowledge_ function:
* On just the engraver instance corresponding to the spanner-id, if this
was indicated when the listener/acknowledger was added
* On all engraver instances otherwise

This does seem to allow each instance (and the engraver code) to only have
to deal with one spanner. Please let me know of any improvements or
concerns, thanks!

Nathan

[dak, 2016-08-13 15:05:26]

Nathan Chou <starrynte@gmail.com> writes:

> Hi,
>
> I am almost done moving multiple/cross-voice spanner code out of specific
> engravers into the Spanner_engraver class. I just want to make sure this
> approach is on the right track before finishing up and submitting the patch:
>
> New instances of the engraver are created to handle
> spanners/events/announcements with particular id's. The instance initially
> created with the context will maintain a map/list of the additional child
> instances that are created.

The "maintain a map/list of the additional child instances" bit is of
course the most relevant.  Each specific engraver source file should
have to bother as little as possible, optimally making the
"spanner-id-ization" a fairly trivial and mechanical replacement.

> Instead of the listen_ or acknowledge_ functions in the engraver, a
> special function is called in Spanner_engraver when listening or
> acknowledging.  This function then calls the listen_ or acknowledge_
> function: * On just the engraver instance corresponding to the
> spanner-id, if this was indicated when the listener/acknowledger was
> added * On all engraver instances otherwise
>
> This does seem to allow each instance (and the engraver code) to only
> have to deal with one spanner. Please let me know of any improvements
> or concerns, thanks!

The general description seems fine to me.  The interesting thing will be
to see how you factored the stuff.

-- 
David Kastrup

[Nathan Chou, 2016-08-20 11:51:59]

Preliminary refactor to use multiple engraver instances to handle multiple
spanners

[Nathan Chou, 2016-08-20 12:12:47]

Hello,

Although I haven't thoroughly checked this patch, and I'll likely have more
questions and changes to make, I thought I'd upload this for some initial
general comments since the GSoC deadline is approaching.

My previously-described approach was to have one Spanner_engraver instance
receive all the events/acknowledgments and then forward them to other instances
(of the same engraver). However, I decided to try a bit different approach: all
Spanner_engraver instances receive all events/acknowledgments, and (for the
specified listeners/acknowledgers) individually filter out all but the ones with
matching spanner-id/spanner-share-context. I'm not sure which way is better, so
the final approach could be subject to change.

I changed spanner-id and spanner-share-context to be properties of
grob-interface so Spanner_engraver can filter acknowledgments based on those
properties of the acknowledged grob. I believe for Dynamic_align_engraver, one
could lookup those properties on the event_cause () of the Grob_info, but I'm
not sure if this is true for acknowledgments in general. Can acknowledgments to
engraver instances be filtered by the spanner-id and spanner-share-context
properties of the acknowledgments' event_cause ()?

Thanks again for all the help!

Nathan

[Nathan Chou, 2016-08-22 09:49:50]

Fix issue with unterminated warnings; add reg tests.

[Nathan Chou, 2016-08-23 09:04:18]

Also, I noticed that when creating cross-staff dynamics I get the error

programming error: My pure_y_common is a VerticalAlignment, which might contain
several staves.

although the output still seemed to be correctly produced for the few cases I
tried. Is this something I need to worry about?

Nathan

[Nathan Chou, 2016-08-25 09:15:23]

Fixed (though not sure if with best approach) issue with filtered acknowledgers.
May try coding alternative approach to see how it compares.

[dak, 2016-09-09 05:20:06]

I'm currently really bad at focusing and still trying to wrap my head around
this properly.  Cannot decide yet whether this would warrant engraver-group
support to pull off better, and if so, whether this can be done well afterwards
or should really come first.  Give me another countdown period, please.

[Nathan Chou, 2016-09-09 05:46:20]

Thanks for looking at this, David! Let me know if there's any portions I could
explain more clearly.

Nathan

[dak, 2016-10-21 13:22:53]

Stuff is not getting better by myself procrastinating any longer, so I'm putting
out the current review, lacklustre as it may seem given the time I let pass on
it.

https://codereview.appspot.com/304160043/diff/80001/lily/dynamic-align-engrav...
File lily/dynamic-align-engraver.cc (right):

https://codereview.appspot.com/304160043/diff/80001/lily/dynamic-align-engrav...
lily/dynamic-align-engraver.cc:57: Grob *script_;
Ok, I'm somewhat confused here.  The code already was dealing with multiple
spanners before?

While you are perfectly keeping the quota of comments intact, the previous state
was programmed in a "standard" manner, and the previous state of the comments
was a hurdle to readers that does not really warrant continuing in the same
style: basically it is an artifact of LilyPond's two-person past and the
confidence of young programmers to live as long as the code lasts.

Now you are reorganizing things in a tentatively new manner, but you don't
document what you are doing.  That's one of the reasons for myself
procrastinating on the review: I expect to be able to understand complex stuff
but when it's no fun doing so, I am running away before getting anywhere.

Which is the wrong reaction.  Other people have their eyes glaze over and pass
on, but as long as they did not promise a review...

Sorry for that.  At any rate, you need to state here what is organised by what,
and sketch how Dynamic_engraver (which already started out organizing multiple
engravers and thus is _not_ behaving like slurs and other multi-instance
engravers) fits into Spanner_engraver with its particular needs.

https://codereview.appspot.com/304160043/diff/80001/lily/dynamic-align-engrav...
lily/dynamic-align-engraver.cc:99: if (current_dynamic_spanner_ == dynamic)
What's the relation between current_spanner_, current_dynamic_spanner_, and
dynamic ?  That's not clear in comparison to the original code.

https://codereview.appspot.com/304160043/diff/80001/lily/dynamic-engraver.cc
File lily/dynamic-engraver.cc (right):

https://codereview.appspot.com/304160043/diff/80001/lily/dynamic-engraver.cc#...
lily/dynamic-engraver.cc:148: current_span_event_->get_property
("spanner-share-context"),
Wouldn't make_multi_spanner be able to consult spanner-share-context and
spanner-id on its own?

https://codereview.appspot.com/304160043/diff/80001/lily/include/engraver-gro...
File lily/include/engraver-group.hh (right):

https://codereview.appspot.com/304160043/diff/80001/lily/include/engraver-gro...
lily/include/engraver-group.hh:56: friend class Spanner_engraver;
What is the friendship needed for in particular?  When it's not too much effort,
it is nice to get along without friends, but if one does lean on them, it makes
sense keeping book on what you need them for.

This is not life advice.

https://codereview.appspot.com/304160043/diff/80001/lily/include/spanner-engr...
File lily/include/spanner-engraver.hh (right):

https://codereview.appspot.com/304160043/diff/80001/lily/include/spanner-engr...
lily/include/spanner-engraver.hh:15: // Context property spannerEngravers is an
alist:
Ok, here is the principal problem with this module: you are microdocumenting it.
 There is no description how to use it, no description how it fits into things,
no description of what it does (only details of how it does it, but not forming
a coherent picture), no description of its expectations and what it can and
cannot be used for.

Yes, this is a bit par for the course for much of LilyPond's code base.  But
it's not right.  For some interfacing module that attempts to do a slightly
better job at sketching how things fit into place, see lily/include/smobs.hh . 
Still documented comparatively briefly, but the microdocumentation then has an
overall _context_ sketched out and you can check how the code falls into place
regarding its purpose and design.

This is really missing here and it makes it hard to review because it is all a
heap of code with local descriptions and no picture of what it does and what it
connects with in order to abstract what task given which parameters and boundary
conditions.

https://codereview.appspot.com/304160043/diff/80001/lily/include/spanner-engr...
lily/include/spanner-engraver.hh:16: // ( #(engraver-class-symbol share-context
spanner-id) . engraver-list )
That sounds like something better maintained in internal variables of
Spanner_engraver rather than a context property?

https://codereview.appspot.com/304160043/diff/80001/lily/include/spanner-engr...
lily/include/spanner-engraver.hh:102: template <class T, void (T::*callback)
(Grob_info)>
I'm not fond of large, comparatively generic template functions, and there are a
lot of them.  Would it make sense to abstract some of this into non-templated
functions?

https://codereview.appspot.com/304160043/diff/80001/lily/include/spanner-engr...
lily/include/spanner-engraver.hh:111: if (is_manager_)
Ok, this is_manager_ concept seems like a bad fit with regard to the overall
data structures.  It means that every engraver instance here has all the
functions and data structures needed to manage multiple engravers.

What are alternative means of doing this?

The simplest would likely be to have the multiple spanner class be templated on
the class it is controlling.  It would then create actual engravers as needed. 
To get equivalent behavior, it might seem that one engraver instance should be
created right away.  However, since the multiple-spanner-ready engravers need to
be prepared to be created later than that anyway, I think I'd just create them
as needed even for the default one.

The advantage would be that the hierarchy would be much cleaner: currently I
feel that there is code distributed up and down, making it harder to track
responsibilities and consequently also for figuring out how to work this into
slurs and others.

Now the question is how such a rearrangement would best be organized: we don't
want the code fall dead, but the time allotment for the project also is
basically over.  See a perspective here time and motivation getting this
simplified?

https://codereview.appspot.com/304160043/diff/80001/lily/include/spanner-engr...
lily/include/spanner-engraver.hh:121: if (ly_is_equal (id, filter_id_) &&
ly_is_equal (share->self_scm (), filter_share_->self_scm ()))
Yes, this is ugly: basically having the managing class duplicated everywhere
receiving everything and sorting it out.  It would be much nicer to have a
single instance receiving the events and distributing them.

[a-kobel_a-kobel.de, 2016-10-21 13:33:35]

David,

while I did not read the entire mail, I just happened to stumble across 
this comment - made my day!

On 2016-10-21 15:22, dak@gnu.org wrote:
> lily/include/engraver-group.hh:56: friend class Spanner_engraver;
>
> What is the friendship needed for in particular?  When it's not too much
> effort, it is nice to get along without friends, but if one does lean on
> them, it makes sense keeping book on what you need them for.
>
> This is not life advice.

[dak, 2016-10-21 14:38:06]

https://codereview.appspot.com/304160043/diff/80001/lily/include/spanner-engr...
File lily/include/spanner-engraver.hh (right):

https://codereview.appspot.com/304160043/diff/80001/lily/include/spanner-engr...
lily/include/spanner-engraver.hh:111: if (is_manager_)
On 2016/10/21 13:22:53, dak wrote:
> Ok, this is_manager_ concept seems like a bad fit with regard to the overall
> data structures.  It means that every engraver instance here has all the
> functions and data structures needed to manage multiple engravers.
> 
> What are alternative means of doing this?
> 
> The simplest would likely be to have the multiple spanner class be templated
on
> the class it is controlling.  It would then create actual engravers as needed.

> To get equivalent behavior, it might seem that one engraver instance should be
> created right away.  However, since the multiple-spanner-ready engravers need
to
> be prepared to be created later than that anyway, I think I'd just create them
> as needed even for the default one.
> 
> The advantage would be that the hierarchy would be much cleaner: currently I
> feel that there is code distributed up and down, making it harder to track
> responsibilities and consequently also for figuring out how to work this into
> slurs and others.
> 
> Now the question is how such a rearrangement would best be organized: we don't
> want the code fall dead, but the time allotment for the project also is
> basically over.  See a perspective here time and motivation getting this
> simplified?

Sorry that's a bit confused as Spanner_engraver is already templated on its
controlled class.  The point is that the controlled class is derived from
Spanner_engraver<Class> which has the advantage that this can provide the same
machinery as Engraver but do it differently.  But it has the disadvantage that
the actually running "master" Engraver is running multiple instances.  So maybe
one needs to shoehorn the one-spanner engraver in question between a single
master Engraver instance (so necessarily being a separate type from the
individual engravers not sharing inheritance) receiving and redistributing the
events and have some interface class in closer relation to the individual
engravers (either being derived from the individual engraver class and
exercising its interfaces while using virtual functions to take over the
underlying Engraver functions, or by being its base class instead of Engraver
and reimplementing the base functionality).

[Nathan Chou, 2016-10-24 10:50:36]

Thanks for the review, David!

I haven't read everything in detail yet but I think I got the gist of your
suggestions. I do now see how having all the events go to a Spanner_engraver
instance, then distributed to the appropriate one-spanner engraver would be more
organized.

> Now the question is how such a rearrangement would best be organized: we don't
> want the code fall dead, but the time allotment for the project also is
> basically over.  See a perspective here time and motivation getting this
> simplified?

I am still willing to work on this, although school has started again, so any
progress would probably be made slowly.

Nathan

[Nathan Chou, 2016-10-24 10:50:38]

Thanks for the review, David!

I haven't read everything in detail yet but I think I got the gist of your
suggestions. I do now see how having all the events go to a Spanner_engraver
instance, then distributed to the appropriate one-spanner engraver would be more
organized.

> Now the question is how such a rearrangement would best be organized: we don't
> want the code fall dead, but the time allotment for the project also is
> basically over.  See a perspective here time and motivation getting this
> simplified?

I am still willing to work on this, although school has started again, so any
progress would probably be made slowly.

Nathan