Issue 4898060: Makes sure that ledger lines do not overlap with accidentals.

Rietveld

test missing.

On Thu, Aug 18, 2011 at 8:03 AM,  <mtsolo@gmail.com> wrote:
> Reviewers: ,
>
> Description:
> Makes sure that ledger lines do not overlap with accidentals.
>
> Please review this at http://codereview.appspot.com/4898060/
>
> Affected files:
>  M lily/ledger-line-spanner.cc
>
>
> Index: lily/ledger-line-spanner.cc
> diff --git a/lily/ledger-line-spanner.cc b/lily/ledger-line-spanner.cc
> index
>
01238222147c9afbf4f33a2e6b62ac2018a46c9f..9487034eabdca68f4cf93df9251e596013b4203c
> 100644
> --- a/lily/ledger-line-spanner.cc
> +++ b/lily/ledger-line-spanner.cc
> @@ -18,6 +18,7 @@
>  */
>
>  #include <map>
> +#include <set>
>  using namespace std;
>
>  #include "note-head.hh"
> @@ -34,6 +35,7 @@ struct Ledger_line_spanner
>   DECLARE_SCHEME_CALLBACK (set_spacing_rods, (SCM));
>   static Stencil brew_ledger_lines (Grob *me,
>                                     int pos,
> +                                    set<int> &poss,
>                                     Interval,
>                                     Real, Real,
>                                     Interval x_extent,
> @@ -45,6 +47,7 @@ struct Ledger_line_spanner
>  Stencil
>  Ledger_line_spanner::brew_ledger_lines (Grob *staff,
>                                         int pos,
> +                                        set<int> &poss,
>                                         Interval staff_extent,
>                                         Real halfspace,
>                                         Real ledgerlinethickness,
> @@ -74,8 +77,16 @@ Ledger_line_spanner::brew_ledger_lines (Grob *staff,
>                   : -dir * halfspace;
>
>       offs += pos * halfspace;
> +      set<int>::iterator it;
> +      set<int> local_poss;
> +
> +      for (it = poss.begin (); it != poss.end (); it++)
> +        local_poss.insert (max (0, line_count - *it));
> +
>       for (int i = 0; i < line_count; i++)
>         {
> +          if (local_poss.find (i) != local_poss.end () && i)
> +            continue;
>           Stencil ledger_line ((i == 0)
>                                ? proto_first_line
>                                : proto_ledger_line);
> @@ -315,6 +326,17 @@ Ledger_line_spanner::print (SCM smob)
>   // create ledgers for note heads
>   Real ledgerlinethickness
>     = Staff_symbol::get_ledger_line_thickness (staff);
> +
> +  Drul_array<set<int > > poss;
> +  for (vsize i = heads.size (); i--;)
> +    {
> +      Item *h = dynamic_cast<Item *> (heads[i]);
> +
> +      int pos = Staff_symbol_referencer::get_rounded_position (h);
> +      if (!staff_extent.contains (pos - sign (pos)) &&
> !staff_extent.is_empty ())
> +        poss[(Direction) sign (pos)].insert (sign (pos) * int (rint (pos -
> staff_extent[Direction (sign (pos))])) / 2);
> +    }
> +
>   for (vsize i = heads.size (); i--;)
>     {
>       Item *h = dynamic_cast<Item *> (heads[i]);
> @@ -347,7 +369,9 @@ Ledger_line_spanner::print (SCM smob)
>               */
>             }
>
> -          ledgers.add_stencil (brew_ledger_lines (staff, pos, staff_extent,
> +          ledgers.add_stencil (brew_ledger_lines (staff, pos,
> +                                                  poss[(Direction) sign
> (pos)],
> +                                                  staff_extent,
>                                                   halfspace,
>                                                   ledgerlinethickness,
>                                                   ledger_size,
>
>
>
> _______________________________________________
> lilypond-devel mailing list
> lilypond-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/lilypond-devel
>

-- 
Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen

http://codereview.appspot.com/4898060/diff/1/lily/ledger-line-spanner.cc
File lily/ledger-line-spanner.cc (right):

http://codereview.appspot.com/4898060/diff/1/lily/ledger-line-spanner.cc#newc...
lily/ledger-line-spanner.cc:89: continue;
can't you check poss.find(max(0, lincount - *it)) ?

I don't understand what this is supposed to do ; in which cases do you want to
skip ledger creation?

http://codereview.appspot.com/4898060/diff/1/lily/ledger-line-spanner.cc#newc...
lily/ledger-line-spanner.cc:337: poss[(Direction) sign (pos)].insert (sign (pos)
* int (rint (pos - staff_extent[Direction (sign (pos))])) / 2);
have you tried if you can change sign() to return a Direction ?
I figured not making a regtest just cuz there will already be little changes to
several regtests.  But, I can certainly add one.

Cheers,
MS

http://codereview.appspot.com/4898060/diff/1/lily/ledger-line-spanner.cc
File lily/ledger-line-spanner.cc (right):

http://codereview.appspot.com/4898060/diff/1/lily/ledger-line-spanner.cc#newc...
lily/ledger-line-spanner.cc:89: continue;
On 2011/08/18 12:16:50, hanwenn wrote:
> can't you check poss.find(max(0, lincount - *it)) ?
> 
> I don't understand what this is supposed to do ; in which cases do you want to
> skip ledger creation?

Good call - I can do this.

It skips over all ledger lines that already have a note on them, thus avoiding
duplicates.

http://codereview.appspot.com/4898060/diff/1/lily/ledger-line-spanner.cc#newc...
lily/ledger-line-spanner.cc:337: poss[(Direction) sign (pos)].insert (sign (pos)
* int (rint (pos - staff_extent[Direction (sign (pos))])) / 2);
On 2011/08/18 12:16:50, hanwenn wrote:
> have you tried if you can change sign() to return a Direction ?

No - I'll have a look.
On Thu, Aug 18, 2011 at 9:27 AM,  <mtsolo@gmail.com> wrote:
> I figured not making a regtest just cuz there will already be little
> changes to several regtests.  But, I can certainly add one.
>
> Cheers,
> MS
>
>
> http://codereview.appspot.com/4898060/diff/1/lily/ledger-line-spanner.cc
> File lily/ledger-line-spanner.cc (right):
>
>
http://codereview.appspot.com/4898060/diff/1/lily/ledger-line-spanner.cc#newc...
> lily/ledger-line-spanner.cc:89: continue;
> On 2011/08/18 12:16:50, hanwenn wrote:
>>
>> can't you check poss.find(max(0, lincount - *it)) ?
>
>> I don't understand what this is supposed to do ; in which cases do you
>
> want to
>>
>> skip ledger creation?
>
> Good call - I can do this.
>
> It skips over all ledger lines that already have a note on them, thus
> avoiding duplicates.

are you sure this works with chords that have seconds in them? It
sounds like an optimization that doesn't really improve output at all,
and performance not by much. Why not skip it altogether and avoid the
complexity?

-- 
Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen

On Aug 18, 2011, at 2:51 PM, Han-Wen Nienhuys wrote:

> On Thu, Aug 18, 2011 at 9:27 AM,  <mtsolo@gmail.com> wrote:
>> I figured not making a regtest just cuz there will already be little
>> changes to several regtests.  But, I can certainly add one.
>> 
>> Cheers,
>> MS
>> 
>> 
>> http://codereview.appspot.com/4898060/diff/1/lily/ledger-line-spanner.cc
>> File lily/ledger-line-spanner.cc (right):
>> 
>>
http://codereview.appspot.com/4898060/diff/1/lily/ledger-line-spanner.cc#newc...
>> lily/ledger-line-spanner.cc:89: continue;
>> On 2011/08/18 12:16:50, hanwenn wrote:
>>> 
>>> can't you check poss.find(max(0, lincount - *it)) ?
>> 
>>> I don't understand what this is supposed to do ; in which cases do you
>> 
>> want to
>>> 
>>> skip ledger creation?
>> 
>> Good call - I can do this.
>> 
>> It skips over all ledger lines that already have a note on them, thus
>> avoiding duplicates.
> 
> are you sure this works with chords that have seconds in them? It
> sounds like an optimization that doesn't really improve output at all,

I agree that the improvement is kinda meh - I was more optimistic about it when
writing it then when seeing the actual results, as there are collisions with
lines above and below depending on the accidental.

> and performance not by much. Why not skip it altogether and avoid the
> complexity?
> 

I am more or less indifferent to how it gets done, but I do think these
collisions should be avoided.

One easy fix is simply making the default whiteout property of accidentals to
true somewhere in ly/ and then pushing their layer way down low and push ledger
lines one notch lower.

{
  \override Staff.Accidental #'layer = #-100
 \override Staff.LedgerLineSpanner #'layer = #-101
\override Staff.Accidental #'whiteout = ##t
<des ces'>

}

Thoughts?

Cheers,
MS
Hi,

i'm very interested in this, i've spotted this problem some time ago.
Why is this Rietveld issue marked as closed?
I'm adding a link in T issue 706
http://code.google.com/p/lilypond/issues/detail?id=706

I've tried it and it ate some ledger lines, but maybe i have dirty
tree.  Try this
{ <c'! f!>2 <bis, cis'> <bes, ces'> < ces' des > }
On my machine it eats ledgers, see attachment.

On 2011/08/18 12:51:09, hanwenn wrote:
> It sounds like an optimization that doesn't really improve
> output at all, and performance not by much. Why not skip it
> altogether and avoid the complexity?

I wholeheartedly disagree, i think that this issue isn't negligible
(or did i misunderstood you, Han-Wen?).
If we care about shortening ledgers when there is an accidental, we
should care about doing it properly in case of chords etc.

cheers,
Janek

2011/8/22 Janek Warchoł <janek.lilypond@gmail.com>:
> I wholeheartedly disagree, i think that this issue isn't negligible
> (or did i misunderstood you, Han-Wen?).

My impression is that the patch was not actually changing output.

If it was intended to, I couldn't tell from a read of the patch.

-- 
Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen

Hey all,

My patch did change output (it prevented duplicate ledger lines), although I did
not spot the result that Janek sees.

The old patch is also flawed, in that it only changes the ledger line directly
next to the accidental.  The entire bounding box of the accidental should,
however, be taken into account.
I maintain that the best solution is:

{
 \override Staff.Accidental #'layer = #-100
\override Staff.LedgerLineSpanner #'layer = #-101
\override Staff.Accidental #'whiteout = ##t
<des ces'>

}

Which is a trivial change to engravers-init.ly.  I'll post a patch later today
unless anyone sees a better alternative.

Cheers,
MS

On Aug 22, 2011, at 11:33 PM, Han-Wen Nienhuys wrote:

> 2011/8/22 Janek Warchoł <janek.lilypond@gmail.com>:
>> I wholeheartedly disagree, i think that this issue isn't negligible
>> (or did i misunderstood you, Han-Wen?).
> 
> My impression is that the patch was not actually changing output.
> 
> If it was intended to, I couldn't tell from a read of the patch.
> 
> -- 
> Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen
> 
> _______________________________________________
> lilypond-devel mailing list
> lilypond-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/lilypond-devel

On Tue, Aug 23, 2011 at 3:59 AM, Mike Solomon <mikesol@ufl.edu> wrote:
>  \override Staff.Accidental #'layer = #-100
> \override Staff.LedgerLineSpanner #'layer = #-101
> \override Staff.Accidental #'whiteout = ##t
> <des ces'>
>
> }
>
> Which is a trivial change to engravers-init.ly.  I'll post a patch later today
unless anyone sees a better alternative.

this will generate square endings on the ledgers.

-- 
Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen

On Aug 23, 2011, at 4:03 PM, Han-Wen Nienhuys wrote:

> On Tue, Aug 23, 2011 at 3:59 AM, Mike Solomon <mikesol@ufl.edu> wrote:
>>  \override Staff.Accidental #'layer = #-100
>> \override Staff.LedgerLineSpanner #'layer = #-101
>> \override Staff.Accidental #'whiteout = ##t
>> <des ces'>
>> 
>> }
>> 
>> Which is a trivial change to engravers-init.ly.  I'll post a patch later
today unless anyone sees a better alternative.
> 
> this will generate square endings on the ledgers.

True.
Question - is there any way to take a glyph from the feta font and make it white
thru pango?  If so, you could increase the size of the white glyph by a factor,
set the accidental against it, and then the ledger line changes would follow the
contour of the accidental.

Cheers,
MS
On Tue, Aug 23, 2011 at 11:05 AM, Mike Solomon <mikesol@ufl.edu> wrote:
>>>
>>> Which is a trivial change to engravers-init.ly.  I'll post a patch later
today unless anyone sees a better alternative.
>>
>> this will generate square endings on the ledgers.
>
> True.
> Question - is there any way to take a glyph from the feta font and make it
white thru pango?  If so, you could increase the size of the white glyph by a
factor, set the accidental against it, and then the ledger line changes would
follow the contour of the accidental.

I think the whole approach of whiting is flawed: what if the
accidental covers a ledger only halfway? The ledgers would look like

a ***
a ***
*****
*****

(a  = acc, * = ledger)

-- 
Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen

Han-Wen Nienhuys writes:

> I think the whole approach of whiting is flawed: what if

To pull that to a meta-level: sometimes it makes sense to program
LilyPond to do that which is obvious.

However, almost always it is Much Better (TM) to use the benchmarking
approach: find how the great masters solved the situation and program
LilyPond to do that.  Is this documented somewhere, by the way?

Jan

-- 
Jan Nieuwenhuizen <janneke@gnu.org> | GNU LilyPond http://lilypond.org
Freelance IT http://JoyofSource.com | Avatar®  http://AvatarAcademy.nl

2011/8/22 Han-Wen Nienhuys <hanwenn@gmail.com>:
> 2011/8/22 Janek Warchoł <janek.lilypond@gmail.com>:
>> I wholeheartedly disagree, i think that this issue isn't negligible
>> (or did i misunderstood you, Han-Wen?).
>
> My impression is that the patch was not actually changing output.
> If it was intended to, I couldn't tell from a read of the patch.

Ah, ok.

2011/8/23 Mike Solomon <mikesol@ufl.edu>:
> The old patch is also flawed, in that it only changes the ledger line
> directly next to the accidental.  The entire bounding box of the accidental
> should, however, be taken into account.

Exactly.

> I maintain that the best solution is:
>
> {
>  \override Staff.Accidental #'layer = #-100
> \override Staff.LedgerLineSpanner #'layer = #-101
> \override Staff.Accidental #'whiteout = ##t
> <des ces'>
>
> }
>
> Which is a trivial change to engravers-init.ly.
> I'll post a patch later today unless anyone sees a better alternative.

Hmm... maybe we shouldn't create ledger lines for noteheads, but for
noteColumns?  This way there would be no duplicate ledgers.  We could
also check "from the level of noteColumns", i.e. cycle through all
notes and see if they have accidentals, then engrave ledgers according
to that.
(maybe i'm speaking nonsense, i have limited knowledge of internals
and moreover my remote destop died so i cannot check it directly)

Another thought: when there are many accidentals in a noteColumn, they
are moved around so they don't collide.  So there is something in the
code that knows about positions of all accidentals in a notecoumn,
isn't it?  It should be easy then to apply this knowledge to
noteColumn's ledgers - if there are no duplicates.

What do you think?

cheers,
Janek

2011/8/23 Jan Nieuwenhuizen <janneke@gnu.org>:
> To pull that to a meta-level: sometimes it makes sense to program
> LilyPond to do that which is obvious.
>
> However, almost always it is Much Better (TM) to use the benchmarking
> approach: find how the great masters solved the situation and program
> LilyPond to do that.  Is this documented somewhere, by the way?

Sorry, i don't understand: do you say that we should use layer
workaround, don't use it, or something else?

2011/8/23 Keith OHara <k-ohara5a5a@oco.net>:
> Mike Solomon <mikesol <at> ufl.edu> writes:
>
>> I maintain that the best solution is:
>>  \override Staff.Accidental #'layer = #-100
>> \override Staff.LedgerLineSpanner #'layer = #-101
>> \override Staff.Accidental #'whiteout = ##t
>>
>> Which is a trivial change to engravers-init.ly.  I'll post a patch later
today
> unless anyone sees a better alternative.
>
> I wouldn't bother.  It would not really fix issue 706, and makes an
> exception to the general rule that everything is layer 1, except
> the grid of lines is on layer 0.
> You could post it as a workaround to issue 706 maybe.

+1

cheers,
Janek

Janek Warchoł writes:

>> However, almost always it is Much Better (TM) to use the benchmarking
>> approach: find how the great masters solved the situation and program
>> LilyPond to do that.  Is this documented somewhere, by the way?
>
> Sorry, i don't understand: do you say that we should use layer
> workaround, don't use it, or something else?

No, I'm not arguing for a certain technical solution, I'm saying that
instead of deciding amongst ourselves what solution to implement, it
would be better to let the great masters decide.  "Just" mimick what
they did.

Jan

-- 
Jan Nieuwenhuizen <janneke@gnu.org> | GNU LilyPond http://lilypond.org
Freelance IT http://JoyofSource.com | Avatar®  http://AvatarAcademy.nl  

On 8/27/11 6:45 AM, "Jan Nieuwenhuizen" <janneke@gnu.org> wrote:

> Janek Warchoł writes:
> 
>>> However, almost always it is Much Better (TM) to use the benchmarking
>>> approach: find how the great masters solved the situation and program
>>> LilyPond to do that.  Is this documented somewhere, by the way?
>> 
>> Sorry, i don't understand: do you say that we should use layer
>> workaround, don't use it, or something else?
> 
> No, I'm not arguing for a certain technical solution, I'm saying that
> instead of deciding amongst ourselves what solution to implement, it
> would be better to let the great masters decide.  "Just" mimick what
> they did.

But in this case, this is arguing against the layer workaround as a solution
to this problem.  The great masters did not engrave a ledger line, then
engrave an accidental with whiteout.  They engraved a shorter ledger line.
This shorter ledger line had rounded ends.  The whiteout solution gives a
short ledger line, but with square ends.

Thanks,

Carl

Issue 4898060: Makes sure that ledger lines do not overlap with accidentals. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Adds regtest. #

Messages