Re: Creates a Flag grob. (issue 4922042)
Pushed as f0978ed121192fee9bdf2453a325d98693148acf. Cheers, MS http://codereview.appspot.com/4922042/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Creates a Flag grob. (issue 4922042)
On Aug 25, 2011, at 12:03 PM, Mike Solomon wrote: On Aug 24, 2011, at 11:53 PM, n.putt...@gmail.com wrote: Thanks for your suggestions Neil! I'm holding off on pushing the patch because I have noticed a spacing discrepancy in a few regtests. See the attached, where old is current master and new is with my patch. The spacing between the last 16th note and the quarter is too tight. I have verified via pacifier prints that the pure heights of the flag are being taken from the stencil function and are going into the spacing engine in separation-item.cc. However, in theory, this patch should have null effect on the minimum and ideal distances of paper columns. This is not the case (see attached). Happily (bizarrely), if I add the flag to the NoteColumn's elements grob-array in the rhythmic-column engraver, this problem goes away in the opposite sense: the paper column's minimum and ideal distances become wider (see attached). I'm tending towards doing this, but it also seems subpar (unless someone can vouch that the previous spacing somehow mishandled flags). If anyone has any intuition as to why this is happening, I'd be much obliged if you'd share it with me. Cheers, MS Addendum - it appears that the width of the paper column of a 16th note + flag in current master is 2.1522 whereas in with my flag work it is 2.2172. This is the length of the discrepancy between paper column ideal and minimum distances when the flag is added to the note head's element list. It is also the length (0.0650) of the x offset of the flag with respect to its x parent, the stem. Current master's width is correct, but oddly, the two print out the same results on the page (at least they look the same in terms of the flags x offset with respect to the stem). That means that, somehow, the flag's x offset with respect to its parent is being tacked on in a way it shouldn't. I'll keep digging (as best I can) and keep reporting results as I find them. Cheers, MS ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Creates a Flag grob. (issue 4922042)
On Thu, Aug 25, 2011 at 7:03 AM, Mike Solomon mike...@ufl.edu wrote: I have verified via pacifier prints that the pure heights of the flag are being taken from the stencil function and are going into the spacing engine in separation-item.cc. However, in theory, this patch should have null effect on the minimum and ideal distances of paper columns. This is not the case (see attached). ? it's not the heights but the widths that should go into separation-item? Have you verified that the new flag grob gets taken into account in note-spacing.cc ? -- Han-Wen Nienhuys - han...@xs4all.nl - http://www.xs4all.nl/~hanwen ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Creates a Flag grob. (issue 4922042)
On Aug 25, 2011, at 3:17 PM, Han-Wen Nienhuys wrote: On Thu, Aug 25, 2011 at 7:03 AM, Mike Solomon mike...@ufl.edu wrote: I have verified via pacifier prints that the pure heights of the flag are being taken from the stencil function and are going into the spacing engine in separation-item.cc. However, in theory, this patch should have null effect on the minimum and ideal distances of paper columns. This is not the case (see attached). ? it's not the heights but the widths that should go into separation-item? Have you verified that the new flag grob gets taken into account in note-spacing.cc ? I found the problem after a couple hours of banging my head against the wall. The Stem::width function uses Stem::flag to get the flag, whereas the stencil uses Stem::get_translated_flag. Thus, the flag used for widths is not translated. As the flag's bounding box in the font is shifted by a half stem length to the right of where the glyph actually is, and as the translated flag was always shifted by half a stem width, this is what made it work (albeit kludgily). I'll work this into the patch tonight or tomorrow. Cheers, MS ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Creates a Flag grob. (issue 4922042)
On Aug 25, 2011, at 9:08 PM, Mike Solomon wrote: On Aug 25, 2011, at 3:17 PM, Han-Wen Nienhuys wrote: On Thu, Aug 25, 2011 at 7:03 AM, Mike Solomon mike...@ufl.edu wrote: I have verified via pacifier prints that the pure heights of the flag are being taken from the stencil function and are going into the spacing engine in separation-item.cc. However, in theory, this patch should have null effect on the minimum and ideal distances of paper columns. This is not the case (see attached). ? it's not the heights but the widths that should go into separation-item? Have you verified that the new flag grob gets taken into account in note-spacing.cc ? I found the problem after a couple hours of banging my head against the wall. The Stem::width function uses Stem::flag to get the flag, whereas the stencil uses Stem::get_translated_flag. Thus, the flag used for widths is not translated. As the flag's bounding box in the font is shifted by a half stem length to the right of where the glyph actually is, and as the translated flag was always shifted by half a stem width, this is what made it work (albeit kludgily). I'll work this into the patch tonight or tomorrow. Cheers, MS Hey all, I've uploaded a new patch that reproduces the spacing calculations from the current master with a TODO item to fix this. I'm comfortable pushing this (after running the regtests again) in its current form even though it does not fix this problem - it simply transfers it to the new code. I would like to fix the problem, though, in the not-too-distant future. Cheers, MS ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Creates a Flag grob. (issue 4922042)
http://codereview.appspot.com/4922042/diff/17001/lily/flag.cc
File lily/flag.cc (right):
http://codereview.appspot.com/4922042/diff/17001/lily/flag.cc#newcode147
lily/flag.cc:147: what style of flag glyph is typeset on a
what
http://codereview.appspot.com/4922042/diff/17001/lily/flag.cc#newcode148
lily/flag.cc:148: @code{Stem}. Valid options include @code{'()}
@code{Stem}.
http://codereview.appspot.com/4922042/diff/17001/lily/flag.cc#newcode149
lily/flag.cc:149: for standard flags, @code{'mensural} and
for
http://codereview.appspot.com/4922042/diff/17001/lily/flag.cc#newcode150
lily/flag.cc:150: @code{'no-flag}, which switches off the flag.,
@code{'no-flag}
http://codereview.appspot.com/4922042/diff/17001/lily/flag.cc#newcode154
lily/flag.cc:154: stem
unused
http://codereview.appspot.com/4922042/diff/17001/lily/stem-engraver.cc
File lily/stem-engraver.cc (right):
http://codereview.appspot.com/4922042/diff/17001/lily/stem-engraver.cc#newcode191
lily/stem-engraver.cc:191: if (scm_is_string (get_property
(whichBar)))
doc whichBar as read property
http://codereview.appspot.com/4922042/diff/17001/lily/stem.cc
File lily/stem.cc (left):
http://codereview.appspot.com/4922042/diff/17001/lily/stem.cc#oldcode1110
lily/stem.cc:1110: flag
restore
http://codereview.appspot.com/4922042/diff/17001/ly/engraver-init.ly
File ly/engraver-init.ly (right):
http://codereview.appspot.com/4922042/diff/17001/ly/engraver-init.ly#newcode678
ly/engraver-init.ly:678: (Voice Flag font-size -3)
also add to make-voice-props-set
http://codereview.appspot.com/4922042/diff/17001/scm/define-grob-properties.scm
File scm/define-grob-properties.scm (left):
http://codereview.appspot.com/4922042/diff/17001/scm/define-grob-properties.scm#oldcode275
scm/define-grob-properties.scm:275: (flag ,ly:stencil? A function
returning the full flag stencil
move to internal properties and change type-predicate and docstring,
e.g.,
(flag ,ly:grob? A pointer to a @code{Flag} object.)
http://codereview.appspot.com/4922042/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Creates a Flag grob. (issue 4922042)
On 2011/08/22 01:56:23, hanwenn wrote: quick remarks - Why are the flags called maybeflags in the engraver? Because we don't know if they are for-real flags until auto beaming has finished doing its thing (and all of the flags with beams are killed). I can change it to flags_ or flags_to_maybe_kill_. - Currently, the stem already does width/2 X-offset, can't you piggyback on that? Yes (I'll write this into the code and you'll see it in a new patch after I get more comments). - You're copying me and jan's name in the header. If anythingn, you should probably put your own. OK - I figured put yours as all I did was copy and paste the code, but I can do this. (we could consider just using The lilypond authors as name at the top everywhere). I'm a fan of this. git blame already shows who did what (that said, I was git blaming the other day and a lot of things show up as Graham Percival because of the great indenting push of 2011 - is there a way to see multiple layers in git blame?). Cheers, MS http://codereview.appspot.com/4922042/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Creates a Flag grob. (issue 4922042)
Am Monday, 22. August 2011, 08:16:47 schrieb mts...@gmail.com: I'm a fan of this. git blame already shows who did what (that said, I was git blaming the other day and a lot of things show up as Graham Percival because of the great indenting push of 2011 - is there a way to see multiple layers in git blame?). In qgit you can select for each file for which commit the blame should be done. http://sourceforge.net/project/screenshots.php?group_id=139897ssid=33929 So you can select the commit before Graham's indentation changes.. (And if someone did some other innocent change to the relevant code, you see the commit and can select the patch before that commit, until you finally see who introduced something into the code). Chers, Reinhold -- -- Reinhold Kainhofer, reinh...@kainhofer.com, http://reinhold.kainhofer.com/ * Financial Actuarial Math., Vienna Univ. of Technology, Austria * http://www.fam.tuwien.ac.at/, DVR: 0005886 * LilyPond, Music typesetting, http://www.lilypond.org ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Creates a Flag grob. (issue 4922042)
http://codereview.appspot.com/4922042/diff/1/input/regression/color.ly File input/regression/color.ly (right): http://codereview.appspot.com/4922042/diff/1/input/regression/color.ly#newcode24 input/regression/color.ly:24: \override Flag #'color = #blue Why don't you choose a different color to check that the two grobs are really handled differently? http://codereview.appspot.com/4922042/diff/1/input/regression/flags-default.ly File input/regression/flags-default.ly (right): http://codereview.appspot.com/4922042/diff/1/input/regression/flags-default.ly#newcode32 input/regression/flags-default.ly:32: \override Flag #'flag-style = #'mensural Can't we rename that to to style now? http://codereview.appspot.com/4922042/diff/1/lily/flag.cc File lily/flag.cc (right): http://codereview.appspot.com/4922042/diff/1/lily/flag.cc#newcode54 lily/flag.cc:54: '() or grace). */ That comment should be moved down to stroke-style http://codereview.appspot.com/4922042/diff/1/lily/flag.cc#newcode57 lily/flag.cc:57: SCM flag_style_scm = me-get_property (flag-style); Rename property to style http://codereview.appspot.com/4922042/diff/1/lily/flag.cc#newcode97 lily/flag.cc:97: if (scm_is_string (stroke_style_scm)) Sooner or later, that should be moved out from here into its own grob. Then we can also have slashed beamed grace notes (which don't have any flag). http://codereview.appspot.com/4922042/diff/1/lily/flag.cc#newcode149 lily/flag.cc:149: flag-style Rename that to style. http://codereview.appspot.com/4922042/diff/1/lily/tie-formatting-problem.cc File lily/tie-formatting-problem.cc (right): http://codereview.appspot.com/4922042/diff/1/lily/tie-formatting-problem.cc#newcode177 lily/tie-formatting-problem.cc:177: boxes.push_back (Box (flag-extent (x_refpoint_, X_AXIS), flag-extent (commony, Y_AXIS))); Line too long http://codereview.appspot.com/4922042/diff/1/ly/engraver-init.ly File ly/engraver-init.ly (right): http://codereview.appspot.com/4922042/diff/1/ly/engraver-init.ly#newcode787 ly/engraver-init.ly:787: \override Flag #'flag-style = #'no-flag rename to style http://codereview.appspot.com/4922042/diff/1/ly/engraver-init.ly#newcode787 ly/engraver-init.ly:787: \override Flag #'flag-style = #'no-flag Do we need/want a convert-ly warning for \override Stem #'flag-style? http://codereview.appspot.com/4922042/diff/1/ly/property-init.ly File ly/property-init.ly (right): http://codereview.appspot.com/4922042/diff/1/ly/property-init.ly#newcode429 ly/property-init.ly:429: \revert TabVoice.Flag #'flag-style rename to style http://codereview.appspot.com/4922042/diff/1/scm/define-grob-properties.scm File scm/define-grob-properties.scm (right): http://codereview.appspot.com/4922042/diff/1/scm/define-grob-properties.scm#newcode276 scm/define-grob-properties.scm:276: (flag-style ,symbol? A symbol determining what style of flag rename to style? Unfortunately, then we don't have any documentation any more about the valid values... :( http://codereview.appspot.com/4922042/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Creates a Flag grob. (issue 4922042)
On Aug 22, 2011, at 12:06 PM, reinhold.kainho...@gmail.com wrote: http://codereview.appspot.com/4922042/diff/1/input/regression/color.ly File input/regression/color.ly (right): http://codereview.appspot.com/4922042/diff/1/input/regression/color.ly#newcode24 input/regression/color.ly:24: \override Flag #'color = #blue Why don't you choose a different color to check that the two grobs are really handled differently? Done. http://codereview.appspot.com/4922042/diff/1/lily/flag.cc File lily/flag.cc (right): http://codereview.appspot.com/4922042/diff/1/lily/flag.cc#newcode54 lily/flag.cc:54: '() or grace). */ That comment should be moved down to stroke-style Done. http://codereview.appspot.com/4922042/diff/1/lily/flag.cc#newcode97 lily/flag.cc:97: if (scm_is_string (stroke_style_scm)) Sooner or later, that should be moved out from here into its own grob. Then we can also have slashed beamed grace notes (which don't have any flag). Agreed. http://codereview.appspot.com/4922042/diff/1/lily/tie-formatting-problem.cc File lily/tie-formatting-problem.cc (right): http://codereview.appspot.com/4922042/diff/1/lily/tie-formatting-problem.cc#newcode177 lily/tie-formatting-problem.cc:177: boxes.push_back (Box (flag-extent (x_refpoint_, X_AXIS), flag-extent (commony, Y_AXIS))); Line too long Fixed. http://codereview.appspot.com/4922042/diff/1/ly/engraver-init.ly#newcode787 ly/engraver-init.ly:787: \override Flag #'flag-style = #'no-flag Do we need/want a convert-ly warning for \override Stem #'flag-style? Yes - I'd put this in a separate patch/issue (unless people think it belongs here - it seems that convert-ly rules usually come after patches get pushed). http://codereview.appspot.com/4922042/diff/1/scm/define-grob-properties.scm File scm/define-grob-properties.scm (right): http://codereview.appspot.com/4922042/diff/1/scm/define-grob-properties.scm#newcode276 scm/define-grob-properties.scm:276: (flag-style ,symbol? A symbol determining what style of flag rename to style? Unfortunately, then we don't have any documentation any more about the valid values... :( You've hit the proverbial nail on its proverbial head. It is for this reason that I'm wary about renaming. New patchset up, and thanks for your comments! Cheers, MS ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Creates a Flag grob. (issue 4922042)
On Mon, Aug 22, 2011 at 7:20 AM, Mike Solomon mike...@ufl.edu wrote: http://codereview.appspot.com/4922042/diff/1/scm/define-grob-properties.scm#newcode276 scm/define-grob-properties.scm:276: (flag-style ,symbol? A symbol determining what style of flag rename to style? Unfortunately, then we don't have any documentation any more about the valid values... :( You've hit the proverbial nail on its proverbial head. It is for this reason that I'm wary about renaming. Yes, but please rename anyway. You could put the 'style values in the doc of flag-interface. -- Han-Wen Nienhuys - han...@xs4all.nl - http://www.xs4all.nl/~hanwen ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Creates a Flag grob. (issue 4922042)
On 2011/08/22 12:52:05, hanwenn wrote: On Mon, Aug 22, 2011 at 7:20 AM, Mike Solomon mailto:mike...@ufl.edu wrote: http://codereview.appspot.com/4922042/diff/1/scm/define-grob-properties.scm#newcode276 scm/define-grob-properties.scm:276: (flag-style ,symbol? A symbol determining what style of flag rename to style? Unfortunately, then we don't have any documentation any more about the valid values... :( You've hit the proverbial nail on its proverbial head. nbsp;It is for this reason that I'm wary about renaming. Yes, but please rename anyway. You could put the 'style values in the doc of flag-interface. -- Han-Wen Nienhuys - mailto:han...@xs4all.nl - http://www.xs4all.nl/%7Ehanwen Done. Cheers, MS http://codereview.appspot.com/4922042/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Creates a Flag grob. (issue 4922042)
quick remarks - Why are the flags called maybeflags in the engraver? - Currently, the stem already does width/2 X-offset, can't you piggyback on that? - You're copying me and jan's name in the header. If anythingn, you should probably put your own. (we could consider just using The lilypond authors as name at the top everywhere). On Sun, Aug 21, 2011 at 6:26 PM, mts...@gmail.com wrote: Reviewers: , Message: This should do the trick. The regtests don't really help as they show tons of fake changes. I'll try Han Wen's suggestion for hacking output-distance.py in the next week if I am still having trouble. The only thing that currently seems broken is stem-tremolo-note-column.ly. This is because, as I state in my patch on stem-tremolos, the stem tremolo pure height function is broken. So, I don't consider this a regression... The order that this stuff would be pushed is: This patch The stem changes My rewrite for stem-tremolo pure height stuff, which would be rewritten yet again to be cleaner and more accurate in function of the pure height changes to stem.cc Cheers, MS Description: Creates a Flag grob. Please review this at http://codereview.appspot.com/4922042/ Affected files: M input/regression/color.ly M input/regression/flags-default.ly M input/regression/flags-in-scheme.ly M input/regression/flags-straight-stockhausen-boulez.ly M input/regression/flags-straight.ly M input/regression/graphviz.ly M input/regression/grid-lines.ly M input/regression/les-nereides.ly M input/regression/mozart-hrn3-defs.ily M input/regression/quote-overrides.ly M lily/beam-quanting.cc M lily/dot-column.cc A lily/flag.cc M lily/include/stem.hh M lily/stem-engraver.cc M lily/stem.cc M lily/tie-formatting-problem.cc M ly/engraver-init.ly M ly/grace-init.ly M ly/gregorian.ly M ly/property-init.ly M scm/define-grob-properties.scm M scm/define-grobs.scm M scm/flag-styles.scm ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel -- Han-Wen Nienhuys - han...@xs4all.nl - http://www.xs4all.nl/~hanwen ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
