LilyIssues / Issues / #3115 Revert chord tremolo change involving beam-gap (issue 704)

Google Importer - 2013-09-30

Originally posted by: PhilEHol...@googlemail.com

This is verging on a regression. If you look at

http://lilypond.org/doc/v2.17/Documentation/snippets/repeats#repeats-engraving-tremolos-with-floating-beams

and compare it with

http://lilypond.org/doc/v2.16/Documentation/snippets/repeats#repeats-engraving-tremolos-with-floating-beams

it's apparent that, what used to work, no longer does. That's because \override Beam #'gap = #1.33 has been changed to \override Beam #'beam-gap = #'(1.33 . 1.33) and the only way I discovered this (having noticed the snippet doesn't work) was by git bisect, reading the commit and experimenting with alternatives. I do believe this is stable release blocking, but would like others to comment.

Labels: -Type-Enhancement Type-Scripts

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-09-30

Originally posted by: tdanielsmusic

Well, clearly we ought to fix the documentation before a release,
but that's easily done. I'd missed it because it's tagged as
Scripts rather than Documentation.

A convert-ly rule would be good, but the use of this particular
feature must be quite rare. Not sure that it should be release-
blocking, although a rule should be quite easy to generate too.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-09-30

Originally posted by: dak@gnu.org

I see several possible approaches here. I suspect that the separate beam-gap property was only introduced because the gap property does not permit a number pair, not because of any other use of the "gap" property in beams.

The easiest path to full compatibility would be to have ly:beam::calc-beam-gap look at the Beam grob's "gap" property first and, if it is set, use that.

Since we don't have any documentation available and this issue has not been worked on since January, that's probably the safest course.

Another, obviously, is to use a convert-ly rule but those can only convert a limited subset of overrides. Obviously, if we don't have to convert a number into a pair, the conversion rules have a better chance of working. Currently beam-gap is of type number-pair? so we would like to be this number-pair-or-number? (to be created, possibly also with a name describing intent rather than data structure, like interval-or-number? or number-or-interval?) in order to have a workably simple convert-ly rule.

Given how late we are in the game, I tend towards letting the default callback check "gap" first before doing its calculation. Possibly additionally allowing to override beam-gap with a number rather than an interval (this requires some more code at the place where beam-gap is being used).

In that way, even if we add a convert-ly rule, we can afford if it doesn't trigger.

Labels: -Type-Scripts Type-Critical
Cc: mts...@gmail.com

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-09-30

Originally posted by: PhilEHol...@googlemail.com

It would certainly seem that using gap if it exists, and converting that to a pair of gaps of the same numerical value would be best.

Failing that, a rule that converts gap to beam-gap and #n.m to #'(n.m . n.m) should work, I think.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-09-30

Originally posted by: dak@gnu.org

#n.m is not a problem. #(some complex expression possibly including side effects) is. It can also be a callback. _If_ we look up the gap property anyway, there's probably not all that much motivation for a convert-ly rule. Not as long as beam-gap is not usefully documentated anyway.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-09-30

Originally posted by: dak@gnu.org

Looking at the code, this is not actually an interval (even though it is queried using robust_scm2interval). Instead it is a pair of independent gap values for the left and for the right end of a beam segment.

This was issue 704. The documentation string for beam-gap is "Size of a gap in a @code{Beam}." which totally does not give any indication what the elements of the pair are supposed to be good for respectively or why this would be a pair rather than a single number.

The commit message is
commit [rd8dfa746ead381a80901106b9c9b079dc9b5d004]
Author: Mike Solomon <mike@apollinemike.com>
Date:   Fri Aug 31 09:27:17 2012 +0200

    Uses a heuristic to determine if chord tremolos collide with accidentals.

    This heuristic makes several assumptions about when a chord tremolo
    will collide with accidentals. It must be between whole notes, it
    must be in the staff, and it must be an ascending major third or lower.
    The heuristic is entirely contained in Beam::whole_note_close_chord_tremolo,
    which should be modified if other cases involving chord tremolos arise.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-09-30

Originally posted by: dak@gnu.org

Maybe we are better off reverting that patch anyway. If you take a look at the following part of the patch:
+MAKE_SCHEME_CALLBACK (Beam, calc_beam_gap, 1);
+SCM
+Beam::calc_beam_gap (SCM smob)
+{
+ Spanner *me = unsmob_spanner (smob);
+ SCM default_value = scm_cons (scm_from_double (0.8), scm_from_double (0.8));
+ if (!whole_note_close_chord_tremolo (me))
+ return default_value;
+
+ Interval left = Paper_column::left_non_note_column_width
+ (me->get_bound (RIGHT)->get_column ());
+
+ if (left.length () > 0.4)
+ return scm_cons (scm_from_double (0.8), scm_from_double (1.3 + left.length ()));
+ else
+ return default_value;
+}

then it is clear that
a) 0.8 now is hardwired instead of overridable
b) if you override beam-gap, the logic for dealing with issue 704 is locked out and the collision avoidance for whole notes with chord tremolos is inactivated
c) the code results in highly discontinuous behavior: when left.length passes 0.4, the second beam-gap value jumps from 0.8 to 1.7

In short: what this code does with the beam-gap does not make enough sense to keep it. It seems sanest to revert the patch and reopen issue 704. It's possible that parts of the patch and its underlying analysis can be turned into a good solution, but the beam-gap part of it does not appear tenable.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-09-30

Originally posted by: tdanielsmusic

I'm not so sure reverting is the best course. The code
fixes the very specific bug of tremolo beams between whole
notes when the second note carries one or two accidentals
by making the beams asymmetric, hence the need for a pair
of numbers rather than one. If the callback is overridden
with a specific pair of numbers it becomes the user's
responsibility to ensure the beams don't collide with any
accidentals present. That seems reasonable enough. I'm not
sure about the hard-wired values, but the jump in the second
beam-gap seems to be required to make a gap when an
accidental is present (?) Perhaps 'gap could be pressed
back into service to replace the hard-wired 0.8, with 0.8
as its default value? Then the existing examples would work
correctly again (I think).

Trevor

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-09-30

Originally posted by: dak@gnu.org

> I'm not so sure reverting is the best course.

The best course is a proper fix. It won't fall magically from the sky. The various parts of the patch are not documented, the commit description does not even mention beaming.

> The code fixes the very specific bug ...

It does so by removing a previously existing user setting.

> If the callback is overridden with a specific pair of numbers it becomes the user's responsibility to ensure the beams don't collide with any accidentals present.

Exactly. That's complete hogwash since it means that any change in the _style_ of beaming (that's why this is a user-customizable value) will result in the bug being back.

That's not a fix of the bug, it's putty and string. It is now impossible to override the equivalent of Beam.gap globally without getting the bug back, and there is no reasonable way a user can incorporate the bug fix into his override.

You write something like

> but the jump in the second beam-gap seems to be required to make a gap when an accidental is present (?)

But that's pure guess work as nothing is documented and there is no way to figure out what the magic values are for. So it's impossible to know how this code will react to differently sized accidentals (different style, cautionary in different size, cautionary with parens) and there is no way to figure out what one will have to do when the magic values don't fit. And then one will not just have to choose different settings but one will have to write new code.

It's not just that the settings are no longer user-serviceable, the code is not programmer-serviceable either.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-10-01

Originally posted by: dak@gnu.org

Issue 3115: Revert "Uses a heuristic to determine if chord tremolos collide with accidentals."

This reverts commit [rd8dfa746ead381a80901106b9c9b079dc9b5d004].

Conflicts:
scm/define-grobs.scm

http://codereview.appspot.com/14209043

Labels: Patch-new
Owner: dak@gnu.org
Status: Started

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-10-01

Originally posted by: dak@gnu.org

Patchy the autobot says: passes tests. No user visible changes, but obviously this may partly be due to the revert including the relevant regtest.

Labels: -Patch-new Patch-review

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-10-01

Originally posted by: pkx1...@gmail.com

Patchy the autobot says: passes make, make check and a full make doc.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-10-01

Originally posted by: dak@gnu.org

Reconstitute input/regression/chord-tremolo-accidental.ly

http://codereview.appspot.com/14209043

Labels: -Patch-review Patch-new

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-10-01

Originally posted by: dak@gnu.org

Patchy the autobot says: passes tests. Snapshot of the changed regtest follows. This is the expected reversal to the unfixed behavior.

Labels: -Patch-new Patch-review

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-10-01

Originally posted by: dak@gnu.org

(No comment was entered for this change.)

regtest3115.png

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-10-01

Originally posted by: dak@gnu.org

Actually, according to the regtest, we are "just" talking about collisions with the whole-note tremoli, and those already suffer from being horizontal which is almost always not what is wanted (tremoli are usually between different pitches).

So we are actually talking about a fix in an area LilyPond does not do properly in other aspects.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-10-04

Originally posted by: pkx1...@gmail.com

Patch on countdown for October 7 - 06:00 GMT

Labels: -Patch-review Patch-countdown

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-10-07

Originally posted by: dak@gnu.org

(No comment was entered for this change.)

Summary: Revert chord tremolo change involving beam-gap (issue 704)

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-10-07

Originally posted by: pkx1...@gmail.com

Patch counted down - please push

Labels: -Patch-countdown Patch-push

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-10-07

Originally posted by: dak@gnu.org

Pushed to staging as
commit [r2122a9643a5374edee7a02730b4945d298443afa]
Author: David Kastrup <dak@gnu.org>
Date:   Tue Oct 1 16:42:37 2013 +0200

    Issue 3115: Revert "Uses a heuristic to determine if chord tremolos collide with accidentals."

    This reverts commit [rd8dfa746ead381a80901106b9c9b079dc9b5d004].

    Conflicts:
        scm/define-grobs.scm

    But reconstitute input/regression/chord-tremolo-accidental.ly. Not
    done in a separate commit in order to preserve blame.

Labels: -Patch-push Fixed_2_17_29
Status: Fixed

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-10-21

Originally posted by: Elu...@gmail.com

(No comment was entered for this change.)

Status: Verified

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Revert chord tremolo change involving beam-gap (issue 704)

Issue Tracker for LilyPond

Searches

Help

#3115 Revert chord tremolo change involving beam-gap (issue 704)

Related

Discussion