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

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

More exactly, after further checking, minimal conditions are: two voices and at least a slur in Voice 2.
Like this:

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++

thanks cadiz for reporting...and here is his minimal test case example...I'll add it as a test script...

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()

and just to be clear, the desired behavior would be for a chord on each beat, where only the E->F is slurred, but no slur on the C->D.

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:

Or even case of quarter rests in voice 1:

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.

Doesn't matter if have slurs on both voices:

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:

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:

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

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...

Imploding voices doesn't add anything to _updateState._deleteList, which would explain why spanner doesn't get deleted.

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).

I ended up doing the "simple fix" of simply deleting orphaned spanners: https://github.com/musescore/MuseScore/pull/4522

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

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

Turns out that #174111: Implode fails if there are rests in voice 1 of destination staff explains why that final measure won't implode...basically is a temporary solution to deliberately avoid imploding into rests in the first voice...but maybe in future someone will allow that ability.

see also #281175: Removing rests attached to slurs in a voice >1 causes a crash

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...

Fixed in branch master, commit 7eb067c2b4

Merge pull request #4522 from ericfont/280817-implode-staff-delete-spanner

fix #280817 staff implode delete orphaned spanners

Automatically closed -- issue fixed for 2 weeks with no activity.