Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Tremolo Fixes for #18173 #18259

Merged
merged 5 commits into from Jun 30, 2023
Merged

Tremolo Fixes for #18173 #18259

merged 5 commits into from Jun 30, 2023

Conversation

asattely
Copy link
Contributor

@asattely asattely commented Jun 28, 2023 •
edited by oktophonie

Resolves: #18173

  • Relayout issue: The stem direction was based on its tremolo (like chords in beams) but the tremolo hadn't necessarily been laid out yet, so we were getting incorrect direction on score load.
  • Beam styles: I don't know where they went, but they're back now
  • Tremolo beams forced horizontal: There was a bug in the combined tremolo/beam placement code that was treating very specific combinations of multi-note chords in two-chord tremolos as beams with more than two notes. Weird. That's been fixed also
  • Flipping manually-adjusted trems: Also fixed, the cause of this was the relayout issue above. Note that this is an engraving fix only--the properties panel will be addressed in a different PR
  • Inserting time inside two-note trems: Any tremolo that is split in this way is removed and the two notes are retained.

@oktophonie oktophonie added the vtests This PR produces approved changes to vtest results label Jun 28, 2023
@oktophonie oktophonie mentioned this pull request Jun 28, 2023
Closed
RomanPudashkin
RomanPudashkin reviewed Jun 29, 2023
View reviewed changes
src/engraving/libmscore/tremolo.cpp Outdated
double offset = gapSp * spatium();
if (_style == TremoloStyle::TRADITIONAL_ALTERNATE) {
mainStroke->line = LineF(startAnchor, endAnchor);
startAnchor.rx() += offset;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't use these rx() and ry(). Use setX and setY

RomanPudashkin
RomanPudashkin reviewed Jun 29, 2023
View reviewed changes
src/engraving/layout/v0/beamtremololayout.cpp Outdated
@@ -894,10 +894,12 @@ int BeamTremoloLayout::isSlopeConstrained(int startNote, int endNote) const
// 0 to constrain to flat, 1 to constrain to 0.25, <0 for no constraint
if (startNote == endNote) {
return 0;
} else if (m_beamType == BeamType::TREMOLO) {
return -1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isSlopeConstrained is not a good name for this type of method. It should either return bool or have another name

RomanPudashkin
RomanPudashkin reviewed Jun 29, 2023
View reviewed changes
src/engraving/layout/v0/beamtremololayout.cpp
@@ -894,10 +894,12 @@ int BeamTremoloLayout::isSlopeConstrained(int startNote, int endNote) const
// 0 to constrain to flat, 1 to constrain to 0.25, <0 for no constraint
if (startNote == endNote) {
return 0;
} else if (m_beamType == BeamType::TREMOLO) {
return -1;
}
// if a note is more extreme than the endpoints, slope is 0
// p.s. _notes is a sorted vector
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong comment

@asattely
Copy link
Contributor Author

All @RomanPudashkin's suggestions implemented, all of the engraving-related points of the original issue (not the UI one for tremolo stem direction) are now addressed!

@oktophonie oktophonie mentioned this pull request Jun 30, 2023
Open
@RomanPudashkin RomanPudashkin merged commit 18cd25e into musescore:master Jun 30, 2023
10 of 11 checks passed
@RomanPudashkin
Copy link
Contributor

@asattely thanks! Looks much better!

@asattely asattely mentioned this pull request Jun 30, 2023
Merged
RomanPudashkin added a commit that referenced this pull request Jun 30, 2023
@RomanPudashkin
Merge pull request #18324 from asattely/tremolo-fixes-410
7185eb8 
PR #18259 for 4.1.0 (Tremolo fix megaissue)
@alexpavlov96 alexpavlov96 mentioned this pull request Jul 2, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oktophonie oktophonie oktophonie approved these changes

@RomanPudashkin RomanPudashkin RomanPudashkin approved these changes

Assignees
No one assigned
Labels
vtests This PR produces approved changes to vtest results
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Multiple bugs related to tremolos between notes in MS 4.1
3 participants
@asattely @RomanPudashkin @oktophonie