Avoid excessive number of dots in chords (#3179).

Avoid excessive number of dots in chords (#3179).

[lemzwerg, 2013-02-16 20:52:49]

Add missing conditional test.

[Keith, 2013-02-17 08:20:39]

This seems like the most reasonable place to remove extra dots. 
Dot_configuration has the job of arranging dots in a chord, but confines its
efforts to the mathematical problem of keeping a set of integer position for
dots close to a set of integer positions of noteheads.  Looking at the resulting
configuration and removing grobs for aesthetic reasons is better done at this
level.

You should count the number of dots you have so you do not remove the last one. 
Someone somewhere has a staff with three lines at positions 1 2 3, so even a
single dot has to move two positions away from the head. You might want to start
removing only after there are >3 dots, or else people will complain that their
dots disappeared in chords like <d' e' f'>4.  If you can't avoid complaints,
you'll have to make this behavior controllable with a grob property.

https://codereview.appspot.com/7319049/diff/2001/lily/dot-column.cc
File lily/dot-column.cc (right):

https://codereview.appspot.com/7319049/diff/2001/lily/dot-column.cc#newcode63
lily/dot-column.cc:63: vector<Interval> allowed_y_positions;
It would be simpler to have just one interval, describing all the dots handled
by one dot_engraver, even if coming from two voices.

https://codereview.appspot.com/7319049/diff/2001/lily/dot-column.cc#newcode80
lily/dot-column.cc:80: Interval hp = Stem::head_positions (stem);
It looks like head_positions are shifted in the test 'grid-lines.ly' but dot
positions are not, at this stage in the code.

https://codereview.appspot.com/7319049/diff/2001/lily/dot-column.cc#newcode195
lily/dot-column.cc:195: 
You could build the allowed interval here,
  allowed_positions.add_point(p);
so that the interval uses the same point-of-origin as does the
Dots_configuration code.

[lemzwerg, 2013-02-17 09:01:47]

Make it work for unusual Y-offset values.

[lemzwerg, 2013-02-17 10:33:08]

Thanks for the review.

https://codereview.appspot.com/7319049/diff/2001/lily/dot-column.cc
File lily/dot-column.cc (right):

https://codereview.appspot.com/7319049/diff/2001/lily/dot-column.cc#newcode63
lily/dot-column.cc:63: vector<Interval> allowed_y_positions;
On 2013/02/17 08:20:39, Keith wrote:
> It would be simpler to have just one interval, describing all the dots handled
> by one dot_engraver, even if coming from two voices.

I've started with a single interval but this doesn't work well.  Just imagine

  << { <e f g a b>2. } \\
     { <c, d e f g>2. } >>

There is a gap between the two clusters, and it looks really strange if this is
filled with dots.

https://codereview.appspot.com/7319049/diff/2001/lily/dot-column.cc#newcode80
lily/dot-column.cc:80: Interval hp = Stem::head_positions (stem);
On 2013/02/17 08:20:39, Keith wrote:
> It looks like head_positions are shifted in the test 'grid-lines.ly' but dot
> positions are not, at this stage in the code.

This is addressed in the newer patchset.

[Keith, 2013-02-18 07:37:48]

https://codereview.appspot.com/7319049/diff/7001/lily/dot-column.cc
File lily/dot-column.cc (right):

https://codereview.appspot.com/7319049/diff/7001/lily/dot-column.cc#newcode82
lily/dot-column.cc:82: // lines.  Be careful to make it work also for unusual
Staff_symbol_referencer::on_line(&any_grob_on_this_staff, position) as used on
line 211, tells whether a position is on a line, for the currently-defined
staff.

[Keith, 2013-02-18 08:06:44]

https://codereview.appspot.com/7319049/diff/7001/lily/dot-column.cc
File lily/dot-column.cc (right):

https://codereview.appspot.com/7319049/diff/7001/lily/dot-column.cc#newcode206
lily/dot-column.cc:206: p += (int) robust_scm2double (dp.dot_->get_property
("staff-position"), 0.0);
p is the position of the Dots, just before-collision resolution by this
function.  This is what I was calling the "desired position" with any setting of
Dots.staff-position being included here.

That is why I suggested building allowed_positions using p:
  allowed_positions.add_point(p)

You can extend this to have a separate allowed_positions for each chord, finding
the Stem of the Dots as you did above.

https://codereview.appspot.com/7319049/diff/7001/lily/dot-column.cc#newcode210
lily/dot-column.cc:210: cfg[p] = dp;
This enters a data structure dp representing a movable Dots into the
Dot_configuration cfg and associates it with the desired position 'p'.  (cfg is
implemented as a map, cfg[p] = dp2 would enter a different Dots at the same
desired position.)

[lemzwerg, 2013-02-18 16:16:33]

... next patch set will follow soon.

https://codereview.appspot.com/7319049/diff/7001/lily/dot-column.cc
File lily/dot-column.cc (right):

https://codereview.appspot.com/7319049/diff/7001/lily/dot-column.cc#newcode206
lily/dot-column.cc:206: p += (int) robust_scm2double (dp.dot_->get_property
("staff-position"), 0.0);
On 2013/02/18 08:06:45, Keith wrote:
> That is why I suggested building allowed_positions using p:
>   allowed_positions.add_point(p)
> 
> You can extend this to have a separate allowed_positions for each
> chord, finding the Stem of the Dots as you did above.

But the `desired position' is not what I need!  Following Gould, I really need
the extrema of the chord's note head positions, slightly extended at the top and
the bottom in case those notes are sitting on a line.

[lemzwerg, 2013-02-19 08:45:31]

Should pass all regression tests now.

[Keith, 2013-02-19 10:35:25]

LGTM.

We do lose the double-dot on  << f'4.. \\ f'2. >>
but can get it back with
 \once\override Staff.DotColumn #'chord-dots = ##f 
(although this override looks like I am asking to remove dots on chords, it
actually gives me Dots for each dotted note-head).

On 2013/02/18 16:16:33, lemzwerg wrote:
> https://codereview.appspot.com/7319049/diff/7001/lily/dot-column.cc#newcode206
> lily/dot-column.cc:206: p += (int) robust_scm2double (dp.dot_->get_property
> ("staff-position"), 0.0);
> 
> But the `desired position' is not what I need!  Following Gould, I really need
> the extrema of the chord's note head positions, slightly extended at the top
and
> the bottom in case those notes are sitting on a line.

Well, Gould is showing the usual case, where notes have Dots.staff-position = 0.
 You were asking for opinions on the optimal way to behave for users who
override Dots.staff-position.  You could generalize by using the requested Dots
position 'p' (which is exactly the NoteHead position in the simple case) in
place of the note-head position.

[lemzwerg, 2013-02-19 11:01:16]

> We do lose the double-dot on  << f'4.. \\ f'2. >>

Yes.  As shown in the scanned Beethoven example in #3179, what lilypond
currently does with `chord-dots' off is not correct either...

> Well, Gould is showing the usual case, where notes have
> Dots.staff-position = 0.  You were asking for opinions
> on the optimal way to behave for users who override
> Dots.staff-position.  You could generalize by using the
> requested Dots position 'p' (which is exactly the NoteHead
> position in the simple case) in place of the note-head
> position.

Thanks for the suggestion.  In case there is ever the need to improve my
solution, you give a recipe to do that :-)

[dak, 2013-02-19 11:23:40]

On 2013/02/19 11:01:16, lemzwerg wrote:

> > Well, Gould is showing the usual case, where notes have
> > Dots.staff-position = 0.  You were asking for opinions
> > on the optimal way to behave for users who override
> > Dots.staff-position.  You could generalize by using the
> > requested Dots position 'p' (which is exactly the NoteHead
> > position in the simple case) in place of the note-head
> > position.
> 
> Thanks for the suggestion.  In case there is ever the need to improve my
> solution, you give a recipe to do that :-)

How about putting it in the code right now so that we don't have to hunt for it
in old code reviews at some future point of time?