Issue 9295044: stencil-integral: use box extents specified in markup; issue 3255 (Closed)
DescriptionImplement the stencil tag 'with-dimensions to support stencil expressions created with functions like \pad-to-box
Patch Set 1 #Patch Set 2 : tag stencil expressions when the extents are set explicitly #Patch Set 3 : understandable regression test #Patch Set 4 : add a box to preserve the effect of \pad-around #
Total comments: 1
Patch Set 5 : rebase #Patch Set 6 : carry extents with the stencil, no extra .ps output #Patch Set 7 : , #
Total comments: 1
Patch Set 8 : remove documentation #Patch Set 9 : rerename #
Total comments: 5
Patch Set 10 : still checking `make doc` #Patch Set 11 : rebase #
MessagesTotal messages: 29
The code looks reasonable, at least to me :-) Thanks for working on this. https://codereview.appspot.com/9295044/diff/11001/input/regression/text-scrip... File input/regression/text-script-vertical-skylines.ly (right): https://codereview.appspot.com/9295044/diff/11001/input/regression/text-scrip... input/regression/text-script-vertical-skylines.ly:6: spacing reservation of a stencil, while @code{with-dimensions} completely This is hard to understand IMHO. What about @code{pad-around} computes the (smallest) bounding box of a stencil and pads this box by the given amount. @code{with-dimensions} completely...
Sign in to reply to this message.
On Sun, 09 Jun 2013 12:37:27 -0700, <lemzwerg@googlemail.com> wrote: > https://codereview.appspot.com/9295044/diff/11001/input/regression/text-scrip... > > This is hard to understand IMHO. What about > > @code{pad-around} computes the (smallest) bounding box of a stencil > and pads this box by the given amount. @code{with-dimensions} > completely... > The point of this test, though, is not how the box is computed (which is done differently for the different \pad-* functions). The point is to check that these boxes show up in the skylines, so that they have effect if the stencil ends up in a TextScript spaced around music. I'll reword that doc-string with the forthcoming patch-set that cleans up the PostScript output. One possibly-interesting choice is whether the stencil-padding functions should always replace the skyline with a box, or make the union of the skyline with the box. Clearly \with-dimensions needs to replace the skyline with a box for your kludge \with-dimensions #'(0 . 0) #'(0 . 0) . However, what would you expect from {c'1^\markup{ \pad-to-box #'(-1 . 5) #'(0 . 1) "High" }} ? I think I should pass both the box and the "High" to the skyline-creator so that we use their union in this case.
Sign in to reply to this message.
> One possibly-interesting choice is whether the stencil-padding functions
should
> always replace the skyline with a box, or make the union of the skyline with
the
> box. Clearly \with-dimensions needs to replace the skyline with a box for
your
> kludge \with-dimensions #'(0 . 0) #'(0 . 0) . However, what would you expect
> from {c'1^\markup{ \pad-to-box #'(-1 . 5) #'(0 . 1) "High" }} ? I think I
> should pass both the box and the "High" to the skyline-creator so that we use
> their union in this case.
Yes, the union, since the documentation of \pad-to box says `at least the X-EXT,
Y-EXT space'. However, the documentation strings for all \pad-XXX functions
should be updated accordingly. In particular, I can't see any difference
between \pad-around and \pad-markup. Uh, oh, looking closely at the source code
I see that they are completely identical, even after your patch! This should
probably be fixed too, both in the docs and in the code...
Sign in to reply to this message.
On 2013/06/11 06:40:13, lemzwerg wrote: > Yes, the union, since the documentation of \pad-to box says `at least the X-EXT, > Y-EXT space'. However, the documentation strings for all \pad-XXX functions > should be updated accordingly. But \pad-around #-1 does work to reduce the extent of markup, and there is no reason to disallow that. > In particular, I can't see any difference > between \pad-around and \pad-markup. Uh, oh, looking closely at the source code > I see that they are completely identical, even after your patch! This should > probably be fixed too, both in the docs and in the code... There is no harm in having both. There might be people who habitually use each, and we cannot convert-ly their brains to use whichever one we pick.
Sign in to reply to this message.
> But \pad-around #-1 does work to reduce the extent of markup, > and there is no reason to disallow that. OK. However, we need good documentation examples... > > In particular, I can't see any difference > > between \pad-around and \pad-markup. > There is no harm in having both. Of course not, but the equality of two commands should (a) be documented as such and (b) have *exactly* the same scheme code.
Sign in to reply to this message.
On 2013/06/11 16:15:45, Keith wrote: > > I see that they are completely identical, even after your patch! This should > > probably be fixed too, both in the docs and in the code... > > There is no harm in having both. There might be people who habitually use > each, and we cannot convert-ly their brains to use whichever one we pick. I disagree. There is harm in having both since it makes people think about which to use in which situation. Since we have \pad-x and \pad-y, \pad-around makes more sense to keep. Not only does the name help with knowing just what is padded, but also we don't tend to put "markup" in command names redundantly. If we don't like convert-ly, we can just keep an undocumented compatibility function. Or document it as "exists only for historical reasons and is the same as pad-around".
Sign in to reply to this message.
dak@gnu.org wrote Wednesday, June 12, 2013 6:42 AM > I disagree. There is harm in having both since it makes people think > about which to use in which situation. Since we have \pad-x and \pad-y, > \pad-around makes more sense to keep. Not only does the name help with > knowing just what is padded, but also we don't tend to put "markup" in > command names redundantly. > > If we don't like convert-ly, we can just keep an undocumented > compatibility function. Or document it as "exists only for historical > reasons and is the same as pad-around". I agree. Leave it available but documented as David suggests only in code comments. > https://codereview.appspot.com/9295044/ > > _______________________________________________ > lilypond-devel mailing list > lilypond-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/lilypond-devel
Sign in to reply to this message.
https://codereview.appspot.com/9295044/diff/37001/scm/define-markup-commands.scm File scm/define-markup-commands.scm (right): https://codereview.appspot.com/9295044/diff/37001/scm/define-markup-commands.... scm/define-markup-commands.scm:1911: (list 'explicit-extent-stencil x y (ly:stencil-expr m)) why not 'with-dimensions instead of 'explicit-extent-stencil?
Sign in to reply to this message.
On 2013/06/12 19:02:55, dak wrote: > https://codereview.appspot.com/9295044/diff/37001/scm/define-markup-commands.scm > File scm/define-markup-commands.scm (right): > > https://codereview.appspot.com/9295044/diff/37001/scm/define-markup-commands.... > scm/define-markup-commands.scm:1911: (list 'explicit-extent-stencil x y > (ly:stencil-expr m)) > why not 'with-dimensions instead of 'explicit-extent-stencil? It could be 'dimension-stencil as it lives alongside 'rotate-stencil, 'scale-stencil, etc.,
Sign in to reply to this message.
On 2013/06/13 07:45:09, Keith wrote: > On 2013/06/12 19:02:55, dak wrote: > > > https://codereview.appspot.com/9295044/diff/37001/scm/define-markup-commands.scm > > File scm/define-markup-commands.scm (right): > > > > > https://codereview.appspot.com/9295044/diff/37001/scm/define-markup-commands.... > > scm/define-markup-commands.scm:1911: (list 'explicit-extent-stencil x y > > (ly:stencil-expr m)) > > why not 'with-dimensions instead of 'explicit-extent-stencil? > > It could be 'dimension-stencil as it lives alongside 'rotate-stencil, > 'scale-stencil, etc., Well, "rotate" and "scale" are verbs and "dimension" isn't, so that particular form has me less than enthused. I am also not a fan of inventing new and unrelated naming schemes for facets of implementing the same functionality. Somewhat in line with rotate and scale would seem to be with-dimensions-stencil, combining the markup name with "stencil".
Sign in to reply to this message.
"Keith OHara" <k-ohara5a5a@oco.net> writes: > On Thu, 13 Jun 2013 01:02:34 -0700, <dak@gnu.org> wrote: > >> On 2013/06/13 07:45:09, Keith wrote: >> >>> It could be 'dimension-stencil as it lives alongside 'rotate-stencil, >>> 'scale-stencil, etc., >> >> Well, "rotate" and "scale" are verbs and "dimension" isn't, so that >> particular form has me less than enthused. I am also not a fan of >> inventing new and unrelated naming schemes for facets of implementing >> the same functionality. >> > > Okay. It implements \pad-around so 'pad-around-stencil it is. At the danger of sounding like an ..., it doesn't. The arguments are wrong for that. \pad-around is implemented in terms of \with-dimensions (not literally but functionally), not the other way round. While one could also provide pad-around-stencil with the semantics of \pad-around, keeping the number of primitives to be implemented in every backend to a minimum seems prudent. -- David Kastrup
Sign in to reply to this message.
https://codereview.appspot.com/9295044/diff/51001/scm/harp-pedals.scm File scm/harp-pedals.scm (right): https://codereview.appspot.com/9295044/diff/51001/scm/harp-pedals.scm#newcode128 scm/harp-pedals.scm:128: (apply ly:stencil-add Uh, (apply x (cons* y z t)) is just the same as (apply x y z t) isn't it?
Sign in to reply to this message.
https://codereview.appspot.com/9295044/diff/51001/lily/stencil-integral.cc File lily/stencil-integral.cc (right): https://codereview.appspot.com/9295044/diff/51001/lily/stencil-integral.cc#ne... lily/stencil-integral.cc:385: Interval x_ext = robust_scm2interval (scm_car (expr), Interval () ); Ok, you'll probably hate me for this... But wouldn't it make sense to allow for extents and/or extent bounds to be #f and/or #t in order to indicate being unbounded and/or prefer the skyline? It looks at least somewhat awkward that one can't just replace the horizontal or vertical skyline alone with fixed extents but always has to do it in tandem.
Sign in to reply to this message.
https://codereview.appspot.com/9295044/diff/51001/scm/define-markup-commands.scm File scm/define-markup-commands.scm (right): https://codereview.appspot.com/9295044/diff/51001/scm/define-markup-commands.... scm/define-markup-commands.scm:1913: #:properties (pad-around-markup) Remove this #:properties line. It makes no sense whatsoever.
Sign in to reply to this message.
On 2013/06/13 16:40:46, dak wrote: > https://codereview.appspot.com/9295044/diff/51001/scm/define-markup-commands.scm > File scm/define-markup-commands.scm (right): > > https://codereview.appspot.com/9295044/diff/51001/scm/define-markup-commands.... > scm/define-markup-commands.scm:1913: #:properties (pad-around-markup) > Remove this #:properties line. It makes no sense whatsoever. Having read the documentation of define-markup-command, I have to take back the "makes no sense whatsoever". Could not remember that bit for the life of me. I am not sure we are using this elsewhere, and it would pretty certainly require pad-around-markup to be defined before pad-markup-markup. But you are using this definitely according to instructions. I think it would likely be worth checking whether replacing this fixes the compilation problem: the sort of bookkeeping that is done here uses weak hashtables, so the error message you got would be consistent with a problem in this area. If that proves to be the case, I'd volunteer for more thorough research into this: I am afraid that this code might be my responsibility to a good degree.
Sign in to reply to this message.
I'll have time to try make doc and re-upload, this weekend. https://codereview.appspot.com/9295044/diff/51001/lily/stencil-integral.cc File lily/stencil-integral.cc (right): https://codereview.appspot.com/9295044/diff/51001/lily/stencil-integral.cc#ne... lily/stencil-integral.cc:385: Interval x_ext = robust_scm2interval (scm_car (expr), Interval () ); On 2013/06/13 15:36:26, dak wrote: > It looks at least somewhat awkward that one can't just replace the horizontal or > vertical skyline alone with fixed extents but always has to do it in tandem. I don't understand the concept. For graphical objects, we can set 'vertical-skylines separately from 'horizontal- to independently choose whether to look into the stencil, or just the extents, to trace each skyline. In stencils, there is an aggregate of drawing commands that are interpreted for the various output targets, and also interpreted here to find their outlines and trace skylines. At this point in the code we came across a section of drawing commands started with 'with-dimensions-stencil: "please use this box, in place of the actual outline of the following sub-stencil". What is an example of how we could act upon seeing #f or #t as a dimensions of that box? https://codereview.appspot.com/9295044/diff/51001/scm/define-markup-commands.scm File scm/define-markup-commands.scm (right): https://codereview.appspot.com/9295044/diff/51001/scm/define-markup-commands.... scm/define-markup-commands.scm:1913: #:properties (pad-around-markup) On 2013/06/13 16:40:46, dak wrote: > Remove this #:properties line. Yep. That was probably the problem.
Sign in to reply to this message.
On 2013/06/14 02:32:29, Keith wrote: > I'll have time to try make doc and re-upload, this weekend. I could not yet get `make doc` to complete, probably due to some problem in my build-directory setup.
Sign in to reply to this message.
Tidy the code, and documentation, in hopes of a clean patchy make doc. For me, `make LANGS="" doc` succeeds with no errors.
Sign in to reply to this message.
make doc, all languages, also succeeds
Sign in to reply to this message.
I saw that there were some translation merges and David put in his elisp fix as well, but I think I already ran my tests against that yesterday. Anyway, to save time for pathcy, when ever I see a merge has taken place I test a random patch (regardless if it fails) so that Patchy has a new baseline to make check against when real patches come around - saves about 20 minutes for the first new patch since a merge. Anyway, just picked this patch again and it failed make doc, so I did it again and it still failed make doc. I've not yet dug through the logs, but could this be anything to do with the last two checkins? I'm having a hard time thinking it could but would like a clarification as I am at a loss to this apparent randomness of failure. I know how troublesome it is for others to pinpoint make doc issues with incremental changes because it is so time consuming. .... I looked at the logs in more detail again and still it seems to start to process chords-funky-ignatzek.ly then I get the child returned error at this point - same as a few days ago. Is there something special about this reg test that might break something intermittently? http://lilypond.org/doc/v2.17/input/regression/38/lily-1d33dc26.ly It looks like something specifically TextScript based
Sign in to reply to this message.
LGTM.
Sign in to reply to this message.
Untangling the stencil regressions. The commit that made skylines from stencils (issue 2148) omitted space-reservations for stencils that: + are not drawn (\transparent) + have unknown contents until after layout (\page-ref and similar) or + had their space reservation modified (\pad-around and similar). It makes sense to put information to restore these space-reservations in the stencil-expression, because that is where the skyline-building code looks for them. We could encode the information in the stencil expression as: 1) a 'with-dimensions primitive with box-dimensions, to be read by the skyline code to restore the former behavior (this patch) 2) a 'transparent primitive with a stencil subexpression, to be read by the skyline code but not printed (http://codereview.appspot.com/13416044/) Both work for me. Supposing for the moment that plan (2) meets resistance, does this code for plan (1) look good?
Sign in to reply to this message.
Conceptually, I prefer (1), but this is based on your descriptions and the previous discussion, not on understanding the code...
Sign in to reply to this message.
On Thu, 29 Aug 2013 21:24:25 -0700, <lemzwerg@googlemail.com> wrote: > Conceptually, I prefer (1), but this is based on your descriptions and > the previous discussion, not on understanding the code... > Then look at (2) and see if you think that would be good enough to clear the last bugs before 2.18. The uses of \transparent, \pad-to-box, etc., are rare. It hurts very little to find ourselves stuck with a second-choice implementation in this case. We won't know what our first-choice implementation is unless we see some application examples in this area.
Sign in to reply to this message.
On 30 août 2013, at 07:49, "Keith OHara" <k-ohara5a5a@oco.net> wrote: > On Thu, 29 Aug 2013 21:24:25 -0700, <lemzwerg@googlemail.com> wrote: > >> Conceptually, I prefer (1), but this is based on your descriptions and >> the previous discussion, not on understanding the code... >> > > Then look at (2) and see if you think that would be good enough to clear the last bugs before 2.18. > > The uses of \transparent, \pad-to-box, etc., are rare. > It hurts very little to find ourselves stuck with a second-choice implementation in this case. > We won't know what our first-choice implementation is unless we see some application examples in this area. > I'd still argue that (2) is the best way to go as it is a one-stop-shopping way to clear all these bugs (and perhaps more) as they arise. To me, the question is "Is the implementation of (2) inferior to (1) to the point where we'd like to allow certain bugs to persist?" My answer is no - LilyPond has already a practice of creating placeholder stencils whose sole purpose is to reserve space and (2) is in this vein. (2) does for skylines what the empty-stencil does for dimensions. Additionally, (2) clears all the stencil-related skyline bugs on the tracker, whereas (1) does not. So my vote is for (2). Cheers, MS
Sign in to reply to this message.
On 2013/08/30 07:41:49, mike7 wrote: > > I'd still argue that (2) is the best way to go as it is a one-stop-shopping way > to clear all these bugs (and perhaps more) as they arise. Since we are currently in the process of recovering from the last one-stop-shopping way to clear bugs galore which is where all those regressions are actually from, I suggest that we refrain from embracing your somewhat optimistic estimates until after wrapping up a stable release. And frankly, I completely fail to see how this advertisement as a magic bullet for all the regressions is even remotely rooted in fact. The regressions will not in any way be automagically addressed by such sweeping changes. The changes will offer different ways to address them and you like those better. But that's not the same as actually getting them addressed.
Sign in to reply to this message.
On 30 août 2013, at 10:11, dak@gnu.org wrote: > On 2013/08/30 07:41:49, mike7 wrote: > >> I'd still argue that (2) is the best way to go as it is a > one-stop-shopping way >> to clear all these bugs (and perhaps more) as they arise. > > Since we are currently in the process of recovering from the last > one-stop-shopping way to clear bugs galore which is where all those > regressions are actually from, The skyline code was not written to address bugs. LilyPond's previous spacing algorithms weren't buggy - they were just less snug than they are now. > I suggest that we refrain from > embracing your somewhat optimistic estimates until after wrapping up a > stable release. I don't see the relationship between the two. The skyline code was a major, over-a-thousand line overhaul of spacing. This is much smaller in scale and addresses one specific aspect of the code. I'm not claiming that any solution - Keith or mine - is bug proof, but rather that we should adopt the one that will get the most mileage against eventual spacing bugs with stencils. The general category of problem we are trying to solve is "a shape around stencil X is not being represented in the skylines." My solution fixes problems in that general category, whereas Keith's addresses the more specific problem "a box around stencil X is not being represented in the skylines." > > And frankly, I completely fail to see how this advertisement as a > magic bullet for all the regressions is even remotely rooted in fact. > The regressions will not in any way be automagically addressed by such > sweeping changes. The changes will offer different ways to address > them and you like those better. But that's not the same as actually > getting them addressed. We agree here. In my previous e-mail, I make the case that this is a "way to clear all these bugs" and above you correctly identify that it offers "ways to address them." I don't think it's a magic bullet, but rather a general framework that allows us to address this category of bugs without coming up with specific solutions for each bug. Cheers, MS
Sign in to reply to this message.
On 2013/08/30 08:55:15, mike7 wrote: > On 30 août 2013, at 10:11, mailto:dak@gnu.org wrote: > > > On 2013/08/30 07:41:49, mike7 wrote: > > > >> I'd still argue that (2) is the best way to go as it is a > > one-stop-shopping way > >> to clear all these bugs (and perhaps more) as they arise. > > > > Since we are currently in the process of recovering from the last > > one-stop-shopping way to clear bugs galore which is where all those > > regressions are actually from, > > The skyline code was not written to address bugs. Neither is this here. We are talking about changing established behavior to something different. And the motivation for that _is_ the responsibility of the skyline code. But if you want a "one-stop-shopping way to clear bugs galore" from which we are still recovering, take a look at issue 3199. It's responsible for at least issues 3359, 3360, 3385 and was pushed without even waiting for the review to clear. > LilyPond's previous spacing algorithms weren't buggy - they were > just less snug than they are now. "Snug" reminds me of issue 2527, another problem solver responsible for issues 3363, 3465, 3497. > > I suggest that we refrain from embracing your somewhat optimistic > > estimates until after wrapping up a stable release. > > I don't see the relationship between the two. Which makes it a good thing that you are not pursuing a career as project manager. At any rate, we are trying to stabilize our code base in a timely manner suitably for a stable release, and the established track record of ingenious solutions solving all problems in one fell swoop does not suggest that this is the way to go.
Sign in to reply to this message.
On 30 août 2013, at 11:39, dak@gnu.org wrote: > >> > I suggest that we refrain from embracing your somewhat optimistic >> > estimates until after wrapping up a stable release. > >> I don't see the relationship between the two. > > Which makes it a good thing that you are not pursuing a career as > project manager. > > At any rate, we are trying to stabilize our code base in a timely > manner suitably for a stable release, and the established track record > of ingenious solutions solving all problems in one fell swoop does not > suggest that this is the way to go. > The sarcasm of your e-mails has gotten to the point where I need to limit my work on the project as a developer. When I work on a team project, I need to be part of a community with a different style of communication than this. I will still use LilyPond as a tool and and continue to help fix any bugs that have resulted from my work. Cheers, MS
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
