possible tuplet bug?

2013-12-25 Thread David Nalesnik 
Hi,

I just noticed something fishy about the interaction of
Tuplet.connect-to-neighbor and Tuplet.break-overshoot.  The property
'break-overshoot is meant to apply to broken spanners, but here it is
having an effect on unbroken "connected" tuplet brackets. (Break-overshoot
is described as "How much does a broken spanner stick out of its bounds?")

The following snippet demonstrates the problem (see attached image):

%%

\version "2.17.97"


 \layout {

  ragged-right = ##t

}


 \relative c' {

  \times 2/3 { c4 d e }

  \times 2/3 { d4 e c }

  \times 2/3 { c'4 d e }

  \times 2/3 { d4 e c }

  \bar "||"

  %This "fixes" the problem:

  %\override TupletBracket.break-overshoot = #'(0 . 0)

  \override TupletBracket.connect-to-neighbor = #'(#t . #t)

  \times 2/3 { c,4 d e }

  \times 2/3 { d4 e c }

  \times 2/3 { c'4 d e }

  \times 2/3 { d4 e c }

}





A default value of '(-0.5 . 0.0) is being applied to the tuplet brackets
affected by the override of 'break-overshoot.  Thus, the left side is
shifted to the right.  (The tuplet number is also moved from its position
in the ordinary tuplets.)


In the "patch" below, I've suggested a modification of the relevant
function in tuplet-bracket.cc which appears to fix the problem.  (The
replacement is above the commented-out section of the original.) You can
see the results in the second image.


I hesitate to submit a formal patch because I haven't worked with C++ in
LilyPond before, and I'd like to get some feedback from someone who
understands this aspect of the codebase to make sure I'm not missing
something big!  (It does compile--the image uses the patch.)


Thanks,

David




from tuplet-bracket.cc:

MAKE_SCHEME_CALLBACK (Tuplet_bracket, calc_x_positions, 1)
SCM
Tuplet_bracket::calc_x_positions (SCM smob)
{
  Spanner *me = unsmob_spanner (smob);
  extract_grob_set (me, "note-columns", columns);

  Grob *commonx = get_common_x (me);
  Direction dir = get_grob_direction (me);

  Drul_array bounds;
  bounds[LEFT] = get_x_bound_item (me, LEFT, dir);
  bounds[RIGHT] = get_x_bound_item (me, RIGHT, dir);

  Drul_array connect_to_other
= robust_scm2booldrul (me->get_property ("connect-to-neighbor"),
   Drul_array (false, false));

  Interval x_span;
  for (LEFT_and_RIGHT (d))
{
  x_span[d] = Axis_group_interface::generic_bound_extent (bounds[d],
commonx, X_AXIS)[d];

  if (connect_to_other[d] && bounds[d]->break_status_dir())
  {
  Interval overshoot (robust_scm2drul (me->get_property
("break-overshoot"),
   Interval (-0.5, 0.0)));
  if (d == RIGHT)
x_span[d] += d * overshoot[d];
  else
x_span[d] = Axis_group_interface::generic_bound_extent
(bounds[d], commonx, X_AXIS)[-d]
- overshoot[LEFT];
  }


/*
  if (connect_to_other[d])
{
  Interval overshoot (robust_scm2drul (me->get_property
("break-overshoot"),
   Interval (-0.5, 0.0)));

  if (d == RIGHT)
x_span[d] += d * overshoot[d];
  else
x_span[d] = (bounds[d]->break_status_dir ()
 ? Axis_group_interface::generic_bound_extent
(bounds[d], commonx, X_AXIS)[-d]
 : robust_relative_extent (bounds[d], commonx,
X_AXIS)[-d])
- overshoot[LEFT];
}
*/


  else if (d == RIGHT
   && (columns.empty ()
   || (bounds[d]->get_column ()
   != dynamic_cast (columns.back
())->get_column (
{
  /*
We're connecting to a column, for the last bit of a broken
fullLength bracket.
  */
  Real padding
= robust_scm2double (me->get_property ("full-length-padding"),
1.0);

  if (bounds[d]->break_status_dir ())
padding = 0.0;

  Real coord = bounds[d]->relative_coordinate (commonx, X_AXIS);
  if (to_boolean (me->get_property ("full-length-to-extent")))
coord = robust_relative_extent (bounds[d], commonx,
X_AXIS)[LEFT];

  coord = max (coord, x_span[LEFT]);

  x_span[d] = coord - padding;
}
}

  return ly_interval2scm (x_span - me->get_bound
(LEFT)->relative_coordinate (commonx, X_AXIS));
}
<>___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: possible tuplet bug?

2013-12-25 Thread David Nalesnik 
Hi again,


On Wed, Dec 25, 2013 at 12:41 PM, David Nalesnik
wrote:
>
>
> The following snippet demonstrates the problem (see attached image):
>
>
And here's the image of the default...
<>___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: possible tuplet bug?

2013-12-25 Thread David Kastrup 
David Nalesnik  writes:

> In the "patch" below, I've suggested a modification of the relevant
> function in tuplet-bracket.cc which appears to fix the problem.  (The
> replacement is above the commented-out section of the original.) You can
> see the results in the second image.
>
>
> I hesitate to submit a formal patch because I haven't worked with C++ in
> LilyPond before,

Formal or not, trying to figure out what you changed from just a quote
of code is a major nuisance and unreliable.

So even if you don't want to propose a "formal" commit, using "git diff"
in order to produce a properly readable patch that you can attach to a
mail will make people's life easier.

-- 
David Kastrup

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: possible tuplet bug?

2013-12-25 Thread David Nalesnik 
David,

On Wed, Dec 25, 2013 at 12:56 PM, David Kastrup  wrote:

> David Nalesnik  writes:
>
> >
> > I hesitate to submit a formal patch because I haven't worked with C++ in
> > LilyPond before,
>
> Formal or not, trying to figure out what you changed from just a quote
> of code is a major nuisance and unreliable.
>
> So even if you don't want to propose a "formal" commit, using "git diff"
> in order to produce a properly readable patch that you can attach to a
> mail will make people's life easier.
>
>
Ah, OK, will do.

--David
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: possible tuplet bug?

2013-12-25 Thread David Nalesnik 
On Wed, Dec 25, 2013 at 12:58 PM, David Nalesnik
wrote:

> David,
>
> On Wed, Dec 25, 2013 at 12:56 PM, David Kastrup  wrote:
>
> So even if you don't want to propose a "formal" commit, using "git diff"
>> in order to produce a properly readable patch that you can attach to a
>> mail will make people's life easier.
>>
>>
> Ah, OK, will do.
>

See attached.  I apologize for any inconvenience.

-David
From 059ff4586a1e6a1fda27f40372a87db972be378c Mon Sep 17 00:00:00 2001
From: David Nalesnik 
Date: Wed, 25 Dec 2013 09:41:39 -0600
Subject: [PATCH] fix problem with connect-to-neighbor and break-overshoot in unbroken tuplets

---
 lily/tuplet-bracket.cc |9 +++--
 1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/lily/tuplet-bracket.cc b/lily/tuplet-bracket.cc
index 1eccaab..7171e2a 100644
--- a/lily/tuplet-bracket.cc
+++ b/lily/tuplet-bracket.cc
@@ -202,18 +202,15 @@ Tuplet_bracket::calc_x_positions (SCM smob)
   for (LEFT_and_RIGHT (d))
 {
   x_span[d] = Axis_group_interface::generic_bound_extent (bounds[d], commonx, X_AXIS)[d];
-
-  if (connect_to_other[d])
+  
+  if (connect_to_other[d] && bounds[d]->break_status_dir())
 {
   Interval overshoot (robust_scm2drul (me->get_property ("break-overshoot"),
Interval (-0.5, 0.0)));
-
   if (d == RIGHT)
 x_span[d] += d * overshoot[d];
   else
-x_span[d] = (bounds[d]->break_status_dir ()
- ? Axis_group_interface::generic_bound_extent (bounds[d], commonx, X_AXIS)[-d]
- : robust_relative_extent (bounds[d], commonx, X_AXIS)[-d])
+x_span[d] = Axis_group_interface::generic_bound_extent (bounds[d], commonx, X_AXIS)[-d]
 - overshoot[LEFT];
 }
 
-- 
1.7.0.4

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: possible tuplet bug?

2013-12-25 Thread David Kastrup 
David Nalesnik  writes:

> See attached.  I apologize for any inconvenience.
>
> -David
> From 059ff4586a1e6a1fda27f40372a87db972be378c Mon Sep 17 00:00:00 2001
> From: David Nalesnik 
> Date: Wed, 25 Dec 2013 09:41:39 -0600
> Subject: [PATCH] fix problem with connect-to-neighbor and break-overshoot in 
> unbroken tuplets
>
> ---
>  lily/tuplet-bracket.cc |9 +++--
>  1 files changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/lily/tuplet-bracket.cc b/lily/tuplet-bracket.cc
> index 1eccaab..7171e2a 100644
> --- a/lily/tuplet-bracket.cc
> +++ b/lily/tuplet-bracket.cc
> @@ -202,18 +202,15 @@ Tuplet_bracket::calc_x_positions (SCM smob)
>for (LEFT_and_RIGHT (d))
>  {
>x_span[d] = Axis_group_interface::generic_bound_extent (bounds[d], 
> commonx, X_AXIS)[d];
> -
> -  if (connect_to_other[d])
> +  
> +  if (connect_to_other[d] && bounds[d]->break_status_dir())
>  {
>Interval overshoot (robust_scm2drul (me->get_property 
> ("break-overshoot"),
> Interval (-0.5, 0.0)));
> -
>if (d == RIGHT)
>  x_span[d] += d * overshoot[d];
>else
> -x_span[d] = (bounds[d]->break_status_dir ()
> - ? Axis_group_interface::generic_bound_extent 
> (bounds[d], commonx, X_AXIS)[-d]
> - : robust_relative_extent (bounds[d], commonx, 
> X_AXIS)[-d])

Actually, commenting inline in the mail is very much like working on a
Rietveld review, just with less of a record for it.

If it makes you feel more comfortable doing it in this "informal" way,
we should try figuring out whether it is "just you" or a general
phenomenon.

At any rate: here is what triggers my discomfort sensors while not
actually having any clue about what the code does:

robust_relative_extent (bounds[d], commonx, X_AXIS)[-d] served as a
fallback value for some cases previously.  It is not used at all now.
That seems strange.


> +x_span[d] = Axis_group_interface::generic_bound_extent 
> (bounds[d], commonx, X_AXIS)[-d]
>  - overshoot[LEFT];
>  }

-- 
David Kastrup

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: possible tuplet bug?

2013-12-28 Thread Janek WarchoĊ‚ 
Hi David(s, especially N) ;-)


2013/12/25 David Nalesnik :
> [tuplet problem]
> See attached.  I apologize for any inconvenience.

It's great to see you working on C++ stuff!  I'm sorry that i don't
have time to investigate this, but i'm rooting for you!

One piece of advice that i have: you may try using 'git blame' to find
the commit which originally introduced the problematic code (you will
probably have to use it repeatedly because of code reformattings that
happened since this was written).  There is no guarantee that you'll
find any explanation, but it's worth trying.

Also, try running the regression tests to see if your change has any
negative side-effects.

best,
Janek

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel