Design issue: Notes in a voice cannot be placed on different staves (using System)

[rvilarl]

This is not allowed by VexFlow and it seems that the solution is to make two voices out of one adding GhostNotes. However this is an impediment to have slurs or beams between staves (see #434)

I am collaborating with @jaredjj3 on https://github.com/stringsync/vexml. This is the application that I am using to produce the jsfiddle examples in the OSMD issues from musicxml files.

There I am facing currently this design issue with test_clef_end_measure.xml from OSMD

@AaronDavidNewman once vexml is more mature we will be able to generate real scores for testing. As you commented once it would be good to have more real score testing. Or as @mscuthbert did use http://lilypond.org/doc/v2.20/input/regression/musicxml/collated-files.html

@0xfe Do we want to make a design change to allow notes from one voice on different staves?

[rvilarl]

The problem is Systemwithout this class it is possible to let voices jump between staves. It is even possible to have cross staff beams although they do not loo very nice (beam is not located in the middle).

https://jsfiddle.net/exqncky4/2/

Reference in http://lilypond.org/doc/v2.20/input/regression/musicxml/collated-files.html

[AaronDavidNewman]

@rvilarl I went over the PR 1409 changes. I have a question and a suggestion:

1. what does the changed code in formatter.ts do? I removed it and didn't see a change in the cross-beam or middle-beam behavior. So maybe it is for that other articulation/beaming thing? If it is required maybe that can be a different PR.
2. It seems like the only change we need to do to get voice on multiple staves is call setStave on the notes after the stave Y are calculated. I don't think this does any harm even if the system is not cross-stave. And if the tickable doesn't have a stave yet, we just use the part stave.

addVoiceToPart isn't technically needed but it seems to make adding voices to a system after the fact a little less awkward.

  addVoiceToPart(voices: Voice[], part: number) {
    const ctx = this.getContext();
    voices.forEach((voice) => {
      voice.setContext(ctx);
    });
    this.parts[part].voices = this.parts[part].voices.concat(voices);
  }

  format(): void {
...
 // Update the start position of all staves.
    this.parts.forEach((part) => {
      part.stave.setNoteStartX(startX);
      part.voices.forEach((voice) => {
        voice.getTickables().forEach((tickable) => {
          const stave = tickable.getStave() ?? part.stave;
          tickable.setStave(stave);
        });
      });
    });

and one small change to formatter.ts, since some of the parts in system may have an empty voice:

  joinVoices(voices: Voice[]): this {
    if (voices.length < 1) {
      return this;
    }
...
  }

Also, we've kind of been stymied by this code at the end of system with the bounding box. It doesn't see that it is used anywhere, and it was never correct. How about if we just remove it? I did and didn't see that it affected anything. It is not even the bounding box of the system, since that would have to include the stems and beams etc.

    // this.boundingBox = new BoundingBox(this.startX, this.lastY, justifyWidth, this.lastY - this.options.y);
    Stave.formatBegModifiers(allStaves);
  }

Making just these changes, I was able to get the same output we had from the changes in the PR.

Let me know what you think if this.

[rvilarl]

@rvilarl I went over the PR 1409 changes. I have a question and a suggestion:

1. what does the changed code in formatter.ts do? I removed it and didn't see a change in the cross-beam or middlebeam behavior. So maybe it is for that other articulation/beaming thing? If it is required maybe that can be a different PR.

You are right this a bug that I found during the change and it was not a good idea to include it. I have already started with the new PR. Issue #1427. Thanks for your feedback on the test! I will make a PR now with the formatter changes to resolve the issue.

2. It seems like the only change we need to do to get voice on multiple staves is call setStave on the notes after the stave Y are calculated. I don't think this does any harm even if the system is not cross-stave. And if the tickable doesn't have a stave yet, we just use the part stave

This is right, it is possible to do cross staving without using System, but I want to make System cross stave aware in order to avoid special cases. I am using System in the XML Test Suite compliance check that I am building with @jaredjj3 in https://github.com/stringsync/vexml.

There are also changes to allow the usage of System without specifying the width. I will make another PR for this one.

addVoiceToPart isn't technically needed but it seems to make adding voices to a system after the fact a little less awkward.

  addVoiceToPart(voices: Voice[], part: number) {
    const ctx = this.getContext();
    voices.forEach((voice) => {
      voice.setContext(ctx);
    });
    this.parts[part].voices = this.parts[part].voices.concat(voices);
  }

  format(): void {
...
 // Update the start position of all staves.
    this.parts.forEach((part) => {
      part.stave.setNoteStartX(startX);
      part.voices.forEach((voice) => {
        voice.getTickables().forEach((tickable) => {
          const stave = tickable.getStave() ?? part.stave;
          tickable.setStave(stave);
        });
      });
    });

and one small change to formatter.ts, since some of the parts in system may have an empty voice:

  joinVoices(voices: Voice[]): this {
    if (voices.length < 1) {
      return this;
    }
...
  }

Also, we've kind of been stymied by this code at the end of system with the bounding box. It doesn't see that it is used anywhere, and it was never correct. How about if we just remove it? I did and didn't see that it affected anything. It is not even the bounding box of the system, since that would have to include the stems and beams etc.

    // this.boundingBox = new BoundingBox(this.startX, this.lastY, justifyWidth, this.lastY - this.options.y);
    Stave.formatBegModifiers(allStaves);
  }

Making just these changes, I was able to get the same output we had from the changes in the PR.

Let me know what you think if this.

I will get back to you with my thoughts.