On 29 nov. 2012, at 03:23, k-ohara5...@oco.net wrote: > > http://codereview.appspot.com/6827072/diff/18003/lily/box-quarantine.cc > File lily/box-quarantine.cc (right): > > http://codereview.appspot.com/6827072/diff/18003/lily/box-quarantine.cc#newcode69 > lily/box-quarantine.cc:69: int mid = ii0.mid_; > assert ((double)(ii0.index - mid) >= 0.0) > assert ((double)(ii1.index - mid) >= 0.0) >
This will fail often...it is possible, for example, that ii0.index is 0 and mid is 3 (mid represents the middle index of the vector). > http://codereview.appspot.com/6827072/diff/18003/lily/box-quarantine.cc#newcode93 > lily/box-quarantine.cc:93: Offset shift (-(epsilon + (ii[0].amount_ + > padding_) / 2.0), 0.0); > The padding appears in some places but not others; see > 'fingering-column.ly' > Fixed. > http://codereview.appspot.com/6827072/diff/18003/lily/skyline.cc > File lily/skyline.cc (right): > > http://codereview.appspot.com/6827072/diff/18003/lily/skyline.cc#newcode234 > lily/skyline.cc:234: // Flatten to height h > What is the difference between this and Skyline:::set_minimum_height() > at line 817 ? > Set minimum height allows for things over the minimum height to retain their skyline-ness, whereas this creates a flat skyline at height X. > http://codereview.appspot.com/6827072/diff/18003/scm/define-grob-properties.scm > File scm/define-grob-properties.scm (right): > > http://codereview.appspot.com/6827072/diff/18003/scm/define-grob-properties.scm#newcode492 > scm/define-grob-properties.scm:492: (horizon-padding ,number? "The > amount to pad the axis > We already have 'skyline-horizontal-padding' on line 815 and > 'skyline-vertical-padding', so it would be better to use those. You > could look up one or the other depending on which axis, X or Y, in which > the buildings grow. > > http://codereview.appspot.com/6827072/diff/18003/scm/define-grobs.scm > File scm/define-grobs.scm (right): > > http://codereview.appspot.com/6827072/diff/18003/scm/define-grobs.scm#newcode253 > scm/define-grobs.scm:253: (horizon-padding . 0.05) > This could be 'skyline-horizontal-padding' as in System and > VerticalAxisGroup. > I definitely agree, but this'd be for another patch set. The eventual goal of all of this is to get all spacing out of axis-group-interface and into side position interface via one general algorithm. When that happens, I'll make these merges. For now, it is a bit difficult, as there are two concurrent systems - one that specifies vertical versus horizontal padding and one that specifies padding versus horizon-padding. The latter system (side-position) probably needs to be changed, but it'd require a major convert-ly rule. > http://codereview.appspot.com/6827072/diff/18003/scm/define-grobs.scm#newcode886 > scm/define-grobs.scm:886: (add-stem-support . #t) > Several regression tests assume this is false by default, and then > toggle it to true, so as to test both cases. > > add-stem-support = #f doesn't seem to work very well with beams with > this patch. > One can write a lambda function that returns #t if there is a beam present and #f otherwise. Otherwise, the beam would have to be added at the engraver stage. I prefer the former. > http://codereview.appspot.com/6827072/diff/18003/scm/define-grobs.scm#newcode909 > scm/define-grobs.scm:909: (FingeringCollision > This would need a convert-ly rule. Why change the name ? > You recommended that in a previous review. I can push the convert-ly rule as a separate patch. > http://codereview.appspot.com/6827072/ _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
