Implode tool crashes if two voices and a slur in Voice 2

• Dec 28, 2018 - 18:00
Reported version
3.0
Priority
P0 - Critical
Type
Functional
Frequency
Once
Severity
S2 - Critical
Reproducibility
Always
Status
closed
Regression
No
Workaround
No
Project

Musescore crashes when using implode on a staff. Reproducible on the attached score by simply selecting the top staff and using the Implode tool.

OS: Arch Linux, Arch.: x86_64, MuseScore version (64-bit): 3.0.0., revision: 3543170

Attachment Size
sclMxlEm.mscz 14.5 KB

Comments

Title Crash when using Implode tool Implode tool crashes if notes are in two voices with slurs

Basically, the crash occurs in the following conditions: notes in two voices and with slurs in each voice, as measure 13 in your score.

Sslur.gif

Title Implode tool crashes if notes are in two voices with slurs Implode tool crashes if two voices and a slur in Voice 2

More exactly, after further checking, minimal conditions are: two voices and at least a slur in Voice 2.
Like this:
case1.jpg
If only a slur in Voice 1, no issue.

Stack trace for reference is:

MuseScore3.exe!Ms::ScoreElement::isChord() Line 261 C++
MuseScore3.exe!Ms::Score::layoutSystemElements(Ms::System * system, Ms::LayoutContext & lc) Line 3594   C++
MuseScore3.exe!Ms::Score::collectSystem(Ms::LayoutContext & lc) Line 3435   C++
MuseScore3.exe!Ms::Score::doLayoutRange(int stick, int etick) Line 4190 C++
MuseScore3.exe!Ms::Score::update() Line 220 C++
MuseScore3.exe!Ms::Score::endCmd(bool rollback) Line 180    C++
MuseScore3.exe!Ms::Score::cmd(const QAction * a, Ms::EditData & ed) Line 3694   C++
MuseScore3.exe!Ms::ScoreView::cmd(const char * s) Line 2314 C++
MuseScore3.exe!Ms::ScoreView::cmd(const QAction * a) Line 1761  C++
MuseScore3.exe!Ms::MuseScore::cmd(QAction * a, const QString & cmd) Line 6087   C++
MuseScore3.exe!Ms::MuseScore::cmd(QAction * a) Line 5578    C++
MuseScore3.exe!Ms::MuseScore::qt_static_metacall(QObject * _o, QMetaObject::Call _c, int _id, void * * _a) Line 850 C++

During Score::layoutSystemElements(), the crash occurs on dereferencing the null scr:

      for (auto s : spanner) {
            Slur* slur = toSlur(s);
            ChordRest* scr = s->startCR();
            ChordRest* ecr = s->endCR();
            if (scr->isChord())

fix probably isn't as simple as checking for non-null before dereferencing....because the bigger question is why would the spanner have a null startCR & endCR...cleary it isn't much of a spanner if it doesn't have these endpoints....

so I'm going to look at the actual cmd for implode as it gets processed when called in MuseScore::cmd()

I'm wondering why in cmdImplode() it only checks the 1st voice when determining if there is a tie:

                        // see if we are tying in to this chord
                        Chord* tied = 0;
                        for (Note* n : dstChord->notes()) {
                              if (n->tieBack()) {
                                    tied = n->tieBack()->startNote()->chord();
                                    break;
                                    }
                              }

Why not check other voices too? I don't know. This whole block dealing with ties is skipped if there wasn't a tie in first voice...but surely there could be ties in other voices that would want to be preserved?:

                                          // add tie to this note if original chord was tied
                                          if (tied) {
                                                // find note to tie to
                                                for (Note *tn : tied->notes()) {
                                                      if (nn->pitch() == tn->pitch() && nn->tpc() == tn->tpc() && !tn->tieFor()) {
                                                            // found note to tie
                                                            Tie* tie = new Tie(this);
                                                            tie->setStartNote(tn);
                                                            tie->setEndNote(nn);
                                                            tie->setTrack(tn->track());
                                                            undoAddElement(tie);
                                                            }
                                                      }
                                                }

So not only am I wondering why the current code looks like is it only cares about if there is a tie in the 1st voice when determining if should do handling for ties, but also I'm wonder why there are no checks for slurs? Wouldn't we want to keep slurs if possible? I see how things can be difficult, though...

So the question of why not worry about slurs was bugging me...but turns out the limitation are more than that, as it seems it doesn't even deal with imploding if there is rests in voice one, either case of measure rest in voice 1:

meausre-nothing-voice-1-slur-voice-2.PNG

Or even case of quarter rests in voice 1:

meausre-nothing-voice-1-slur-voice-2-quarter-rests-voice-1.PNG

Without knowing exactly what implode is supposed to do and what it isn't supposed to do...it will be hard to know how I should fix this bug. I would want to try at least to implode the slur, but if that is not what the desired behavior of this feature includes, then the simple answer would be to remove spanners that have been orphaned.

The state of the score's spanners at the end of this function has one slur, with NULL for both _startElement and _endElement. So clearly that slur has been orphaned...the code seems to not care about connecting slurs in voices outside of voice 1.

spanner-at-end-of-implode-function.PNG

Doesn't matter if have slurs on both voices:

slur-both-voices.PNG

Will still crash. That is because the state of score's spanners at the end of the implode command still has the spanner in the second voice orphaned:

after-implode-with-slur-on-both-voices.PNG

Well it seems that current voice implode code basically deletes a glissando in the second voice, so not limited to slurs. But at least the code works.

Well I'm going to keep this fix simple, and just delete spanners if they start or end on voices 2,3,4 of imploded measures.

Actually I give up...because I don't know the best way forward. I'll wait for input form one of the other devs, or better yet if someone else who has a better idea of desired behavior wants to fix this, feel free. Ideally code would "reconnect" spanners whose notes in voices 2,3,4 were moved to voice 1. But unfortunately I don't have enough knowledge of exactly how spanners are dealt with...it seems the individual Note::spannerForward and Note::spannerBack aren't dealing with slurs, which seem to be stored in the Score's top level _spanner. I'm guessing SpannerMap is an efficent datastructure for searching though the list of spanners...I'm guessing use that to find spanners that end or start in the measures being deleting and see if can match. But there are all sorts of corner cases that I worry about such as spanners coming from outside the range in voices 2,3,4 which will then have to be connected to a note in voice 1, which probably violates some unspoken implicit assumption that spanners dont' cross voices. Oh well. I'll see what people say.

just to note...looking at implode for multiple-staves, the clone voices does leave an "orphaned" slur:

after-implode-with-slur-on-both-voices.PNG

Although I should note that detached slur subsequently get reattached in when endCmd(); is executed:

reattached.PNG

I'm guessing Score::deletePostponed() (called by Score::updat() inside Score::endcmd()) is where the actual removal of the orphaned slur is supposed to occur...so maybe it is not properly handling removal for voices...

hmm...it wasn't Score::deletePostponed() that does the reattaching when imploding staves, but rather is s->doLayoutRange(cs.startTick(), cs.endTick()); which does...

hmm...I might be confusing myself some. Part of my getting confused is because imploding staves make copies of the imploded notes, while imploding voices in a single staff rather deletes the imploded notes (except of course it doesn't delete the imploded slur).

FYI, this is the behavior with a slur hell test...basically deletes all spanners not in voice 1:

implode-slur-hell.gif

Now I'm trying to investigate why that final half measure isn't completely being imploded...

Attachment Size
slur-hell.mscz 9.48 KB
Status PR created fixed

Fixed in branch master, commit 7dd7c617b0

fix #280817 staff implode delete orphaned spanners

Previously, imploding a staff would leave orphaned spanners after the notes they connected to were deleted when moving those notes to the first voice. These orphaned spanners would cause a crash during layout when the Implode command finished.

This is a simple fix to avoid a crash by deleting these orphaned spanners.

An ideal solution would be to somehow reconnect these orphaned spanners to the new version of their original notes after they move to the first 1st voice. But that solution would require a lot more thought to get correct, and would also require special consideration for various corner cases, such as what happens with spanners who have one end connected to notes outside the imploded range but the other end connected to a note inside the range...

Fix version
3.0.1