Re: [GSoC] Implement cross-voice dynamic spanners (issue 304160043 by address@hidden)

[dak]

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-alignment-autobreak.ly
File input/regression/dynamics-alignment-autobreak.ly (right):

https://codereview.appspot.com/304160043/diff/20001/input/regression/dynamics-alignment-autobreak.ly#newcode19
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-engraver.cc
File lily/dynamic-align-engraver.cc (right):

https://codereview.appspot.com/304160043/diff/20001/lily/dynamic-align-engraver.cc#newcode124
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?

https://codereview.appspot.com/304160043/