Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(2784)

Issue 557500043: Issue 5788: New French Beamimg Approach

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 1 month ago by Be-3
Modified:
2 years, 3 months ago
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 10

Patch Set 2 : Changes applied following the reviewers' comments #

Total comments: 1

Patch Set 3 : Final property name: french-beaming-stem-adjustment #

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

Unified diffs Side-by-side diffs Delta from patch set Stats (+145 lines, -86 lines) Patch
M Documentation/changes.tely View 1 1 chunk +33 lines, -0 lines 0 comments Download
M input/regression/beam-french.ly View 1 chunk +10 lines, -2 lines 0 comments Download
A input/regression/beam-standard-french-compare.ly View 1 1 chunk +24 lines, -0 lines 0 comments Download
M lily/beam.cc View 1 2 6 chunks +45 lines, -39 lines 0 comments Download
M lily/beam-quanting.cc View 1 1 chunk +6 lines, -5 lines 0 comments Download
M lily/include/beam.hh View 1 2 2 chunks +10 lines, -3 lines 0 comments Download
M lily/include/stem.hh View 1 chunk +1 line, -1 line 0 comments Download
M lily/stem.cc View 1 2 3 4 chunks +14 lines, -3 lines 0 comments Download
M lily/tuplet-number.cc View 2 chunks +0 lines, -33 lines 0 comments Download
M scm/define-grob-properties.scm View 1 2 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 22
Be-3
Deep breath---please review. Ta, Torsten
4 years, 1 month ago (2020-02-24 01:29:54 UTC) #1
lemzwerg
LGTM. Very nice, thanks! Some minor nits only. https://codereview.appspot.com/557500043/diff/551490044/Documentation/changes.tely File Documentation/changes.tely (right): https://codereview.appspot.com/557500043/diff/551490044/Documentation/changes.tely#newcode67 Documentation/changes.tely:67: @emph{exactly} ...
4 years, 1 month ago (2020-02-24 05:24:25 UTC) #2
hanwenn
LGTM special thanks for your well reasoned and extensive commit message. One thing to consider: ...
4 years, 1 month ago (2020-02-24 06:44:39 UTC) #3
Be-3
Proposed changes applied to my local branch (most of them), see comments. Ta, Torsten https://codereview.appspot.com/557500043/diff/551490044/Documentation/changes.tely ...
4 years, 1 month ago (2020-02-25 12:35:18 UTC) #4
Be-3
On 2020/02/24 06:44:39, hanwenn wrote: > One thing to consider: since the mechanics are now ...
4 years, 1 month ago (2020-02-25 13:06:31 UTC) #5
lemzwerg
> > s/i.e./i.e.,/ > > Ah, then I suppose the changes.tely language is American English... ...
4 years, 1 month ago (2020-02-25 13:33:13 UTC) #6
Be-3
On 2020/02/25 13:33:13, lemzwerg wrote: > If you really wanted 'i.e.' without a trailing comma, ...
4 years, 1 month ago (2020-02-25 14:02:27 UTC) #7
dak
On 2020/02/25 13:06:31, Be-3 wrote: > On 2020/02/24 06:44:39, hanwenn wrote: > > One thing ...
4 years, 1 month ago (2020-02-25 14:11:06 UTC) #8
dak
On 2020/02/25 13:33:13, lemzwerg wrote: > > > Please add one or more test cases ...
4 years, 1 month ago (2020-02-25 15:09:36 UTC) #9
lemzwerg
> It sounds like actual manipulation of that property > by the user would interfere ...
4 years, 1 month ago (2020-02-25 17:59:55 UTC) #10
Be-3
Changes applied following the reviewers' comments
4 years, 1 month ago (2020-02-26 21:04:59 UTC) #11
Be-3
On 2020/02/26 21:04:59, Be-3 wrote: > Changes applied following the reviewers' comments All of the ...
4 years, 1 month ago (2020-02-26 21:14:54 UTC) #12
hanwenn
On 2020/02/26 21:14:54, Be-3 wrote: > On 2020/02/26 21:04:59, Be-3 wrote: > > Changes applied ...
4 years, 1 month ago (2020-02-26 21:17:47 UTC) #13
dak
https://codereview.appspot.com/557500043/diff/563610046/scm/define-grob-properties.scm File scm/define-grob-properties.scm (right): https://codereview.appspot.com/557500043/diff/563610046/scm/define-grob-properties.scm#newcode1374 scm/define-grob-properties.scm:1374: amount of space in case of French beaming style.") ...
4 years, 1 month ago (2020-02-26 21:18:21 UTC) #14
Be-3
> I'll really make myself unpopular here, […] No, that's what discussions are for. I ...
4 years, 1 month ago (2020-02-26 21:43:53 UTC) #15
lemzwerg
> How about > * beamed-stem-french-adjustment > * french-beaming-stem-adjustment > …? I like the second ...
4 years, 1 month ago (2020-02-26 21:50:05 UTC) #16
dak
On 2020/02/26 21:50:05, lemzwerg wrote: > > How about > > * beamed-stem-french-adjustment > > ...
4 years, 1 month ago (2020-02-26 21:56:46 UTC) #17
thomasmorley651
On 2020/02/26 21:56:46, dak wrote: > On 2020/02/26 21:50:05, lemzwerg wrote: > > > How ...
4 years, 1 month ago (2020-02-26 22:58:30 UTC) #18
Be-3
Final property name: french-beaming-stem-adjustment
4 years, 1 month ago (2020-02-27 01:15:08 UTC) #19
Be-3
missing semicolon at end of statement (ahem...)
4 years, 1 month ago (2020-02-27 19:31:37 UTC) #20
islasromero05
2 years, 3 months ago (2021-12-16 13:09:04 UTC) #21
islasromero05
2 years, 3 months ago (2021-12-16 13:19:30 UTC) #22
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b