Adds padding between Hairpins and SpanBars.

Adds padding between Hairpins and SpanBars.

[pkx166h, 2011-11-27 20:51:09]

Passes make and make check

James

[Carl, 2011-11-28 01:24:34]

Looks pretty good.  Thanks for hitting this so quickly.  Just a couple of
stylistic comments.

Thanks,

Carl

http://codereview.appspot.com/5438060/diff/1/lily/include/system.hh
File lily/include/system.hh (right):

http://codereview.appspot.com/5438060/diff/1/lily/include/system.hh#newcode45
lily/include/system.hh:45: Grob * get_neighboring_staff (Direction dir, Grob
*vag);
*vag should be *vertical_axis_group for clarity in the header file, IMO.

In the engraver, the code that defines vag shows it to be a vertical_axis_group,
so I'm ok with it there.

http://codereview.appspot.com/5438060/diff/1/lily/system.cc
File lily/system.cc (right):

http://codereview.appspot.com/5438060/diff/1/lily/system.cc#newcode759
lily/system.cc:759: System::get_neighboring_staff (Direction dir, Grob *vag)
Here, *vag should be *vertical_axis_group because there is no context for
understanding what it means.

http://codereview.appspot.com/5438060/diff/1/scm/define-grob-properties.scm
File scm/define-grob-properties.scm (right):

http://codereview.appspot.com/5438060/diff/1/scm/define-grob-properties.scm#n...
scm/define-grob-properties.scm:1047: (has-span-bar ,pair? "A pair of span bars
indicating whether a a span bar
"A pair of grobs containing the span bars to be drawn above and below the staff.
 If no span bar is in a position, the respective element is set to @code{#f}."

Is this a correct statement?  If so, I think it's clearer than the current
wording.

[mike_apollinemike.com, 2011-11-28 07:29:19]

On Nov 28, 2011, at 2:24 AM, Carl.D.Sorensen@gmail.com wrote:

> Looks pretty good.  Thanks for hitting this so quickly.  Just a couple
> of stylistic comments.
> 
> Thanks,
> 
> Carl
> 
> 
> 
> http://codereview.appspot.com/5438060/diff/1/lily/include/system.hh
> File lily/include/system.hh (right):
> 
> http://codereview.appspot.com/5438060/diff/1/lily/include/system.hh#newcode45
> lily/include/system.hh:45: Grob * get_neighboring_staff (Direction dir,
> Grob *vag);
> *vag should be *vertical_axis_group for clarity in the header file, IMO.
> 
> In the engraver, the code that defines vag shows it to be a
> vertical_axis_group, so I'm ok with it there.
> 
> http://codereview.appspot.com/5438060/diff/1/lily/system.cc
> File lily/system.cc (right):
> 
> http://codereview.appspot.com/5438060/diff/1/lily/system.cc#newcode759
> lily/system.cc:759: System::get_neighboring_staff (Direction dir, Grob
> *vag)
> Here, *vag should be *vertical_axis_group because there is no context
> for understanding what it means.
> 
> http://codereview.appspot.com/5438060/diff/1/scm/define-grob-properties.scm
> File scm/define-grob-properties.scm (right):
> 
>
http://codereview.appspot.com/5438060/diff/1/scm/define-grob-properties.scm#n...
> scm/define-grob-properties.scm:1047: (has-span-bar ,pair? "A pair of
> span bars indicating whether a a span bar
> "A pair of grobs containing the span bars to be drawn above and below
> the staff.  If no span bar is in a position, the respective element is
> set to @code{#f}."
> 
> Is this a correct statement?  If so, I think it's clearer than the
> current wording.
> 
> http://codereview.appspot.com/5438060/

I've incorporated all these suggestions into a new patch & posted it.  Thank
you!

Cheers,
MS

[Keith, 2011-11-28 09:05:07]

Hairpins already get padding at the span bars, so rather than add a second
implementation can't we just repair the existing implementation ?

Maybe <http://codereview.appspot.com/5373048/> will do.  (I'm just starting a
make check.)

http://codereview.appspot.com/5438060/diff/7010/lily/system.cc
File lily/system.cc (right):

http://codereview.appspot.com/5438060/diff/7010/lily/system.cc#newcode756
lily/system.cc:756: // Finds the furthest staff in the given direction whose
x-extent
Comment applies to the function above, not this one.

[mike_apollinemike.com, 2011-11-28 09:11:47]

On Nov 28, 2011, at 10:05 AM, k-ohara5a5a@oco.net wrote:

> Hairpins already get padding at the span bars, so rather than add a
> second implementation can't we just repair the existing implementation ?
> 
> Maybe <http://codereview.appspot.com/5373048/> will do.  (I'm just
> starting a make check.)
> 

Ah, I didn't know that this already existed.

I think the regtest I added w/ my patch covers all the bases (see below for the
code & see attached for the result).  If the hairpins stop before span bars but
extend all the way when span bar's don't exist (including when they are not
present because of the RemoveEmptyStaffContext), then I'd much rather go with
your patch, as it is much less invasive than mine.

Lemme know!

Cheers,
MS

[pkx166h, 2011-11-28 16:29:21]

Passes Make and make check

james

[Keith, 2011-11-29 02:13:25]

On 2011/11/28 09:11:47, mike_apollinemike.com wrote:
> If the hairpins stop before span bars but
> extend all the way when span bar's don't exist 
> (including when they are not
> present because of the RemoveEmptyStaffContext), 
> then I'd much rather go with
> your patch, as it is much less invasive than mine.
> 
> Lemme know!
> 
From the response on the tracker issue, it seems that people don't want the
complicated behavior you just described, so we can go with the simpler
<http://codereview.appspot.com/5373048/> ; it made a clean check.  I linked it
to issue 2060 but it fixes 2057 as well.

[mike_apollinemike.com, 2011-11-29 06:58:20]

On Nov 29, 2011, at 3:13 AM, k-ohara5a5a@oco.net wrote:

> On 2011/11/28 09:11:47, mike_apollinemike.com wrote:
>> If the hairpins stop before span bars but
>> extend all the way when span bar's don't exist
>> (including when they are not
>> present because of the RemoveEmptyStaffContext),
>> then I'd much rather go with
>> your patch, as it is much less invasive than mine.
> 
>> Lemme know!
> 
> From the response on the tracker issue, it seems that people don't want
> the complicated behavior you just described, so we can go with the
> simpler <http://codereview.appspot.com/5373048/> ; it made a clean
> check.  I linked it to issue 2060 but it fixes 2057 as well.
> 
> http://codereview.appspot.com/5438060/

Hey all,

I've attached the regtest I added compiled w/ Keith's patch & with my patch.
In the pdf compiled with version of my patch from last night, all of the
hairpins line up and stop short of the span bar. Alternatively, when there are
no span bar intersection problems, the hairpins go to the end of the line. 
Keith - the pdf compiled with your patch seems to still have the intersection
problem.  In general, it is very difficult to anticipate if a hairpin will bump
into a span bar without going through some code acrobatics, and even more
difficult to apply this change to all hairpins ending at the same
NonMusicalPaperColumn without a code 3-ring-circus.  So, even though my code is
sprawley, I think that it is the only way to ensure the correct
collision-avoidance behavior.

Cheers,
MS

[Keith, 2011-11-29 07:25:10]

On Mon, 28 Nov 2011 22:58:07 -0800, mike@apollinemike.com
<mike@apollinemike.com> wrote:

> I've attached the regtest I added compiled w/ Keith's patch & with my patch.

Your test case has one very long hairpin in each staff, spanning three systems.
Do we want to insert a gap in the broken hairpin to make room for the span bar
at the end of the line?  We don't create a gap for span bars in the middle of a
line, so I didn't think that is what the bug report asked for.

I split off issue 2060, which simply makes hairpins at the end-of-line act just
as they do at any other bar line.  I will want to be able to do that regardless
of what you do with your patch.  I also want to fix the original code so that it
works consistently, otherwise your hairpin-shortening code will need to be
synchronized to the bug in the original implementation to avoid
double-shortening.

[mike_apollinemike.com, 2011-11-29 07:38:35]

On Nov 29, 2011, at 8:25 AM, Keith OHara wrote:

> On Mon, 28 Nov 2011 22:58:07 -0800, mike@apollinemike.com
<mike@apollinemike.com> wrote:
> 
>> I've attached the regtest I added compiled w/ Keith's patch & with my patch.
> 
> Your test case has one very long hairpin in each staff, spanning three
systems.
> Do we want to insert a gap in the broken hairpin to make room for the span bar
at the end of the line?  We don't create a gap for span bars in the middle of a
line, so I didn't think that is what the bug report asked for.
> 
> I split off issue 2060, which simply makes hairpins at the end-of-line act
just as they do at any other bar line.  I will want to be able to do that
regardless of what you do with your patch.  I also want to fix the original code
so that it works consistently, otherwise your hairpin-shortening code will need
to be synchronized to the bug in the original implementation to avoid
double-shortening.
> 


Ah, ok, no harm no foul!
In the original report, my eyes zoomed directly to the second system, last
measure (which my patch fixes).  I see what you mean that these two work
independently.

Could you post some PDFs of the patch in action?  Just reading the code, with
respect to:

if (bound->is_non_musical (bound) || bound->break_status_dir())

I have trouble imagining how the second condition could add grobs that the first
doesn't cover (I thought that grobs with a break status dir were a strict subset
of non-musical grobs given how the property defs work in define-grobs.scm).

Cheers,
MS

[Keith, 2011-11-29 08:03:23]

On Mon, 28 Nov 2011 23:38:28 -0800, mike@apollinemike.com
<mike@apollinemike.com> wrote:

> Could you post some PDFs of the patch in action?
There's an example at <http://code.google.com/p/lilypond/issues/detail?id=2060>

> Just reading the code, with respect to:
>   if (bound->is_non_musical (bound) || bound->break_status_dir())
>
> I have trouble imagining how the second condition could add grobs that the
first doesn't cover (I thought that grobs with a break status dir were a strict
subset of non-musical grobs given how the property defs work in
define-grobs.scm).

The function Item:is_non_musical() returns false for copies of the original
Item.  The barline at the end of the line always returns false, and I
tentatively concluded that was because it was a copy when the original barline
was split into three.  I started looking for the reason for the strange behavior
of is_non_musical(), and concluded that the current behavior of is_non_musical()
is too deeply embedded to change.

[pkx166h, 2011-11-29 08:03:44]

passes make and make check

james

[pkx166h, 2011-11-29 21:25:14]

passes make - reg tests attached

http://code.google.com/p/lilypond/issues/detail?id=2057#c18

James

[MikeSol, 2011-11-30 13:00:43]

This most recent patchset has keith's patch applied on top of it in order to
work.

Cheers,
MS

[Carl, 2011-11-30 21:30:16]

Looks good to me, but I want to be sure that both Keith and Mike agree that this
is what ought to be applied, so we don't have two non-compatible solutions
fighting with each other.

http://codereview.appspot.com/5438060/diff/4006/lily/include/system.hh
File lily/include/system.hh (right):

http://codereview.appspot.com/5438060/diff/4006/lily/include/system.hh#newcode45
lily/include/system.hh:45: Grob * get_neighboring_staff (Direction dir, Grob
*vertical_axis_group, Interval_t<int> bounds);
Should there be a space between * and get_neighboring_staff ?

http://codereview.appspot.com/5438060/diff/4006/scm/define-grob-properties.scm
File scm/define-grob-properties.scm (right):

http://codereview.appspot.com/5438060/diff/4006/scm/define-grob-properties.sc...
scm/define-grob-properties.scm:1169: 
Remove extra blank line.

[Keith, 2011-12-01 00:32:44]

On Wed, 30 Nov 2011 13:30:16 -0800, <Carl.D.Sorensen@gmail.com> wrote:

> Looks good to me, but I want to be sure that both Keith and Mike agree
> that this is what ought to be applied, so we don't have two
> non-compatible solutions fighting with each other.

No problem there; Mike incorporated my 2-line patch into this.
I want that 2-line fix to be a separate commit, because it does what the
original bug report wanted, by restoring the intent of the original code.

The full patch no longer does any harm, but I do not like the idea of having
every Hairpin search through every other element on the page to find fellow
Hairpins and ask them about their neighbors.  We hardware folk have spoiled you
software guys with fast computers.

I also want a way to turn off the new feature.

http://codereview.appspot.com/5438060/diff/4006/lily/hairpin.cc
File lily/hairpin.cc (right):

http://codereview.appspot.com/5438060/diff/4006/lily/hairpin.cc#newcode245
lily/hairpin.cc:245: for (vsize i = 0; i < vertical_axis_groups.size (); i++)
For each hairpin, say an orchestral score, this loops over about 10-15 staves

http://codereview.appspot.com/5438060/diff/4006/lily/hairpin.cc#newcode248
lily/hairpin.cc:248: for (vsize j = 0; j < vag_elts.size (); j++)
then for each staff this loops over about 100 elements

http://codereview.appspot.com/5438060/diff/4006/lily/hairpin.cc#newcode252
lily/hairpin.cc:252: for (vsize k = 0; k < elts.size (); k++)
then for each dynamic-line-spanner a handful of dynamics

http://codereview.appspot.com/5438060/diff/4006/lily/hairpin.cc#newcode256
lily/hairpin.cc:256: if (span_elt->get_bound (RIGHT)->spanned_rank_interval
()[LEFT] == bounds[RIGHT]->spanned_rank_interval ()[LEFT])
to find any simultaneously-ending hairpins and ask them if they are next to a
span bar.

http://codereview.appspot.com/5438060/diff/4006/scm/define-grob-properties.scm
File scm/define-grob-properties.scm (right):

http://codereview.appspot.com/5438060/diff/4006/scm/define-grob-properties.sc...
scm/define-grob-properties.scm:913: (uniform-span-bar-padding ,boolean? "Should
the span-bar
no longer used by the code

[pkx166h, 2011-12-01 17:53:44]

Fails make

--snip--

 [/home/jlowe/lilypond-git/ly/context-mods-init.ly]
  [/home/jlowe/lilypond-git/ly/engraver-init.ly]]
 [/home/jlowe/lilypond-git/ly/generate-documentation.ly
[/home/jlowe/lilypond-git/build/out/share/lilypond/current/scm/documentation-lib.scm]
[/home/jlowe/lilypond-git/build/out/share/lilypond/current/scm/lily-sort.scm]
[/home/jlowe/lilypond-git/build/out/share/lilypond/current/scm/document-functions.scm]
[/home/jlowe/lilypond-git/build/out/share/lilypond/current/scm/document-translation.scm]
[/home/jlowe/lilypond-git/build/out/share/lilypond/current/scm/document-music.scm]
[/home/jlowe/lilypond-git/build/out/share/lilypond/current/scm/document-type-predicates.scm]
[/home/jlowe/lilypond-git/build/out/share/lilypond/current/scm/document-identifiers.scm]
[/home/jlowe/lilypond-git/build/out/share/lilypond/current/scm/document-context-mods.scm]
[/home/jlowe/lilypond-git/build/out/share/lilypond/current/scm/document-backend.scm
fatal error: define-grob-properties.scm: cannot find interface for property:
span-bar-padding
make[1]: *** [out/internals.texi] Error 1
make[1]: Leaving directory `/home/jlowe/lilypond-git/build/Documentation'
make: *** [all] Error 2
jlowe@jlowe-lilybuntu2:~/lilypond-git/build$ 


--snip--

James

[pkx166h, 2011-12-02 09:07:02]

Passes make and make check

James

[Keith, 2011-12-03 03:56:56]

http://codereview.appspot.com/5438060/diff/14024/lily/concurrent-hairpin-engr...
File lily/concurrent-hairpin-engraver.cc (right):

http://codereview.appspot.com/5438060/diff/14024/lily/concurrent-hairpin-engr...
lily/concurrent-hairpin-engraver.cc:78: for (vsize i = 0; i <
arriving_hairpins_.size () - 1; i++)
i < arriving_hairpins.size();

http://codereview.appspot.com/5438060/diff/14024/lily/concurrent-hairpin-engr...
lily/concurrent-hairpin-engraver.cc:79: for (vsize j = i + 1; j <
arriving_hairpins_.size (); j++)
for (vsize j = i;

[MikeSol, 2011-12-05 06:53:18]

http://codereview.appspot.com/5438060/diff/14024/lily/concurrent-hairpin-engr...
File lily/concurrent-hairpin-engraver.cc (right):

http://codereview.appspot.com/5438060/diff/14024/lily/concurrent-hairpin-engr...
lily/concurrent-hairpin-engraver.cc:79: for (vsize j = i + 1; j <
arriving_hairpins_.size (); j++)
The issue is that if I did this, an arriving hairpin would be in its own
"concurrent-hairpins" grob array.

[Keith, 2011-12-05 07:58:08]

On 2011/12/05 06:53:18, MikeSol wrote:
> The issue is that if I did this, an arriving hairpin would be in its own
> "concurrent-hairpins" grob array.

I thought that was the idea, because that is the array of all hairpins that are
checked for span-bar closeness.

Somehow the current hairpin has to notice if he is up against a span bar.

http://codereview.appspot.com/5438060/diff/14024/lily/hairpin.cc
File lily/hairpin.cc (right):

http://codereview.appspot.com/5438060/diff/14024/lily/hairpin.cc#newcode246
lily/hairpin.cc:246: Real broken_bound_padding = 0.0;
= me->"broken-bound-padding"

[Keith, 2011-12-08 00:30:20]

Mike,
 The old code was making a distinction between hairpins that end at the end of a
line and those that continue on the next line.
 Sadly, the meaning of the boolean 'broken[]' stores at first whether the bound
on either end of a hair pin is at a line break, and then broken[RIGHT] is
changed so that it stores whether the hairpin itself was broken into pieces on
the right hand side.
 Does your code preserve this distinction?

http://codereview.appspot.com/5438060/diff/2027/lily/hairpin.cc
File lily/hairpin.cc (right):

http://codereview.appspot.com/5438060/diff/2027/lily/hairpin.cc#newcode138
lily/hairpin.cc:138: if (broken[RIGHT])
The former code determining which hairpins were broken on the right, in the
sense of broken in the middle of the hairpin {c2\< c \break c c\f}, is still
around, and it is difficult to tell whether it still does anything.

http://codereview.appspot.com/5438060/diff/2027/scm/define-grob-properties.scm
File scm/define-grob-properties.scm (right):

http://codereview.appspot.com/5438060/diff/2027/scm/define-grob-properties.sc...
scm/define-grob-properties.scm:1049: be drawn above and below the staff.  If no
span bar is in a position,
be drawn below and above the staff, respectively.

[mike_apollinemike.com, 2011-12-08 08:18:05]

Le Dec 8, 2011 à 1:30 AM, k-ohara5a5a@oco.net a écrit :

> Mike,
> The old code was making a distinction between hairpins that end at the
> end of a line and those that continue on the next line.
> Sadly, the meaning of the boolean 'broken[]' stores at first whether
> the bound on either end of a hair pin is at a line break, and then
> broken[RIGHT] is changed so that it stores whether the hairpin itself
> was broken into pieces on the right hand side.
> Does your code preserve this distinction?
> 
> 

Yes.  I care about the latter (whether the hairpin itself is broken) and all my
code kicks in after this distinction is made.

> http://codereview.appspot.com/5438060/diff/2027/lily/hairpin.cc
> File lily/hairpin.cc (right):
> 
> http://codereview.appspot.com/5438060/diff/2027/lily/hairpin.cc#newcode138
> lily/hairpin.cc:138: if (broken[RIGHT])
> The former code determining which hairpins were broken on the right, in
> the sense of broken in the middle of the hairpin {c2\< c \break c c\f},
> is still around, and it is difficult to tell whether it still does
> anything.
> 

I'm not sure...in a separate patch, we can try removing it and see if that
results in anything.

> http://codereview.appspot.com/5438060/diff/2027/scm/define-grob-properties.scm
> File scm/define-grob-properties.scm (right):
> 
>
http://codereview.appspot.com/5438060/diff/2027/scm/define-grob-properties.sc...
> scm/define-grob-properties.scm:1049: be drawn above and below the staff.
> If no span bar is in a position,
> be drawn below and above the staff, respectively.

Sorry, I don't understand this comment.  Could you please rephrase it?

Cheers,
MS

[Keith, 2011-12-08 08:38:13]

On Thu, 08 Dec 2011 00:17:56 -0800, mike@apollinemike.com
<mike@apollinemike.com> wrote:

>>
http://codereview.appspot.com/5438060/diff/2027/scm/define-grob-properties.sc...
>> scm/define-grob-properties.scm:1049: be drawn above and below the staff.
>> If no span bar is in a position,
>> be drawn below and above the staff, respectively.
>
> Sorry, I don't understand this comment.  Could you please rephrase it?
>

I suggest swapping "above"-"below" because I think you switched the order of
contents in has-span-bar.

[mike_apollinemike.com, 2011-12-08 08:47:55]

Le Dec 8, 2011 à 9:37 AM, Keith OHara a écrit :

> On Thu, 08 Dec 2011 00:17:56 -0800, mike@apollinemike.com
<mike@apollinemike.com> wrote:
> 
>>>
http://codereview.appspot.com/5438060/diff/2027/scm/define-grob-properties.sc...
>>> scm/define-grob-properties.scm:1049: be drawn above and below the staff.
>>> If no span bar is in a position,
>>> be drawn below and above the staff, respectively.
>> 
>> Sorry, I don't understand this comment.  Could you please rephrase it?
>> 
> 
> I suggest swapping "above"-"below" because I think you switched the order of
contents in has-span-bar.
> 

Ah, gotchya.  Done.

Cheers,
MS

[thomasmorley65, 2011-12-08 23:51:22]

Hi Mike,

please excuse me disturbing the discussion. I do not understand it, so
I simply ask:

Will these patch fix the problem drawing the hairpin under a new
KeySignature at the line-end as shown with this example?

{
  c'1\< \key bes\major \break
  d'2 c'\!
}

(Perhaps you remember helping me to create a work-around for this:
http://lists.gnu.org/archive/html/lilypond-user/2011-08/msg00583.html
)

If not, would it be hard to do so?

Cheers,
  Harm