https://codereview.appspot.com/265410043/diff/60001/lily/part-combine-iterator.cc File lily/part-combine-iterator.cc (right):
https://codereview.appspot.com/265410043/diff/60001/lily/part-combine-iterator.cc#newcode53 lily/part-combine-iterator.cc:53: void start_mmrest (Context *c, const Moment &length); We don't pass Moment by reference. It's a simple scalar type and references reached via unsmob are rarely guaranteed to survive for long. https://codereview.appspot.com/265410043/diff/60001/lily/part-combine-iterator.cc#newcode132 lily/part-combine-iterator.cc:132: ev->set_property ("length", length.smobbed_copy()); A multi-measure rest event having a length (which is a cache of the "duration" converted to a Moment) but not a duration is very, very, weird. https://codereview.appspot.com/265410043/diff/60001/lily/part-combine-iterator.cc#newcode156 lily/part-combine-iterator.cc:156: { Setting "duration" to SCM_EOL is just wrong. Instead of setting lengths (assuming that a Moment is all you have available), you should use make-duration-of-length or similar in order to get a useful "duration" value. https://codereview.appspot.com/265410043/diff/60001/lily/part-combine-iterator.cc#newcode172 lily/part-combine-iterator.cc:172: Moment *rem = unsmob<Moment> (c->get_property ("multiMeasureRestBusy")); Please use robust_scm2moment here. https://codereview.appspot.com/265410043/diff/60001/ly/engraver-init.ly File ly/engraver-init.ly (right): https://codereview.appspot.com/265410043/diff/60001/ly/engraver-init.ly#newcode841 ly/engraver-init.ly:841: %% by the part combiner. Ugh. It would appear that multi-measure rest splitting (or other rest splitting) is only needed in the part combiner so why does it make sense to add all this complication to the Multi_measure_rest_engraver and let it idle in NullVoice? Why not keep the code for dealing with this in the part combiner and make the Multi_measure_rest_engraver just do what is expected from it? https://codereview.appspot.com/265410043/diff/60001/scm/define-context-properties.scm File scm/define-context-properties.scm (right): https://codereview.appspot.com/265410043/diff/60001/scm/define-context-properties.scm#newcode480 scm/define-context-properties.scm:480: (multiMeasureRestBusy ,ly:moment? "Remaining multi-measure rest duration.") It seems like a strange deal to maintain an engraver-specific property in a context property. Most of the time, such variables are kept inside the engraver. Stupid question: would the partcombiner work better by having a context PartCombinePart accepted by Voice, and then we use \change Voice = "soloII" and similar in the two original parts? Or would it make more sense to put something like PartCombinePart between Staff and Voice and run the original stuff in Voice, using \change PartCombinePart = "soloII" and so on? One of those might lead to a reasonably natural way of dealing with slurs and stuff. The reason I am asking in this context is that I think that you use multiMeasureRestBusy for transplanting the internal state of a Multi_measure_rest_engraver, and it may be expedient to instead transplant the whole engraver by an appropriate use of context hierarchy. Just some food for thought. https://codereview.appspot.com/265410043/ _______________________________________________ lilypond-devel mailing list [email protected] https://lists.gnu.org/mailman/listinfo/lilypond-devel
