LilyIssues / Issues / #5788 New French Beamimg Approach

Torsten Hämmerle - 2020-02-24

Attachments has changed:

Diff:

--- old +++ new @@ -0,0 +1 @@ +issue5788-old.pdf (101.6 kB; application/pdf)
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Torsten Hämmerle - 2020-02-24

Issue 5788: New French Beamimg Approach

Completely new approach to French beaming.
This will automatically tackle all kinds of not-yet resolved positioning
problems caused by the current French beaming implementation.

==========
BASIC IDEA
==========

Hypothesis: The only difference between standard and French beaming
is that French "inner group" stems will not pass through
all the beams.
THAT'S ALL! Nothing else about it!

The current approach to generally shorten French stems from the very
beginning causes many follow-up positioning problems that have to
be remedied later-on in many different places by neutralizing this
deviation somehow.
Tuplet numbers (w/o tuplet brackets) have already been dealt with, but
many other problems still remain.

GENERAL SOLUTION (in short)

Junk all exceptions and do not distinguish between French and standard
beaming at all (quite radical, but extremely helpful)

That way we can be sure that all positionings will exactly match
the standard beaming case

Only at the very end, when it comes to actually printing the stem, it
has to be shortened by the appropriate amount.

GENERAL SOLUTION (more detailed)

Junk all (!) existing French beaming special treatment, using standard
stem lenghts throughout for all calculations so that everything exactly
matches the standard beaming case automatically.

The standard beam length is extremely important for all the positioning
calculations in several ways:

Special beam properties as all kinds of minimum lengths will only work
correctly for standard length beams and there's no need at all for
introducing more and more exceptions for French beams

Stem ends are crucial for placement of all kinds of grobs such as
articulations, fingerings, text scripts and they must behave
as if all the stems had standard length (property unchanged!) for
vertical positioning of grobs aligned to the stem ends.

ONLY when it comes to actually printing the stem, French shortening
becomes relevant.
So only the ly:stem::pring function will actually need to know
about the new french-correction property, using (standard) "length"
property minus new "french-correction" property.

=============
MODIFICATIONS
=============

modified: Documentation/changes.tely

Mention that French beaming will now exactly behave like standard
beaming in all beam positionings and all articulation, fingering,
etc. placements will now be identical to the standard beaming case.
Show an example comparing standard/French beaming (w/o displaying
the coding, though. It's just about demonstrating the (now) exactly
matching positioning.

modified: input/regression/beam-french.ly

(existing) regression file. One beam positioning flaw (too far away
from the noteheads) can already be seen in the current test cases,
but I've added a few more examples containing exceptionally critical cases.

modified: scm/define-grob-properties.scm

Introduce new property "french-correction"

modified: lily/include/stem.hh

Additional Real parameter for set_stem_positions to pass over
french_correction in order to be able to leave standard stem length
untouched and set both property "length" and "french-correction".

modified: lily/stem.cc

Stem::set_stem_positions

additional parameter Real fc for french_correction value

Set Stem property "length" as before, but now containing the
unaltered (i.e. "unfrenched") stem length

if (and only if) fc is non-zero, set "french-correction" property
of the Stem

Callback Stem::print

retrieve the (now unfrenched) stem-length property

retrieve the (new) french-correction property (0 if non-existent)

subtract french_correction from the standard stem_length
(for printing the correctly shortened stem)

Add new property "french-correction" to the interface

modified: lily/include/beam.hh

New struct Beam_stem_length
containing stem_y_ (as before, but unaltered by French beaming)
plus a french_correction_ containing the Frech beaming stem length
delta.

Function Beam::calc_stem_y adaped

return Beam_stem_length rather than a mere Real stem_y

Boolean parameter "french" replaced by int value french_count
for passing over the individual number of beam translations the
stem has to be shortened by (if applicable;
0 for standard beaming or standard-length outer group French beams).

modified: lily/beam.cc

Junk (now) obsolete French beaming special treatment

Initialize new struct Beam_stem_length

Adapt Beam::calc_stem_y

return new struct Beam_stem_length

replace bool french by int french_count

in case of non-zero french_count, calculate french_correction
respecting beam_translation and feather_factor

pass over (now unaltered!) stem_y + id as before plus
french_correction via new struc Beam_stem_length

Adapt Beam::set_stem_lengths

Determine value of french_count by intersecting left and right
Slices of the stem's beaming property

Call Beam::calc_stem_y passing over french_count

Call Stem::set_stem_positions now with an additional parameter
so that finally, both of the Stem properties "length" and
new "french-correction" can be set.

modified: lily/beam-quanting.cc

Junk (now) obsolete French beaming special treatment

Call of Beam::calc_stem_y adapted to return a structure rather than
a single Real value

REMARK: French beaming is irrelevant (and even obstructive) for beam
quanting (i.e. positioning) and so this feature is deliberately not
being used and when calling calc_stem_y, french_count 0 is being
passed over in any case to suppress unnecessary calculations.

modified: lily/tuplet-number.cc

Completely junk (now) obsolete French beaming special treatment,
no exceptions are needed anymore.

=========
IMPORTANT:
=========
All hitherto unresolved placement issues (articulations, fingerings, etc.)
caused by French beaming are automatically remedied by this completely
new French beaming approach, as we are using standard beam lenghts
throughout for all calculations, callbacks, positionings, minimum-length
rules, and so forth...

Cheers,
Torsten

https://codereview.appspot.com/557500043
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Description has changed:

Diff:

--- old
+++ new
@@ -1,10 +1,11 @@
+https://codereview.appspot.com/557500043
 # Completely new approach to French beaming
 This will automatically tackle all kinds of not-yet resolved positioning problems caused by the current French beaming implementation.

 As this is quite a radical and complete re-design of the LilyPond&#39;s current French beaming approach, I&#39;ve decided to open up this issue because a general French beaming overhaul can not be associated with individual bug-related issues.

 # Basic Idea (Hypothesis)
-The *only* difference between standard and French beaming should be that Frech &#34;inner group&#34; stems will not pass through all the beams.  That&#39;s all!  It&#39;s as easy as this!
+The *only* difference between standard and French beaming should be that French &#34;inner group&#34; stems will not pass through all the beams.  That&#39;s all!  It&#39;s as easy as this!
 *Unfortunately, all that Elaine Gould has to say about French beaming is: &#34;don&#39;t do it!&#34;.* 

 # Current Implementation
@@ -24,3 +25,5 @@

 Cheers,
 Torsten
+
+PS: **New French Beaming Comparison PDF** attached.

Attachments has changed:

Diff:

--- old
+++ new
@@ -1 +1,2 @@
+issue5788-new.pdf (102.3 kB; application/pdf)
 issue5788-old.pdf (101.6 kB; application/pdf)

Needs: -->
Type: -->

Anonymous - 2020-02-24

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

Anonymous - 2020-02-24

Passes make, make check and a full make doc - comments on Rietveld

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

Anonymous - 2020-02-24

reg test diffs

Screenshot from 2020-02-24 12-09-18.png

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
- Torsten Hämmerle - 2020-02-25
  
  Not taking into account the newly added examples in
  input/regression/beam-french.ly
  both regression files dealing with French beaming clearly show intentional deviations.
  And, most importantly, the reg test difss prove that the new French beaming mechanism does not impair standard beaming.
  
  So: "All's Well That Ends Well" [Shakespeare about stems]
  
  If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2020-02-26

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

Anonymous - 2020-02-26

Patch on countdown for Feb 28th

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

Torsten Hämmerle - 2020-02-26

Changes applied following the reviewers' comments

https://codereview.appspot.com/557500043

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

Torsten Hämmerle - 2020-02-26

Following Werner's suggesition, I added a new regression file directly comparing French beaming to standard beaming so that any deviation from standard will immediately catch one's eye.
When playing around with it, I was utterly surprised to find that in the original implementation, slurs didn't work, either.

Please find the reg test example attached (with old and new French beaming)

new-regtest-.png

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

Torsten Hämmerle - 2020-02-27

Final property name: french-beaming-stem-adjustment

https://codereview.appspot.com/557500043

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

Anonymous - 2020-02-27

Needs: -->

Patch: new --> needs_work

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

This fails make

Making lily/out/bar-check-iterator.o < cc
Making lily/out/stem.o < cc
Making lily/out/balloon-engraver.o < cc
Making lily/out/system-start-delimiter-engraver.o < cc
/home/jlowe/lilypond-git/lily/stem.cc: In static member function 'static scm_unused_struct* Stem::print(SCM)':
/home/jlowe/lilypond-git/lily/stem.cc:903:3: error: expected ';' before 'Real'
   Real y2 = dir * stem_length + y1;
   ^~~~
/home/jlowe/lilypond-git/lily/stem.cc:905:45: error: 'y2' was not declared in this scope
   Interval stem_y = Interval (std::min (y1, y2), std::max (y2, y1)) * half_space;
                                             ^~
/home/jlowe/lilypond-git/lily/stem.cc:905:45: note: suggested alternative: 'yn'
   Interval stem_y = Interval (std::min (y1, y2), std::max (y2, y1)) * half_space;
                                             ^~
                                             yn
Making lily/out/fingering-engraver.o < cc
Making lily/out/concurrent-hairpin-engraver.o < cc
/home/jlowe/lilypond-git/build/../stepmake/stepmake/c++-rules.make:4: recipe for target 'out/stem.o' failed
make[1]: *** [out/stem.o] Error 1
make[1]: *** Waiting for unfinished jobs....
/home/jlowe/lilypond-git/build/../stepmake/stepmake/generic-targets.make:6: recipe for target 'all' failed
make: *** [all] Error 2

Torsten Hämmerle - 2020-02-27

Blimey, in the final renaming (patch set 3) a semicolon at the end of a statement just vanished into thin air without me noticing.
How very embarrassing, sorry for that!

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

Torsten Hämmerle - 2020-02-27

missing semicolon at end of statement (ahem...)

https://codereview.appspot.com/557500043

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

Anonymous - 2020-02-27

Passes make, make check and a full make doc - comments on Rietveld

Reg test diffs attached

Screenshot from 2020-02-27 20-08-55.png

Screenshot from 2020-02-27 20-09-11.png

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

Anonymous - 2020-03-01

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

Anonymous - 2020-03-01

Patch on countdown for March 3rd

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

Anonymous - 2020-03-03

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

Anonymous - 2020-03-03

Patch counted down - please push.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
- Torsten Hämmerle - 2020-03-03
  
  Patch formatted against current master attached.
  Please push it for me.
  
  Thanks,
  Torsten
  
  0001-Issue-5788-New-French-Beamimg-Approach.patch
  
  If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

David Kastrup - 2020-03-03

labels: --> Fixed_2_21_0

status: Started --> Fixed

Patch: push -->

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

Pushed to staging (with a somewhat exuberant commit message) as
commit cf714372e43703b6a76e2abcfacbda912f66d662
Author: Torsten Hämmerle torsten.haemmerle@web.de
Date: Tue Feb 18 22:22:42 2020 +0100

Issue 5788: New French Beamimg Approach

Completely new approach to French beaming.
This will automatically tackle all kinds of not-yet resolved positioning
problems caused by the current French beaming implementation.

==========
BASIC IDEA
==========

Hypothesis: The *only* difference between standard and French beaming
            is that French "inner group" stems will not pass through
            all the beams.
            THAT'S ALL! Nothing else about it!

The current approach to generally shorten French stems from the very
beginning causes many follow-up positioning problems that have to
be remedied later-on in many different places by neutralizing this
deviation somehow.
Tuplet numbers (w/o tuplet brackets) have already been dealt with, but
many other problems still remain.

GENERAL SOLUTION (in short)

- Junk all exceptions and do not distinguish between French and standard
  beaming at all (quite radical, but extremely helpful)
- That way we can be sure that all positionings will exactly match
  the standard beaming case
- Only at the very end, when it comes to actually printing the stem, it
  has to be shortened by the appropriate amount.

GENERAL SOLUTION (more detailed)

- Junk all (!) existing French beaming special treatment, using standard
  stem lenghts throughout for all calculations so that everything exactly
  matches the standard beaming case automatically.
- The standard beam length is extremely important for all the positioning
  calculations in several ways:
  * Special beam properties as all kinds of minimum lengths will only work
    correctly for standard length beams and there's no need at all for
    introducing more and more exceptions for French beams
  * Stem ends are crucial for placement of all kinds of grobs such as
    articulations, fingerings, text scripts and they *must* behave
    as if all the stems had standard length (property unchanged!) for
    vertical positioning of grobs aligned to the stem ends.
  * ONLY when it comes to actually printing the stem, French shortening
    becomes relevant.
    So *only* the ly:stem::pring function will actually need to know
    about the new french-correction property, using (standard) "length"
    property minus new "french-correction" property.

=============
MODIFICATIONS
=============

        modified:   Documentation/changes.tely

Mention that French beaming will now exactly behave like standard
beaming in all beam positionings and all articulation, fingering,
etc. placements will now be identical to the standard beaming case.
Show an example comparing standard/French beaming (w/o displaying
the coding, though.  It's just about demonstrating the (now) exactly
matching positioning.

        modified:   input/regression/beam-french.ly

(existing) regression file. One beam positioning flaw (too far away
from the noteheads) can already be seen in the current test cases,
but I've added a few more examples containing exceptionally critical cases.

        modified:   scm/define-grob-properties.scm

Introduce new property "french-correction"

        modified:   lily/include/stem.hh

Additional Real parameter for set_stem_positions to pass over
french_correction in order to be able to leave standard stem length
untouched and set both property "length" and "french-correction".

        modified:   lily/stem.cc

- Stem::set_stem_positions
  * additional parameter Real fc for french_correction value
  * Set Stem property "length" as before, but now containing the
    unaltered (i.e. "unfrenched") stem length
  * if (and only if) fc is non-zero, set "french-correction" property
    of the Stem
- Callback Stem::print
  * retrieve the (now unfrenched) stem-length property
  * retrieve the (new) french-correction property (0 if non-existent)
  * subtract french_correction from the standard stem_length
    (for printing the correctly shortened stem)
- Add new property "french-correction" to the interface

        modified:   lily/include/beam.hh

- New struct Beam_stem_length
  containing stem_y_ (as before, but unaltered by French beaming)
  plus a french_correction_ containing the Frech beaming stem length
  delta.
- Function Beam::calc_stem_y adaped
  * return Beam_stem_length rather than a mere Real stem_y
  * Boolean parameter "french" replaced by int value french_count
    for passing over the individual number of beam translations the
    stem has to be shortened by (if applicable;
    0 for standard beaming or standard-length outer group French beams).

        modified:   lily/beam.cc

- Junk (now) obsolete French beaming special treatment
- Initialize new struct Beam_stem_length
- Adapt Beam::calc_stem_y
  * return new struct Beam_stem_length
  * replace bool french by int french_count
  * in case of non-zero french_count, calculate french_correction
    respecting beam_translation and feather_factor
  * pass over (now unaltered!) stem_y + id as before plus
    french_correction via new struc Beam_stem_length
- Adapt Beam::set_stem_lengths
  * Determine value of french_count by intersecting left and right
    Slices of the stem's beaming property
  * Call Beam::calc_stem_y passing over french_count
  * Call Stem::set_stem_positions now with an additional parameter
    so that finally, both of the Stem properties "length" and
    new "french-correction" can be set.

        modified:   lily/beam-quanting.cc

- Junk (now) obsolete French beaming special treatment
- Call of Beam::calc_stem_y adapted to return a structure rather than
  a single Real value
- REMARK: French beaming is irrelevant (and even obstructive) for beam
  quanting (i.e. positioning) and so this feature is deliberately not
  being used and when calling calc_stem_y, french_count 0 is being
  passed over in any case to suppress unnecessary calculations.

        modified:   lily/tuplet-number.cc

Completely junk (now) obsolete French beaming special treatment,
no exceptions are needed anymore.

=========
IMPORTANT:
=========
All hitherto unresolved placement issues (articulations, fingerings, etc.)
caused by French beaming are automatically remedied by this completely
new French beaming approach, as we are using standard beam lenghts
throughout for all calculations, callbacks, positionings, minimum-length
rules, and so forth...

Cheers,
Torsten

Rename and add new regression file

Patch Set 2

The following changes have been applied following the reviewers' comments:

Rename stru Beam_stem_length ->  Beam_stem_end (Han-Wen)
Rename property french-correction -> stem-end-shorten (Han-Wen)

Make property stem-end-shorten (was: french-correction) internal (DAK)

Corrections to doc text (Werner)

New regression file added directly comparing french beaming to standard
beaming (Werner)

On my behalf:
Modified changes.tely LilyPond example to show more variations in less
space so that it will not stick out into the margin in PDF version.

Removed an unneccessary if, just multiplying by int 0 in this case.

Full test successfully performed.

Patch set 3: Rename stem-end-shorten -> french-beaming-stem-adjustment

Patch set 4: missing semicolon at end of statement (ahem...)

New French Beamimg Approach

Issue Tracker for LilyPond

Searches

Help

#5788 New French Beamimg Approach

Completely new approach to French beaming

Basic Idea (Hypothesis)

Current Implementation

New Approach

Discussion