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

Lyrics revamp and new options #22434

Merged
merged 6 commits into from
May 15, 2024

Conversation

mike-spa
Copy link
Contributor

Resolves: #22432
Resolves: #21166
Resolves: #21045

#22181 Should be merged before this.

@mike-spa mike-spa force-pushed the lyricsRevampAndNewOptions branch 2 times, most recently from 9d88bb5 to 0dc7dae Compare April 17, 2024 15:27
@@ -509,6 +489,16 @@ void Lyrics::triggerLayout() const
}
}

double Lyrics::yRelativeToStaff() const
{
return pos().y() + chordRest()->pos().y();
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it return multdata()->posY() instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it should, but because of this issue, I need this function to return the position including offset, hence why I need to use pos() instead of mutldata()->pos()

@@ -4543,6 +4543,28 @@ ChordRest* Score::findCRinStaff(const Fraction& tick, staff_idx_t staffIdx) cons
return 0;
}

ChordRest* Score::findChordRestEndingBeforeTickInTrack(const Fraction& tick, track_idx_t trackIdx) const
{
Measure* measure = tick2measureMM(tick - Fraction::eps());
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please explain why we need to subtract eps() here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because when tick falls exactly in-between two measures (meaning that one measure ends at tick and the next one starts at tick) tick2Measure returns the next one (which is what we want in 99% of cases). I'm subtracting eps() to make sure that I get the previous one in this case

if (!segment->isChordRestType() || !segment->elementAt(trackIdx)) {
continue;
}
ChordRest* chordRest = toChordRest(segment->elementAt(trackIdx));
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to call elemenetAt only once

continue;
}
ChordRest* chordRest = toChordRest(segment->elementAt(trackIdx));
chordRest->actualTicks().toString();
Copy link
Contributor

Choose a reason for hiding this comment

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

Unused

return nullptr;
}

for (Segment* segment = measure->last(); segment; segment = segment->prev()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

const Segment*

{
Shape& highResShape = mutldata()->highResShape.mut_value();
highResShape.elements().reserve(m_text.size());
highResShape.clear();
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to call clear() before reserve()

}

void LyricsLayout::setDefaultPositions(staff_idx_t staffIdx, LyricsVersesMap& lyricsVersesAbove,
LyricsVersesMap& lyricsVersesBelow,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make these maps const here and elsewhere?

LyricsVersesMap& lyricsVersesBelow)
{
double systemX = system->pageX();
// HACK: subtract minVerticalDistance here because it's added later during staff distance calculations. Needs a better solution.
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you going to replace it with a better solution? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but the better solution means rewriting all the vertical spacing system, so I have to keep this for now :)

return lyricsSkyline;
}

void LyricsLayout::moveThisVerseAndOuterOnes(int verse, int lastVerse, bool above, double diff, LyricsVersesMap lyricsVerses)
Copy link
Contributor

Choose a reason for hiding this comment

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

const LyricsVersesMap&

@avvvvve
Copy link

avvvvve commented May 13, 2024

@mike-spa It's not possible to scroll with my touchpad inside the second column of the text styles page. Maybe a side effect of adding the anchor link to that page?

image

@mike-spa
Copy link
Contributor Author

@mike-spa It's not possible to scroll with my touchpad inside the second column of the text styles page. Maybe a side effect of adding the anchor link to that page?

Can you check if you have the same problem on the current master? I would be very weird if that caused it

@avvvvve
Copy link

avvvvve commented May 14, 2024

@mike-spa just checked and yes, it's in the 4.4 master so never mind :)

Rewrite layout of melisma lines

Rewrite layout of lyrics dashes

Code corrections

Add to skyline

Code cleanup

Introduce new lyrics dash and melisma options

Implement min melisma length option

Implement always force melisma option

Implement lyrics dash options at start of system

Implement dash option on first note of the system

Remove all lyrics vertical positioning code

more

Rewrite lyrics vertical positioning


Refactoring

Cleanup unused code in stable layout

Add padding on the outside of lyrics

Introduce highRes shape for text base items


Correction
@oktophonie oktophonie added the vtests This PR produces approved changes to vtest results label May 14, 2024
@RomanPudashkin RomanPudashkin merged commit fdf6726 into musescore:master May 15, 2024
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vtests This PR produces approved changes to vtest results
Projects
None yet
4 participants