Code review - Issue 304160043: [GSoC] Implement cross-voice dynamic spannershttps://codereview.appspot.com/2016-10-24T10:50:38+00:00rietveld
Message from unknown
2016-07-20T00:56:40+00:00Nathan Chouurn:md5:6344244aae506da4a1f597668427d24f
Message from unknown
2016-07-23T02:31:52+00:00Nathan Chouurn:md5:b649d5103432bf614349239e06d5efa4
Message from starrynte@gmail.com
2016-07-23T02:31:56+00:00Nathan Chouurn:md5:733bbfbc361e064c4a4515071e2bf8bf
Made some changes to Spanner_engraver, and added reg tests
Message from dak@gnu.org
2016-07-25T10:01:52+00:00dakurn:md5:3522470d5867990fe54519aef93e91e0
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?
Message from starrynte@gmail.com
2016-07-25T10:47:33+00:00Nathan Chouurn:md5:aef29aeb16dff3f93fb1987da21d7b7a
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-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.
Will do.
Message from starrynte@gmail.com
2016-08-13T10:13:50+00:00Nathan Chouurn:md5:3c5b43a533a65c557f516213d29a242d
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
Message from dak@gnu.org
2016-08-13T15:05:26+00:00dakurn:md5:7c6562d170d50fbd86411009ac3058bb
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
Message from unknown
2016-08-20T11:51:54+00:00Nathan Chouurn:md5:d6ab09348f189634396c2cf115d7ff0a
Message from starrynte@gmail.com
2016-08-20T11:51:59+00:00Nathan Chouurn:md5:1ab3e4e9acae4c24ab861ab0876459a2
Preliminary refactor to use multiple engraver instances to handle multiple spanners
Message from starrynte@gmail.com
2016-08-20T12:12:47+00:00Nathan Chouurn:md5:c9294df80eee374d69314b141f025ada
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
Message from unknown
2016-08-22T09:49:48+00:00Nathan Chouurn:md5:afc8895506ff4d15bd6084907f131789
Message from starrynte@gmail.com
2016-08-22T09:49:50+00:00Nathan Chouurn:md5:636f869c1a66db8a88fbed34c44de678
Fix issue with unterminated warnings; add reg tests.
Message from starrynte@gmail.com
2016-08-23T09:04:18+00:00Nathan Chouurn:md5:081c4f599ffe166de61e18f5e4b639cd
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
Message from unknown
2016-08-25T09:15:18+00:00Nathan Chouurn:md5:2bfae81eff374f1c8de00e4ca5f5bd41
Message from starrynte@gmail.com
2016-08-25T09:15:23+00:00Nathan Chouurn:md5:ddf2f3f6f38744c74b7731cc611c9fe4
Fixed (though not sure if with best approach) issue with filtered acknowledgers. May try coding alternative approach to see how it compares.
Message from dak@gnu.org
2016-09-09T05:20:06+00:00dakurn:md5:244612c7de9fb98036891b23fe075779
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.
Message from starrynte@gmail.com
2016-09-09T05:46:20+00:00Nathan Chouurn:md5:b0fcda4428c27a55d705b4e1f204405a
Thanks for looking at this, David! Let me know if there's any portions I could explain more clearly.
Nathan
Message from dak@gnu.org
2016-10-21T13:22:53+00:00dakurn:md5:b125a64cff8bb75e0002deb893fe8cf4
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-engraver.cc
File lily/dynamic-align-engraver.cc (right):
https://codereview.appspot.com/304160043/diff/80001/lily/dynamic-align-engraver.cc#newcode57
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-engraver.cc#newcode99
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#newcode148
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-group.hh
File lily/include/engraver-group.hh (right):
https://codereview.appspot.com/304160043/diff/80001/lily/include/engraver-group.hh#newcode56
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-engraver.hh
File lily/include/spanner-engraver.hh (right):
https://codereview.appspot.com/304160043/diff/80001/lily/include/spanner-engraver.hh#newcode15
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-engraver.hh#newcode16
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-engraver.hh#newcode102
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-engraver.hh#newcode111
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-engraver.hh#newcode121
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.
Message from a-kobel@a-kobel.de
2016-10-21T13:33:35+00:00a-kobel_a-kobel.deurn:md5:fbc43ec075f71dda321fd9bb8d62faaf
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.
Message from dak@gnu.org
2016-10-21T14:38:06+00:00dakurn:md5:16ec5850fe30ddc1d200b13e996afb2a
https://codereview.appspot.com/304160043/diff/80001/lily/include/spanner-engraver.hh
File lily/include/spanner-engraver.hh (right):
https://codereview.appspot.com/304160043/diff/80001/lily/include/spanner-engraver.hh#newcode111
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).
Message from starrynte@gmail.com
2016-10-24T10:50:36+00:00Nathan Chouurn:md5:8411577c196158a9b8e5d502c784b9e1
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
Message from starrynte@gmail.com
2016-10-24T10:50:38+00:00Nathan Chouurn:md5:01eb0c6514eaae8c3bac134fe11c6635
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