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.

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

backwards melisma #4946

Merged
merged 4 commits into from Apr 26, 2019

Conversation

Projects
None yet
3 participants
@MarcSabatella
Copy link
Contributor

commented Apr 20, 2019

There are a few situations where we draw melisma lines backwards or otherwise incorrectly, this PR should fix them.

The first fix is a very simple change that should prevent any and all backwards melisma lines, leaving the zero length instead. The second change is bit more involved, it fundamentally changes the calculation of the end element for melisma lines. As such, it definitely warrants further review/testing. In the basic cases it should always result in the same thing, but it's the overlapping voices where it changes, hopefully always for the better.

@@ -183,7 +183,7 @@ SpannerSegment* LyricsLine::layoutSystem(System* system)
if (tick() >= stick) {
layout();
if (ticks().isZero()) // only do layout if some time span
return 0;
return nullptr;

This comment has been minimized.

Copy link
@Jojo-Schmitz

Jojo-Schmitz Apr 21, 2019

Contributor

Should this be made part of the MuseScore coding style?
It isn't strictly needed, as a 0 in any pointer context automagically turns into a nullptr (C++) or NULL (C), but might be better doe selfdocumenting code?

This comment has been minimized.

Copy link
@MarcSabatella

MarcSabatella Apr 21, 2019

Author Contributor

Maybe so. I know it gets mentioned in code reviews, so I just think of it as part of the style even if it is not officially.

// lyrics ticks should already indicate the segment we want
// except for TEMP_MELISMA_TICKS case
Lyrics* l = toLyricsLine(this)->lyrics();
Fraction tick = (l->ticks().ticks() == Lyrics::TEMP_MELISMA_TICKS) ? l->tick() : l->endTick();

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 Apr 22, 2019

Contributor

To make it more clear, was the issue that LyricsLine::tick2() returned a wrong tick value or was it findCRinStaff that returned a chord from a wrong segment?

This comment has been minimized.

Copy link
@MarcSabatella

MarcSabatella Apr 22, 2019

Author Contributor

Well, findCRinStaff() does what it supposed to do, it's the right thing for most line types, it's just the wrong thing for melisma lines which have their own unique rules. There is no value we can pass that function that will produce what we want, so no way to improve the result just by altering LyricsLine::tick2(). Although now that you mention it, it probably would be better to use that value here instead of Lyrics::tick2(). They aren't necessarily the same, but probably they should be. I will take another look at this.

@MarcSabatella MarcSabatella force-pushed the MarcSabatella:282099-melisma-backwards branch from a2f69e7 to e6743f9 Apr 22, 2019

@MarcSabatella

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2019

I updated the PR just to add some comments & fix a debug message. I tried making the lyricsline tick2 same as the lyrics, but it just isn't designed to work that way - the lyricsline tick2 is for the next note, same as for all other lines. So I do need to continue to use lyrics tick2.

Meanwhile, I added a vtest to show the two cases that are wrong. Attempts to use findCRinStaff() with an appropriate tick value can fix one but not the other, depending on what tick you pass in.

image

@dmitrio95

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

OK, thanks for the explanation! Let's use this solution then.

@dmitrio95 dmitrio95 merged commit 3b29956 into musescore:master Apr 26, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.