Fix cursor issues

[jaredjj3]

Here it is!

Two issues

• the cursor does not cover the two instruments
• there seems to be only one active element

Two questions

• how do I move the cursor to the next position?
• is it possible to have more than one cursor?

Feedback to README

• vexml.renderMXL returns a Promise<vexml.Score>

• document.getElementById does not return HTMLDivElement

  I had to do the following:

vexml.renderMXL(file, div as HTMLDivElement).then((score) => {
    this.score = score;
}

Originally posted by @rvilarl in #260

[jaredjj3]

Breadcrumbs: I'm using the same document from the comment. I reproduced the issue by rendering the document and inspecting the elements.Score on the dev site.

So far I see two issues:

• activeElements always has a single rest
• activeElements doesn't include anchorElement, but I think it should since anchorElement is the active element that caused the sequence entry to be created

Next steps:

• Check to see if the elements.Rest is the same object throughout the playback.Sequence.
• Ensure anchorElement is included in activeElements.

[jaredjj3]

Check to see if the elements.Rest is the same object throughout the playback.Sequence.

It is the same element:

Meanwhile, the anchorElement does seem to be changing correctly:

I believe the problem is in playback.SequenceFactory.

[jaredjj3]

Ensure anchorElement is included in activeElements.

When addressing this, I realized that this issue was caused by how the anchor was being set here:

vexml/src/playback/sequencefactory.ts

Line 210 in bebff72

this.anchor = event.element;

Instead of just setting the anchor, we need to:

1. Remove the previous anchor from the active elements.
2. Update the current anchor.
3. Add the current anchor to the active elements.

Luckily, this is a simple fix. I'll write a PR that does this.

edit: This logic is not completely correct. We shouldn't unconditionally remove the previous anchor when setting a new one. It's possible for a new anchor to show up while the previous one is still active. For example, when playing piano, the left hand could play a whole note, while the right hand plays eighth notes. The anchor will change for every eighth note, but the whole note should remain active.

[jaredjj3]

I found a bug:

vexml/src/playback/sequencefactory.ts

Line 258 in bebff72

this.entries.push({ durationRange, xRange, anchorElement: anchor, activeElements: active });

When adding a SequenceEntry, I didn't copy the active array. Any mutations on the array affected all the entries.

After fixing that, it kind of works, but I'm still getting unexpected results. I'm going to debug more.

[jaredjj3]

I think I found the other issues.

1. When sorting SequenceEvent (not SequenceEntry) objects, we need to make sure we process 'stop' events before 'start' events:

vexml/src/playback/sequencefactory.ts

Line 111 in bebff72

return events.sort((a, b) => a.time.ms - b.time.ms);

2. We need a new case to handle when x1 === x2 here:

vexml/src/playback/sequencefactory.ts

Lines 323 to 324 in bebff72

	const x1 = previous.rect().center().x;
	const x2 = current.rect().center().x;

It's normal for parts with multiple staves to have voice entries that have the same x value. In this case, we should update the most recent activeElements, and update this.active.

After addressing all of these, activeElements looks correct. I really would like some test coverage here to make this easier to debug/refactor.

[rvilarl]

@jaredjj3 There is another cursor related issue. If you go to https://michaelecke.com/pianoplaynew/ and load MOONLIGHT, PASSACAGLIA or PRELUDE and press start you will get the following error on the console.

[jaredjj3]

Thanks for the heads up. I'll take a look next.

[jaredjj3]

I was able to get PASSACAGLIA into my dev environment. The error is being thrown here:

vexml/src/playback/sequencefactory.ts

Line 187 in 51d66c0

this.processPending(new NumberRange(x1, x2), t1);

[jaredjj3]

I think this.x is getting out-of-sync with this.anchor by the time is gets here on broken documents:

vexml/src/playback/sequencefactory.ts

Line 182 in 51d66c0

const x1 = this.x;

In getXRangeInstruction, we're making decisions based on this.anchor's position, but then we don't directly use it and rely on the system to be synced properly.

[jaredjj3]

For PASSACAGLIA, one of the issues is how the whole rest is rendered. It's centered and purposefully not aligned with the note that starts on the same beat. When computing sequence entries, vexml incorrectly treats them as if they were formatted wrong.

I'm not sure if there are any other issues yet.

[jaredjj3]

I was able to fix PASSACAGLIA by modifying this sort method further

Before:

vexml/src/playback/sequencefactory.ts

Lines 111 to 117 in 51d66c0

	return events.sort((a, b) => {
	// When the times match, we want the stop event to come first.
	if (a.time.ms === b.time.ms) {
	return a.type === 'stop' ? -1 : 1;
	}
	return a.time.ms - b.time.ms;
	});

After:

    return events.sort((a, b) => {
      if (a.time.ms !== b.time.ms) {
        return a.time.ms - b.time.ms;
      }

      if (a.type !== b.type) {
        // Stop events should come before start events.
        return a.type === 'stop' ? -1 : 1;
      }

      // If two events occur at the same time and have the same type, sort by x-coordinate.
      return a.element.rect().center().x - b.element.rect().center().x;
    });

Now I'm going to check the other failing documents.