-
-
Notifications
You must be signed in to change notification settings - Fork 3
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
Fix GH#359: Crash on Voice2-4 expanding into new measure #364
Fix GH#359: Crash on Voice2-4 expanding into new measure #364
Conversation
expandVoice(seg, cr1->track()); | ||
} | ||
} | ||
} | ||
makeGap(cr1->segment(), cr1->track(), f2, tuplet, first); |
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.
looks good, this is exactly the place it crashes
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.
Any idea why this doesn't seem needed in master?
Hm, okay.. into the next-next measure. Originally it used to happen even without going into the next measure when increasing the duration like this (this is 3.6.2:) screencast2.mp4I'll check it out later tomorrow. I think if I were to just reset the boolean i put in there afterwards, it'd be okay but that's just a quick guess. |
dd4feb2
to
00595b7
Compare
I threw in a reset of the expand boolean test. Try it out. For me it's sleep time. |
Good night! |
Yep, that fixes it. diff --git a/libmscore/cmd.cpp b/libmscore/cmd.cpp
index 5eb6c9ecff..35eee14b12 100644
--- a/libmscore/cmd.cpp
+++ b/libmscore/cmd.cpp
@@ -1384,8 +1384,17 @@ void Score::changeCRlen(ChordRest* cr, const Fraction& dstF, bool fillWithRest)
Chord* oc = 0;
bool first = true;
+ bool expand = ((cr->tick() + dstF) > Fraction(1, 1));
for (Fraction f2 : qAsConst(flist)) {
f -= f2;
+ if (expand) {
+ if (auto nm = cr1->measure()->nextMeasure()) {
+ if (auto seg = nm->first(SegmentType::ChordRest)) {
+ expandVoice(seg, cr1->track());
+ }
+ }
+ expand = false;
+ }
makeGap(cr1->segment(), cr1->track(), f2, tuplet, first);
if (cr->isRest()) { The following does not work though: diff --git a/libmscore/cmd.cpp b/libmscore/cmd.cpp
index 5eb6c9ecff..4cf6313442 100644
--- a/libmscore/cmd.cpp
+++ b/libmscore/cmd.cpp
@@ -1386,6 +1386,14 @@ void Score::changeCRlen(ChordRest* cr, const Fraction& dstF, bool fillWithRest)
bool first = true;
for (Fraction f2 : qAsConst(flist)) {
f -= f2;
+ bool expand = ((cr1->tick() + f) > Fraction(1, 1));
+ if (expand) {
+ if (auto nm = cr1->measure()->nextMeasure()) {
+ if (auto seg = nm->first(SegmentType::ChordRest)) {
+ expandVoice(seg, cr1->track());
+ }
+ }
+ }
makeGap(cr1->segment(), cr1->track(), f2, tuplet, first);
if (cr->isRest()) { |
00595b7
to
51df4b4
Compare
Checking in: |
51df4b4
to
a726261
Compare
Unfortunately this crashes when prolonging a non-voice 1 note across more than 1 measure, like a quarter on beat four (in 4/4 time sig) to a longa (8) or maxima (9) |
Damn. |
Maybe add the check from #359 (comment) ? |
I'll try a few things in a moment shortly |
Looks like the code needs to check for needing expansion on every loop,... which is worse than just verifying an invalid CR... |
a726261
to
0d777fd
Compare
Demonstration: it should be okay. Test ? ;) screencast2.mp4 |
@@ -1465,7 +1477,8 @@ void Score::changeCRlen(ChordRest* cr, const Fraction& dstF, bool fillWithRest) | |||
if (m1 == 0) | |||
break; | |||
Segment* s = m1->first(SegmentType::ChordRest); | |||
cr1 = toChordRest(s->element(track)); | |||
if (!(cr1 = toChordRest(s->element(track)))) | |||
break; |
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.
Might be worth an assert, like "this shouldn't happen, ever"?
Looks good. But how about this: diff --git a/libmscore/cmd.cpp b/libmscore/cmd.cpp
index 5eb6c9ecff..8522c14fa0 100644
--- a/libmscore/cmd.cpp
+++ b/libmscore/cmd.cpp
@@ -1384,8 +1384,17 @@ void Score::changeCRlen(ChordRest* cr, const Fraction& dstF, bool fillWithRest)
Chord* oc = 0;
bool first = true;
+ Fraction totalLen = cr->rtick() + f;
for (Fraction f2 : qAsConst(flist)) {
f -= f2;
+ if (totalLen.reduced() > Fraction(1, 1)) {
+ if (auto nm = cr1->measure()->nextMeasure()) {
+ if (auto seg = nm->first(SegmentType::ChordRest)) {
+ expandVoice(seg, track);
+ totalLen.setNumerator(totalLen.numerator() - totalLen.denominator());
+ }
+ }
+ }
makeGap(cr1->segment(), cr1->track(), f2, tuplet, first);
if (cr->isRest()) {
@@ -1462,10 +1471,12 @@ void Score::changeCRlen(ChordRest* cr, const Fraction& dstF, bool fillWithRest)
}
Measure* m = cr1->measure();
Measure* m1 = m->nextMeasure();
- if (m1 == 0)
+ if (!m1)
break;
Segment* s = m1->first(SegmentType::ChordRest);
cr1 = toChordRest(s->element(track));
+ if (!cr1)
+ break;
}
connectTies();
} (not sure about whether to have an assert at the end, probably not, it seems it will happen) |
I'd leave the assert out, but your changes are fine... it's the same thing. Hell, the compiler will probably optimize the stuff anyways to be the exact same anyways. In other words, "Six one way, half-a-dozen the other". |
0d777fd
to
b38a53e
Compare
It's working perfectly now. Thank you. |
Resolves: #359
Instead of only reverting the change, I tried to get the same "intention" as the 4.x branch by way of seeing if the resulting "total fraction" was going to spill over into the next measure. If so, then perform the expand function (it was crashing because 3.x apparently needs to expand there when extending past current measure). Putting it back in how it was would cause unnecessary rests though every time. Test please if you're willing...
I have a screen cast showing how it doesn't make a voice-2 rest for no reason and also that it doesn't crash on my side over here if it "spills over".
screencast.mp4