-
Notifications
You must be signed in to change notification settings - Fork 2.9k
[Draft] Fix #27806: fix changing voices when tuplets are involved #29793
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
base: master
Are you sure you want to change the base?
[Draft] Fix #27806: fix changing voices when tuplets are involved #29793
Conversation
1b62e53
to
bfba88c
Compare
46eba86
to
73f0655
Compare
It's looking good! Certainly an improvement from nightly. I hope repro steps in the video is easy to follow, let me know if you need more info video1805664819.mp4 |
@zacjansheski nice catch! Actually the corruption on your video happens when you delete those notes. Maybe I've missed some part of logic related to working with two voices or just multiple chords simultaneously. I'll check it 🙂 |
ok, fixed that and couple of other stuff around deletions I'll prepare a full list of what might be affected by my changes, it turned out to be a scary chunk of work Little touch beyond all that logic: now it respects base tuplet len (quarters in this case) instead of filling it with larger rests. Though I'm not sure if it's reasonable to glue it to this pull request or it would be better to open another one specifically for this. Screen.Recording.2025-09-13.at.2.21.26.AM.mov |
183825d
to
4c98867
Compare
Note here: this changes expected behavior in engraving tests, but I couldn't find clear documentation on how to update those tests correctly. If I save my project as .mscz and then extract .mscx from it - after running tests it changes in terms of scores. Another note: for now I've reverted that "little touch" as it changes behavior of even more tests |
@cbjeukendrup do I need to ping somebody here again? It's ready for testing from my side, there is one test that need to be updated (and I don't know how to do it the right way), and after that it'll need a quick cleanup |
@prodoelmit The easiest way to update unit test references files is by temporarily making the following change: diff --git a/src/engraving/tests/utils/scorecomp.cpp b/src/engraving/tests/utils/scorecomp.cpp
index 49ed865a4b..f27e173bc3 100644
--- a/src/engraving/tests/utils/scorecomp.cpp
+++ b/src/engraving/tests/utils/scorecomp.cpp
@@ -32,7 +32,7 @@ using namespace mu::engraving;
bool ScoreComp::saveCompareScore(Score* score, const String& saveName, const String& compareWithLocalPath)
{
- if (!ScoreRW::saveScore(score, saveName)) {
+ if (!ScoreRW::saveScore(score, ScoreRW::rootPath() + u"/" + compareWithLocalPath)) {
return false;
}
bool val = compareFiles(ScoreRW::rootPath() + u"/" + compareWithLocalPath, saveName); And then run the unit tests. Then commit the changed reference files if the changes look good. It might take a few days until we're available again for testing, because right now the 4.6 release is the priority. |
|
||
Fraction DurationElement::totalTupletRatio() const | ||
{ | ||
return tuplet() ? tuplet()->totalRatio() : Fraction(1, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return tuplet() ? tuplet()->totalTupletRatio() * tuplet->ratio() : Fraction(1, 1);
Maybe use recursion instead here, to avoid an extra method declared for tuplets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll see how it would look when used - I have a feeling that it might be a bit confusing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, actually not bad, though I couldn't do it in one method - had to make it virtual+override
src/engraving/dom/tuplet.h
Outdated
const Fraction totalRatio() const | ||
{ | ||
Fraction totalRatio = ratio(); | ||
const Tuplet* t = this; | ||
while ((t = t->tuplet())) | ||
{ | ||
totalRatio *= t->ratio(); | ||
} | ||
return totalRatio; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still uncleaned, do not merge * Unified working with global tuplet ratio
4c98867
to
7e279a5
Compare
THIS IS DRAFT, DO NOT PULL
Resolves: #27806
Changes how durations are calculated when moving from/to tuplets.
Updates both chord ticks and duration-type when moving between voices.
Changes
makeGap
logic - it now expects global ticks as parameter, returns global ticksUpdates some of functions (but not all yet) that use makeGap