Skip to content
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 #297957: Ties extended in region after time signature change #5506

Merged
merged 1 commit into from
Dec 3, 2019

Conversation

mattmcclinch
Copy link
Contributor

Resolves: https://musescore.org/en/node/297957.

When searching for a note to complete a tie, what matters is not so much that the note doesn't already have a tieBack(), but rather that the two notes have the same unisonIndex().

@bryanhanner
Copy link

@mattmcclinch noob question: why is the unisonIndex relevant here? It seems like we wouldn't care about where the notes are in each chord for the tie.

@mattmcclinch
Copy link
Contributor Author

It goes back to my original purpose for changing this line in #5275. Writing it this way allows the right notes to be matched up, even if there previously was a tie.

Resolves: https://musescore.org/en/node/297957.

When searching for a note to complete a tie, what matters is not so much that the note doesn't already have a tieBack(), but rather that the two notes have the same unisonIndex().
@mattmcclinch
Copy link
Contributor Author

I think I was originally going to compare unisonIndex() values in searchTieNote() when I made #5275, but it just seemed so inefficient. I figured I could simply check if n->tieBack() was not null, but it turns out that was wrong. But note->unisonIndex() only needs to be called once, and the unisonIndex for n can be computed without having to actually call the function, since we are already looping through the notes in n->chord().

@anatoly-os anatoly-os added this to the Musescore 3.3.4 milestone Dec 2, 2019
@dmitrio95
Copy link
Contributor

With this fix it becomes possible to tie two notes in one chord to one note in another. See the following:

  1. Input two tied notes (say, A pitch).
  2. In the first chord add a note above the existing note (for example, B), move it down to A with arrow keys.
  3. In the second chord add a note below the existing note (G), move it up to A with arrow keys.
  4. Select the untied note in the first chord, press +.
    Result: two notes are tied to one. You can now repitch any of the tied notes and obtain an even more unusual ties configuration like this one:
    Screenshot_20191202_140519
    I wasn't able to produce any crash or other serious issue from that but that is still something that would be good to avoid. At first glance I see the following options of how could this be avoided:
  • check both unison index and tieBack presence;
  • alter the algorithm of sorting notes in chords to handle adding unisons in a more consistent way;
  • fix cmdAddTie/cmdToggleTie to check the presence of backward tie in the target note before adding a new tie. In this case though the issue may appear in some other place that uses searchTieNote().

@mattmcclinch
Copy link
Contributor Author

It seems to me that option 2 would be the right approach. Checking for tieBack presence is what caused this issue in the first place. I don't see how checking both unison index and tieBack is going to help. What we want to do is make sure that two tied notes always have the same unison index after operations that can can cause this value to change (such as adding a note to the chord, removing a note from the chord, or moving a note within the chord).

@dmitrio95
Copy link
Contributor

I agree that this would be a good way to resolve the issue. Would you implement this approach for this PR? We are planning to finalize a 3.3.4 release tomorrow so if you don't have an opportunity to do that before that time I could try implementing it myself.

@dmitrio95 dmitrio95 merged commit 116f3eb into musescore:master Dec 3, 2019
dmitrio95 added a commit that referenced this pull request Dec 3, 2019
Fixup to f6998a4: needed to avoid
a possibility to tie two different notes to one.
See #5506 (comment)
@dmitrio95
Copy link
Contributor

I pushed 2d140a3, as far as I understand, this is what is needed to avoid this incorrect ties configuration issues, at least in most common cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants