More beautiful default layout related to tremolos (fix #20390) (+ collect_artifacts) #5619

Harmoniker1 · 2020-01-17T21:06:05Z

This PR uses minAbsStemLength() for all stems with tremolos (default placement and placing mid-stem), after some modification.

Below are a quick overview and some explanations. All layout principles stated below are in Behind Bars:

Layout diff:

See #5619 (review) for the layout change of the tremolo in the third measure

Improvements for single-note tremolos:

Fixes https://musescore.org/node/20390. The tremolo should be inside the staff. This is done here in Tremolo::layoutOneNoteTremolo().
Avoid collision between hook/beam and tremolo by extending stem. This is done in Chord::minAbsStemLength() with the calculation of beamLvl and beamDist. It becomes very effective by calculating a fairly new variable: the vertical distance between note and the far edge of tremolo, which can be seen here.
Set a minimum value and reasonable interval of note-tremolo distance, and extend stem accordingly. The former is done here in Tremolo::layoutOneNoteTremolo(). and the latter is done in Chord::minAbsStemLength(). This is based on the principle that nearest distance between chord and tremolo stroke should be no less than 1sp, which is represented by 3.0 in the code.

Improvements for two-note tremolos:

Reduce slope of beam stroke(s) if too steep by extending stem (if stem exists, which can automatically alter tremolo layout subsequently) or altering tremolo layout (if no stem). This is seen in various examples provided by Behind Bars. The new stem lengths are calculated in a new function extendedStemLenWithTwoNotesTremolo(), which is called after the default stem lengths of two notes are both calculated. Semibreves and longer notes don't have stem, so for those tremolos. the layout is directly altered. Since layout of these tremolos also depend on the concept of "stem length" even if there're no stem, in here, extendedStemLenWithTwoNotesTremolo() is called externally to get the right layout.
Avoid collison between beam and tremolo. This is also done in Chord::minAbsStemLength().

MarcSabatella · 2020-01-18T05:04:51Z

Definitely a big improvement! I wonder though, if it's really necessary to make the stem so long with the hooks? Seems Gould is OK with the hooks overlapping a bit, and it looks less weird to me. Maybe 1sp less long for that second note?

Also, I'm not sure the ramifications of altering stem length in "minAbs..." versus "default..." but be sure there are no weird dependencies where this causes, say, autoplaced text above the staff to not position itself properly. Could you do an update with "+ collect_artifacts" to make it easier for others to test?

Harmoniker1 · 2020-01-18T06:08:22Z

I wonder though, if it's really necessary to make the stem so long with the hooks?

Under this musical font, you'll probably find the occasions of 16th and shorter notes pretty ugly, if the stem is shorter, even if an 8th note looks fine, because the offset of different hooks is, well, different. But if you then want to differentiate the offset based on note duration, then some other musical fonts, which have the same offset for all kinds of hooks, will not be happy too. Besides, this is not that weird to look at. Personally, I find shortening the stem another kind of "weird" too.

Harmoniker1 · 2020-01-18T06:09:41Z

Could you do an update with "+ collect_artifacts" to make it easier for others to test?

Commit message or PR title?

MarcSabatella · 2020-01-18T19:08:30Z

Commit message. See the "git workflow" documentation for MuseScore for more on build artifacts. I do this when I particularly want to get some real world testing of a proposed change, it allows people to download a nightly build without needing to build for themselves.

Harmoniker1 · 2020-01-19T05:29:20Z

@MarcSabatella Done. I saw that artifacts had been attached to the build job.

libmscore/tremolo.cpp

Harmoniker1 · 2020-01-26T15:41:26Z

libmscore/tremolo.cpp

-            y1 = _chord1->stemPosBeam().y() - firstChordStaffY + _chord1->defaultStemLength();
-            y2 = _chord2->stemPosBeam().y() - firstChordStaffY + _chord2->defaultStemLength();
+            const std::array<double, 2> extendedLen 
+               = extendedStemLenWithTwoNoteTremolo(this, spatium(), _chord1->defaultStemLength(), _chord2->defaultStemLength());
+            y1 = _chord1->stemPos().y() - firstChordStaffY + extendedLen[0];
+            y2 = _chord2->stemPos().y() - firstChordStaffY + extendedLen[1];


In case anyone is wondering, regarding the stemPosBeam() to stemPos() change: this is intended. We're actually getting the y coordinate of stem tip here by calculating stem position (which is where it's attached to a note) and stem length. stemPosBeam() gives the position of the nearest note to beam, whereas stemPos() gives the real start position of the stem (the position of the furthest note to beam). So stemPosBeam() is actually a bad mistake.

EDIT: In the PR description there's an example (tremolo in the third measure) of how this affects layout.

worldwideweary · 2020-01-28T19:58:59Z

Hey Howard, looks way better.
Additionally, have you looked into the cross-staffing between tremolos? Maybe in the future you could fix that up (not trying to push my luck of course). For example, as it is currently:

The note on the right belongs to the (not shown) staff below through cross-staffing.

A screen capture of what is "supposed to be correct" is included here:

The dotted lines are apparently to provide a visual aid as to how the placement should be considered. I figured since you seem to have a grip on what's going on, maybe you can check it out or something :)

Anyways, thanks again for this PR.

Harmoniker1 · 2020-01-29T13:47:48Z

@worldwideweary I'm working on it, but there're some problems I cannot get rid of very soon. I posted a question in the developers' chat and haven't get any answer yet.

MarcSabatella · 2020-02-16T21:20:34Z

libmscore/layout.cpp

+                                    // if there is a two-note tremolo attached, and it is too steep,
+                                    // extend stem of one of the chords (if not cross-staff)
+                                    // or extend both stems (if cross-staff)
+                                    // this should be done after the stem lengths of two notes are both calculated


I guess the problem you are having is that there is no way stem lengths can be known this early for the cross staff case, at least not if beams are involved. Probably not for the cross-measure case either. This is only finalized later.

While it might be possible to resolve these issues with some work, to me it would be acceptable to not do any such adjustment for cross-staff or cross-measure cases,and just let users contonue to need to adjust these manually. not sure how easy it is to detect cross-measure at this point though.

Aside: One of the issues of leaving customization of tremolos between notes up to the user is that the user can't adjust the "grow left/right" (cf. feather beams terminology) related to the tremolos (or maybe they can and I'm missing something). Although the placement algorithm works well enough for most use cases, maybe it would be appropriate to have inspector attributes for changing the angles of note tremolos for some "off cases", ... but that's another story.

@MarcSabatella Manual adjustment can work for minim cross-staff tremolo and shorter, because you can adjust stem lengths. But for semibreve and longer there's no way of manual adjustment.

Plus, there aren't any grips ("adjustment handles") for the tremolo strokes themselves. Inspector attributes are nice, but it's nicer to have grips ;-)

True, I mixed up one and two note tremolos in my head here. Anyhow, I assume it's still the case that this isn't going to work here, you'd need to do this at the point where the stem length is actually calculated during cross-staff layout.

Stem lengths are correct now, it's stemPos() (or stem()->pagePos()) that's wrong. But thanks for replying!

Stem directions also are not known for sure until cross staff processing, and thus, stem positions too.

libmscore/chord.cpp

libmscore/layout.cpp

libmscore/chord.cpp

libmscore/layout.cpp

libmscore/tremolo.cpp

igorkorsukov · 2020-04-08T14:11:06Z

libmscore/tremolo.cpp

+                  t = up ? -3.0 - (2.0 * (lines() - 1)) * td - 2.0 * sw : 3.0;
+                  }
+            else { 
+                  if      (!up && !(line & 1)) // stem is down; even line


I intend to align the conditionals, in this way it's clearer.

fix musescore#20390: tremolo through stem collide with ledger lines

…ng it in a Chord member function + provide a commented-out part of adjusting stem lengths for cross-staff two-note tremolos that currently isn't working in some cases

…Between() (+ collect_artifacts) The case of two chords having the same `up()` value is already taken care of in the usage of `extendedStemLenWithTwoNoteTremolo()`.

+ fix one bug regarding `_spatium` not considerated

igorkorsukov · 2020-04-14T13:06:23Z

@Howard-C vtests failed

Harmoniker1 · 2020-04-14T13:30:20Z

@Howard-C vtests failed

Yes, @AntonioBL told me failure just indicates some generated images were different from previous commits. It doesn't necessarily mean something went wrong. In this case, tremolo-1.png and grace-4.png are different, which is expected. You can download the artifact as suggested in https://github.com/musescore/MuseScore/pull/5619/checks?check_run_id=585441612 and see for yourself, nothing "failed".

Jojo-Schmitz · 2020-04-20T09:10:01Z

Not good for tremolos on grace notes.
Before (bad):

After (too long stem, too big tremolo strokes):

Came up in #5449
It should be smaller and thicker tremolo strokes, matching the smaller notes. A grace note tremolo should look like a normal tremolo, just smaller.

Harmoniker1 · 2020-04-20T12:30:09Z

Behind Bars doesn't specify this, probably because the chance of adding tremolos to grace notes is too rare. But you're probably right.

Jojo-Schmitz · 2020-04-20T12:47:47Z

Same for small notes, so probably should get fixed in the same go

Harmoniker1 requested a review from MarcSabatella January 17, 2020 21:06

Harmoniker1 mentioned this pull request Jan 18, 2020

Support "all lines attached to stem" beam style of tremolo (partially fix #18897) #5612

Merged

Harmoniker1 force-pushed the tremolo-layout branch from 180f8fe to 01319ff Compare January 19, 2020 03:41

Harmoniker1 changed the title ~~More beautiful default layout related to tremolos (fix #20390)~~ More beautiful default layout related to tremolos (fix #20390) (+ collect_artifacts) Jan 19, 2020

Harmoniker1 commented Jan 19, 2020

View reviewed changes

libmscore/tremolo.cpp Outdated Show resolved Hide resolved

Harmoniker1 force-pushed the tremolo-layout branch from 01319ff to 84f0074 Compare January 19, 2020 14:57

anatoly-os added the Improvement label Jan 20, 2020

Harmoniker1 force-pushed the tremolo-layout branch 5 times, most recently from be487b0 to 1eb3628 Compare January 26, 2020 15:29

Harmoniker1 commented Jan 26, 2020

View reviewed changes

Harmoniker1 force-pushed the tremolo-layout branch 4 times, most recently from 9b948ee to acd5da0 Compare January 27, 2020 06:00

Harmoniker1 force-pushed the tremolo-layout branch 2 times, most recently from b1c8e59 to 6018579 Compare January 29, 2020 13:46

Harmoniker1 force-pushed the tremolo-layout branch from 6018579 to d44acb4 Compare February 4, 2020 15:14

MarcSabatella reviewed Feb 16, 2020

View reviewed changes

Harmoniker1 force-pushed the tremolo-layout branch 2 times, most recently from 8c16ad2 to 42820ca Compare February 22, 2020 06:12