Skip to content

Move CursorFrameHint to CursorStateHint#280

Merged
jaredjj3 merged 7 commits intomasterfrom
playback
Apr 11, 2025
Merged

Move CursorFrameHint to CursorStateHint#280
jaredjj3 merged 7 commits intomasterfrom
playback

Conversation

@jaredjj3
Copy link
Collaborator

@jaredjj3 jaredjj3 commented Apr 11, 2025

This PR fixes the bug mentioned in #260 (comment). It will be released in 0.1.7.

  • Moves CursorFrameHint to CursorStateHint.
  • Adds StartHint and StopHint.
  • Keeps CursorStateHint[] calculation lazy.
  • Adds light test coverage to CursorStateHint (checks that getting hints does not throw).
  • Improves playback tests by making each test more configurable.
  • Adds Cursor.iterable.
  • Fixes rendering.Document bug described in Pianoplay integration #260 (comment) by allowing elements.Note to return multiple pitches.
  • Surfaces Note subtypes in elements.Note.getSubtype (note or chord).

I added playback_chords.musicxml to the vexml playback test suite, which is a snippet from #260 (comment).

image

@jaredjj3 jaredjj3 self-assigned this Apr 11, 2025
@jaredjj3 jaredjj3 marked this pull request as ready for review April 11, 2025 12:51
@jaredjj3 jaredjj3 merged commit 26746ab into master Apr 11, 2025
1 check passed
@jaredjj3 jaredjj3 deleted the playback branch April 11, 2025 12:52
@rvilarl
Copy link
Collaborator

rvilarl commented Apr 12, 2025

@jaredjj3 where are the hints now? Sorry I do not find them.

@jaredjj3
Copy link
Collaborator Author

cursor.getCurrentState().hints.get()

@rvilarl
Copy link
Collaborator

rvilarl commented Apr 12, 2025

I get

Property 'getCurrentState' does not exist on type 'CursorState'.ts(2339)

@jaredjj3
Copy link
Collaborator Author

If you have a CursorState object, it should just be cursorState.hints.get().

@rvilarl
Copy link
Collaborator

rvilarl commented Apr 12, 2025

Hi @jaredjj3

I have two doubts:

  1. If only one pitch of a chord is tied to another note, how is this going to be presented?
  2. I would expect to get start first and then stop, but the first hints in the score come with stop.

image

@jaredjj3
Copy link
Collaborator Author

  1. If only one pitch of a chord is tied to another note, how is this going to be presented?

All the pitches existing in the previous state will also have a sustain hint, even though that's not completely accurate. If this is undesirable, would you file a bug please?

That's a shortcoming of the way elements.Note.sharesACurveWith implemented:

sharesACurveWith(otherNote: Note): boolean {
return this.noteRender.curveIds.some((curveId) => otherNote.noteRender.curveIds.includes(curveId));
}

I think I need to create an elements.ChordNote and implement an elements.ChordNote.sharesACurveWith(chordNote: ChordNote).

  1. I would expect to get start first and then stop, but the first hints in the score come with stop.

Me too, that's definitely a bug. Right now, the tests just check that nothing is thrown when getting hints. However, I think this warrants more thorough tests.

@rvilarl
Copy link
Collaborator

rvilarl commented Apr 13, 2025

@jaredjj3 I have a suggestion. What about making the hints pitch based rather than note based? Then we would have the types start, stop, keep.

@jaredjj3
Copy link
Collaborator Author

The reason it's based on notes is to support more use cases. For example, if you want to color and uncolor the notes based on if they're active or not, just having the pitch is not enough information.

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.

2 participants