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

[MU3 Backend] ENG-34: Fix erroneous space created for melismas #8176

Merged

Conversation

iveshenry18
Copy link
Contributor

@iveshenry18 iveshenry18 commented May 26, 2021

(Partially) Resolves: ENG-34: Improve lyrics spacing

Previously, melismas were treated the same as normal Lyrics in terms of
horizontal spacing—they would cause a spacer to be added to the
ChordRest to which they were attached, causing an unnecessary gap after
the first note of a melisma.

This commit prevents a melisma from adding a right spacer to
their first ChordRest, and rather creates a right
spacer on the last ChordRest of a lyric if necessary.

In pursuit of this, this commit refactors ChordRest::_melismaEnd(s) from
a bool to a std::set<*Lyrics>, giving a ChordRest a way to access the
melismatic Lyrics that end on it. This change also solves the problem of
said bool being untrustworthy (i.e. erroneously being changed to false
when one of multiple melismas was removed).

  • I signed CLA
  • I made sure the code in the PR follows the coding rules
  • I made sure the code compiles on my machine
  • I made sure there are no unnecessary changes in the code
  • I made sure the title of the PR reflects the core meaning of the issue you are solving
  • I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"
  • I made sure the commit message title starts with "fix #424242:" if there is a related issue
  • I created the test (mtest, vtest, script test) to verify the changes I made

@iveshenry18 iveshenry18 changed the title ENG-34: Fix erroneous space created for melismas [MU3 Backend] ENG-34: Fix erroneous space created for melismas May 26, 2021
@RobFog
Copy link

RobFog commented Jun 8, 2021

And another one for @lvinken 😉

//---------------------------------------------------------

void ChordRest::setMelismaEnd(bool v)
bool ChordRest::removeMelismaEnd(const Lyrics* l)
Copy link
Member

Choose a reason for hiding this comment

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

just use the combination of "std::set<Key,Compare,Allocator>::find" and "std::set<Key,Compare,Allocator>::erase"

I'm also not sure if whether it makes sense to store it in std::set, what was the point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call; I honestly can't recall why I made this so overly complex.

The reason I'm storing the values is this: previously there was a _melismaEnd boolean that stored whether a ChordRest had the end of a melisma, but this actually was unreliable if there were multiple lyrics. Additionally, this spacing fix required that a ChordRest have pointers to any lyrics/melismas that end on it in order to create a properly-sized spacer. Since I needed to store those pointers and there already was an existing melisma-end data member, I basically made a drop-in replacement for the existing _melismaEnd functions that stored all pointers (realistically, this will be ~0–3 values) rather than just setting a boolean.

I chose a std::set to avoid redundancy (since I didn't write the original code where they get added); although possibly an unordered_set would be quicker, or some other Qt container. Does that make sense? Let me know if I'm missing something

Copy link
Member

Choose a reason for hiding this comment

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

ah, sorry, missed that one in my emails

prefer to use std::containers wherever it's possible within the libmscore
regarding your case, I think it's enough to stick with std::set then

@iveshenry18 iveshenry18 force-pushed the ENG-34-lyric-spacing branch 2 times, most recently from 5db1c61 to 70bacdc Compare June 23, 2021 22:32
This updates the previous ENG-34 commit by simplifying the
removeMelismaEnds function.
@vpereverzev vpereverzev merged commit 13770e4 into musescore:3.6.2_backend Jul 5, 2021
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jul 6, 2021
This updates the previous ENG-34 commit by simplifying the
removeMelismaEnds function.

Duplicate of musescore#8176
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jul 6, 2021
This updates the previous ENG-34 commit by simplifying the
removeMelismaEnds function.

Duplicate of musescore#8176
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jul 6, 2021
This updates the previous ENG-34 commit by simplifying the
removeMelismaEnds function.

Duplicate of musescore#8176
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jul 6, 2021
This updates the previous ENG-34 commit by simplifying the
removeMelismaEnds function.

Duplicate of musescore#8176
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jul 13, 2021
This updates the previous ENG-34 commit by simplifying the
removeMelismaEnds function.

Duplicate of musescore#8176
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jul 27, 2021
This updates the previous ENG-34 commit by simplifying the
removeMelismaEnds function.

Duplicate of musescore#8176
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jul 27, 2021
This updates the previous ENG-34 commit by simplifying the
removeMelismaEnds function.

Duplicate of musescore#8176
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jul 27, 2021
This updates the previous ENG-34 commit by simplifying the
removeMelismaEnds function.

Duplicate of musescore#8176
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jul 27, 2021
This updates the previous ENG-34 commit by simplifying the
removeMelismaEnds function.

Duplicate of musescore#8176
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jul 27, 2021
This updates the previous ENG-34 commit by simplifying the
removeMelismaEnds function.

Duplicate of musescore#8176
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jul 27, 2021
This updates the previous ENG-34 commit by simplifying the
removeMelismaEnds function.

Duplicate of musescore#8176
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jul 28, 2021
This updates the previous ENG-34 commit by simplifying the
removeMelismaEnds function.

Duplicate of musescore#8176
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jul 28, 2021
This updates the previous ENG-34 commit by simplifying the
removeMelismaEnds function.

Duplicate of musescore#8176
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 13, 2021
This updates the previous ENG-34 commit by simplifying the
removeMelismaEnds function.

Duplicate of musescore#8176
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 13, 2021
This updates the previous ENG-34 commit by simplifying the
removeMelismaEnds function.

Duplicate of musescore#8176
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 19, 2021
This updates the previous ENG-34 commit by simplifying the
removeMelismaEnds function.

Duplicate of musescore#8176
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 19, 2021
This updates the previous ENG-34 commit by simplifying the
removeMelismaEnds function.

Duplicate of musescore#8176
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 30, 2021
This updates the previous ENG-34 commit by simplifying the
removeMelismaEnds function.

Duplicate of musescore#8176
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Sep 1, 2021
This updates the previous ENG-34 commit by simplifying the
removeMelismaEnds function.

Duplicate of musescore#8176
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Sep 2, 2021
This updates the previous ENG-34 commit by simplifying the
removeMelismaEnds function.

Duplicate of musescore#8176
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Sep 10, 2021
This updates the previous ENG-34 commit by simplifying the
removeMelismaEnds function.

Duplicate of musescore#8176
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Sep 23, 2021
This updates the previous ENG-34 commit by simplifying the
removeMelismaEnds function.

Duplicate of musescore#8176
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Mar 5, 2023
This updates the previous ENG-34 commit by simplifying the
removeMelismaEnds function.

Duplicate of musescore#8176
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants