Issue 4278058: Avoid repeats of 'staff-affinity' warning; change text. (Closed)
|
Created:
13 years, 1 month ago by Keith Modified:
12 years, 12 months ago CC:
lilypond-devel_gnu.org Visibility:
Public. |
DescriptionAvoid repeats of 'staff-affinity' warning; change text.
issue 1555
Patch Set 1 #Patch Set 2 : This is C, not Pascal #Patch Set 3 : no side effects if this function was asked to be 'pure' #Patch Set 4 : simply warn once #MessagesTotal messages: 17
LGTM I like this warning text. Much better. Trevor
Sign in to reply to this message.
On 2011/03/18 11:22:11, Trevor Daniels wrote: > LGTM > I like this warning text. Much better. > > Trevor Applying the patch gave the following: /home/colin/lilypond-git/lily/page-layout-problem.cc: In member function 'void Page_layout_problem::solve_rod_spring_problem(bool)': /home/colin/lilypond-git/lily/page-layout-problem.cc:425: warning: conversion to 'Real' from 'vsize' may alter its value /home/colin/lilypond-git/lily/page-layout-problem.cc:427: warning: conversion to 'Real' from 'vsize' may alter its value /home/colin/lilypond-git/lily/page-layout-problem.cc: In static member function 'static scm_unused_struct* Page_layout_problem::get_spacing_spec(Grob*, Grob*, bool, int, int)': /home/colin/lilypond-git/lily/page-layout-problem.cc:861: error: expected ';' before '}' token make[1]: *** [out/page-layout-problem.o] Error 1 make[1]: Leaving directory `/home/colin/lilypond-git/build/lily' make: *** [all] Error 2 colin@Sherlock:~/lilypond-git/build$
Sign in to reply to this message.
On 2011/03/19 23:37:50, Colin Campbell wrote: > error: expected ';' Oops. I guess you can't teach an old dog to use a new editor. Fixed, and I hope people do try it, because this method of suppressing the repeated warnings changes the output. Using this patch, LilyPond uniformly changes the 'staff-affinity that was giving her trouble, instead of just changing it when she notices trouble. Before the patch, LilyPond
Sign in to reply to this message.
LGTM. I can't think of a case where updating `after' would cause problems - this seems like a good solution.
Sign in to reply to this message.
On Sat, Mar 19, 2011 at 7:03 PM, <mtsolo@gmail.com> wrote: > LGTM. > I can't think of a case where updating `after' would cause problems - > this seems like a good solution. It might cause problems if "pure" is true. When the method is called with "pure," it shouldn't cause any side effects. For a concrete example, this will mess up if you have Staff Lyrics with affinity down Staff that sometimes disappears Lyrics with affinity up Staff because the staff that sometimes disappears will trigger the warning _before_ line breaking (with "pure" set to true). This can be fixed by adding if (!pure) { warning (...); after_affinity = before_affinity; } because there isn't really a need to print out the warning every time the pure version is called. Cheers, Joe
Sign in to reply to this message.
On 2011/03/20 05:46:51, joeneeman wrote: > This can be fixed by adding > if (!pure) Thanks very much. Just in case anybody wonders, we can safely put the assignment to after_affinity inside the if (!pure) with no change, because that assignment can't change behavior. If after_affinity > before_affinity then we know before_affinity != UP so we will return without ever using after_affinity.
Sign in to reply to this message.
On Sat, Mar 19, 2011 at 11:58 PM, <k-ohara5a5a@oco.net> wrote: > On 2011/03/20 05:46:51, joeneeman wrote: > >> This can be fixed by adding >> if (!pure) >> > > Thanks very much. > > Just in case anybody wonders, we can safely put the assignment to > after_affinity inside the if (!pure) with no change, because that > assignment can't change behavior. If after_affinity > before_affinity > then we know before_affinity != UP so we will return without ever > using after_affinity. In that case, is there any need to set after_affinity at all?
Sign in to reply to this message.
On Sun, 20 Mar 2011 00:17:48 -0700, Joe Neeman <joeneeman@gmail.com> wrote: > On Sat, Mar 19, 2011 at 11:58 PM, <k-ohara5a5a@oco.net> wrote: > > In that case, is there any need to set after_affinity at all? > I could ask the author of the original code the same question :) I think yes. As commentary. It assures the reader that LilyPond is proceeding as if after_affinity were changed.
Sign in to reply to this message.
On 2011/03/20 07:45:11, Keith wrote: > On Sun, 20 Mar 2011 00:17:48 -0700, Joe Neeman <mailto:joeneeman@gmail.com> wrote: > > On Sat, Mar 19, 2011 at 11:58 PM, <mailto:k-ohara5a5a@oco.net> wrote: > > > > In that case, is there any need to set after_affinity at all? > > > I could ask the author of the original code the same question :) Surely you aren't asking me to understand code I wrote months ago? :P > I think yes. As commentary. > It assures the reader that LilyPond is proceeding as if after_affinity were > changed. Wouldn't a comment be better then? The current discussion suggests that redundant code can actually create confusion...
Sign in to reply to this message.
On Sat, 19 Mar 2011 22:46:51 -0700, Joe Neeman <joeneeman@gmail.com> wrote: > > It might cause problems if "pure" is true. When the method is called with > "pure," it shouldn't cause any side effects. For a concrete example, this > will mess up if you have > > Staff > Lyrics with affinity down > Staff that sometimes disappears > Lyrics with affinity up > Staff Unfortunately, if I protect the assignment to the property with an if (!pure), I am letting the page-breaking planning rely on the user-requested affinities, and then changing them for the page-layout phase. The boolean 'pure' asks explicitly that we keep the state unchanged, but there was always an implicit expectation that certain properties are unchanging. On Mon, 21 Mar 2011 19:22:49 -0700, <joeneeman@gmail.com> wrote: > > Wouldn't a comment be better then? Yep. I'll add a comment next time I have Linux up. The only effect of the original code was to give warning, not to change the effective affinity, demonstrating that nothing explodes if 'staff-affinities increase'. Do you remember what the warning intended to protect against? The serious problem (coming up again in issue 1569) occurs when staff-affinity of the first or last non-staff points to a spaceable staff that is not present in the system. It seems this needs to be caught one or two layers up, in distribute_loose_lines() or maybe better in Align_interface::internal_get_minimum_translations() So I'll tidy up this small issue, but let's start looking at the problem of unrequited affinity of loose lines at the top/bottom of the system.
Sign in to reply to this message.
On Mon, Mar 21, 2011 at 8:11 PM, Keith OHara <k-ohara5a5a@oco.net> wrote: > On Sat, 19 Mar 2011 22:46:51 -0700, Joe Neeman <joeneeman@gmail.com> > wrote: > >> >> It might cause problems if "pure" is true. When the method is called with >> "pure," it shouldn't cause any side effects. For a concrete example, this >> will mess up if you have >> >> Staff >> Lyrics with affinity down >> Staff that sometimes disappears >> Lyrics with affinity up >> Staff >> > > Unfortunately, if I protect the assignment to the property with an if > (!pure), I am letting the page-breaking planning rely on the user-requested > affinities, and then changing them for the page-layout phase. The boolean > 'pure' asks explicitly that we keep the state unchanged, but there was > always an implicit expectation that certain properties are unchanging. I don't quite understand this comment. The only effect of set_property is to prevent the warning from being triggered more than once per system. In fact, the layout would be completely unchanged even if you removed the whole if(after_affinity > before_affinity) block. So why does it matter if we condition set_property on something? > > > On Mon, 21 Mar 2011 19:22:49 -0700, <joeneeman@gmail.com> wrote: > >> >> Wouldn't a comment be better then? >> > Yep. I'll add a comment next time I have Linux up. > > > The only effect of the original code was to give warning, not to change the > effective affinity, demonstrating that nothing explodes if 'staff-affinities > increase'. Do you remember what the warning intended to protect against? > I think it was just to let the user know not to expect a sane layout. The serious problem (coming up again in issue 1569) occurs when > staff-affinity of the first or last non-staff points to a spaceable staff > that is not present in the system. It seems this needs to be caught one or > two layers up, in distribute_loose_lines() or maybe better in > Align_interface::internal_get_minimum_translations() > > So I'll tidy up this small issue, but let's start looking at the problem of > unrequited affinity of loose lines at the top/bottom of the system. > I think the problem is that not enough space is being reserved in the first pass (ie. the non-loose lines part) of the layout and so the loose lines don't fit. Align_interface::internal_get_minimum_translations may be responsible, but it isn't actually used for distances between systems, so it's also worth investigating bottom_skyline_. Unfortunately, I'm going backpacking starting tomorrow, but I'm happy to answer questions once I get back (Sunday). Cheers, Joe
Sign in to reply to this message.
On Mon, 21 Mar 2011 23:06:12 -0700, Joe Neeman <joeneeman@gmail.com> wrote: > >> Unfortunately, if I protect the assignment to the property with an if >> (!pure), I am letting the page-breaking planning rely on the user-requested >> affinities, and then changing them for the page-layout phase. The boolean >> 'pure' asks explicitly that we keep the state unchanged, but there was >> always an implicit expectation that certain properties are unchanging. > > > I don't quite understand this comment. The only effect of set_property is to > prevent the warning from being triggered more than once per system. In fact, > the layout would be completely unchanged even if you removed the whole > if(after_affinity > before_affinity) block. So why does it matter if we > condition set_property on something? > In a typical case, the effect is after->set_property("staff-affinity", DOWN) and then when get_spacing_spec is called to determine the next spring, working through the (non)staffs in the system, it will look up the staff affinity we just set for use as before_affinity, thus the next spring is changed. The code in the if(after_affinity>before_affinity) block does change the output for the example at the head of issue 1555.
Sign in to reply to this message.
On Mon, Mar 21, 2011 at 11:18 PM, Keith OHara <k-ohara5a5a@oco.net> wrote: > On Mon, 21 Mar 2011 23:06:12 -0700, Joe Neeman <joeneeman@gmail.com> > wrote: > > >> Unfortunately, if I protect the assignment to the property with an if >>> (!pure), I am letting the page-breaking planning rely on the >>> user-requested >>> affinities, and then changing them for the page-layout phase. The >>> boolean >>> 'pure' asks explicitly that we keep the state unchanged, but there was >>> always an implicit expectation that certain properties are unchanging. >>> >> >> >> I don't quite understand this comment. The only effect of set_property is >> to >> prevent the warning from being triggered more than once per system. In >> fact, >> the layout would be completely unchanged even if you removed the whole >> if(after_affinity > before_affinity) block. So why does it matter if we >> condition set_property on something? >> >> In a typical case, the effect is after->set_property("staff-affinity", > DOWN) > and then when get_spacing_spec is called to determine the next spring, > working through the (non)staffs in the system, it will look up the staff > affinity we just set for use as before_affinity, thus the next spring is > changed. The code in the if(after_affinity>before_affinity) block does > change the output for the example at the head of issue 1555. > Ok, good point. In that case, a better way to avoid too many warnings might be just to add a static bool to check if a warning has already been issued. Joe
Sign in to reply to this message.
On 2011/03/22 06:28:42, joeneeman wrote: > In that case, a better way to avoid too many warnings might > be just to add a static bool to check if a warning has already > been issued. Done, and removed the original code that had no effect. Given that the old code merely printed warning, the warning text about "incompatible" settings is not really accurate; a single output of the original warning text should be fine.
Sign in to reply to this message.
LGTM.
Sign in to reply to this message.
LGTM.
Sign in to reply to this message.
|
