Fix corruption issues on old files #20230
Conversation
mike-spa
force-pushed
the
fixCorruptionIssuesOnOldFiles
branch
from
367db39
to
cb7f2db
Compare
|
The changes to the unit test scores reverts them back to what they looked like in Mu3. Not sure what that says about those tests though, no idea why they haed been change for Mu4 rather than triggering more investigations into the "why"? |
backport of musescore#20230, as far as it makes sense
src/engraving/rw/read114/read114.cpp
Outdated
| Rest* rest = Factory::createRest(segment2); | ||
| Rest* rest = Factory::createRest(ctx.score()->dummy()->segment()); | ||
| rest->setDurationType(DurationType::V_MEASURE); | ||
| rest->setTicks(m->timesig() / timeStretch); | ||
| rest->setTrack(ctx.track()); | ||
| readRest(m, rest, e, ctx); | ||
| if (!rest->segment()) { | ||
| rest->setParent(segment2); | ||
| } | ||
|
|
||
| Segment* segment2 = m->getSegment(SegmentType::ChordRest, ctx.tick()); | ||
| rest->setParent(segment2); |
There was a problem hiding this comment.
I don't fully understand this change yet. So in the old situation, a rest was created with the segment specified as the parent; but then m_isParentExplicitlySet is still false, so explicitParent returns nullptr, and thus segment() too; and thus the parent does get set explicitly in the old line 1751.
And in the new situation, actually exactly the same thing happens, except that the initial parent (which doesn't really count anyway) is the dummy segment.
(I'm not saying I don't like the change, I'm just wondering about is since I still don't fully understand it 🙂)
Also, from the lines below, segment2 = rest->segment(); looks a bit unnecessary now; and inside readRest there is also some code that looks suspiciously similar to the code you modified, so I wonder if that would need to be adjusted too.
There was a problem hiding this comment.
On old files, the way that the second voice was written was not by opening a new <voice> tag, like we do now, but by using a <tick> tag to "rewind" the time. As in: write the first voice for the whole measure, then rewind the tick to the beginning of the measure, then write the second voice. The problem is that the <tick> tag which signals the rewind is written inside the <rest> and therefore read as part of readRest(), but segment2 was created earlier, when the tick was still at the end of the measure. As a result, the rest was being created at the end of the measure instead of the start.
The end of the story is: we shouldn't perform m->getSegment(...) until we have read the rest, hence why I'm now creating it with a dummy segment and setting the true segment later.
There was a problem hiding this comment.
Also, from the lines below,
segment2 = rest->segment();looks a bit unnecessary now
True, corrected it now.
There was a problem hiding this comment.
Aah, that makes a whole lot of sense! :)
backport of musescore#20230, as far as it makes sense
Not actually related to the corruption, but still a clear error: as it was, the method was always returning true.
unknown() is already called by the caller of this function where necessary, it shouldn't be called here.
This was corrupting entire scores because it was setting full-measure rests to wrong durations. At this stage timeSigMap isn't computed yet, so it can't be used. The case of full-measure rests seems to be correctly managed anyway, without needing this check.
mike-spa
force-pushed
the
fixCorruptionIssuesOnOldFiles
branch
from
cb7f2db
to
2678c54
Compare
Resolves: #17940 *
Resolves: #17752
Shoutout to @sammik for figuring out the true cause of the corruption and logging the clearest issue.
* (corruptions will still be reported in this file, but these are identical to those that 3.6 reports; it will now actually open and display)