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

[BUG] (v0.0.0): Prelude_No._1_BWV_846_in_C_Major.mxl #207

Open
3 tasks done
rvilarl opened this issue Jan 27, 2024 · 13 comments
Open
3 tasks done

[BUG] (v0.0.0): Prelude_No._1_BWV_846_in_C_Major.mxl #207

rvilarl opened this issue Jan 27, 2024 · 13 comments

Comments

@rvilarl
Copy link
Collaborator

rvilarl commented Jan 27, 2024

Expected behavior
Render correctly.
image

Actual behavior
Exception and wrong rendering.

Screenshot
image

Hints
The more you provide, the faster we can fix it.

  • Attach the MusicXML file used to produce the bug.
  • Attach the error messages.
  • Include any vexml code references where the bug may be.
@rvilarl
Copy link
Collaborator Author

rvilarl commented Jan 27, 2024

@rvilarl
Copy link
Collaborator Author

rvilarl commented Jan 27, 2024

@jaredjj3 @ronyeh I am moving to another city. Once I settle I will be more active. :)

@jaredjj3
Copy link
Collaborator

@rvilarl thanks for the bug report and good luck with your move!

@jaredjj3 jaredjj3 self-assigned this Feb 3, 2024
@jaredjj3
Copy link
Collaborator

jaredjj3 commented Feb 3, 2024

Looking at this now.

@jaredjj3
Copy link
Collaborator

jaredjj3 commented Feb 4, 2024

To troubleshoot, I colored the stave notes being used for the start and end notes.

  render(): PedalRendering {
    const vfStaveNotes = this.getVfStaveNotes();
    const vfPedalMarkingType = this.getVfPedalMarkingType();
    const vfPedalMarking = new vexflow.PedalMarking(vfStaveNotes).setType(vfPedalMarkingType);

    vfStaveNotes[0].setStyle({ fillStyle: 'green' }); // Change the color to green
    vfStaveNotes[1].setStyle({ fillStyle: 'red' }); // Change the color to red

    return {
      type: 'pedal',
      vexflow: {
        pedalMarking: vfPedalMarking,
      },
    };
  }
image

It looks like the second note of the second pedal marking is not being associated correctly. Even if the same note was being used as a start and end, I expect it to show as the end color.

When I implemented <direction> support, I always attached it to a note, since that's what vexflow needs. However, I think I might need to conditionally create TextNote to avoid issues like this.

I'm going to spend some time figuring out a better strategy for <direction> elements, especially before I try to do lilypond31a (Directions).

@jaredjj3
Copy link
Collaborator

jaredjj3 commented Feb 4, 2024

I thought I could make vexflow.TextNote objects that have no ticks, but that seems to be a special case for vexflow.BarNote objects (code). I'm skeptical that even if there was a supported way to render vexflow.TextNote as such, it would probably interfere with formatting. I'm going to study the problem a bit more.

@rvilarl, @infojunkie, and @sschmidTU, do you have any advice for handling <direction> elements? They're inherently not attached to a specific <note>, but vexflow typically requires a vexflow.Tickable to render them. It seems like I might have to handle each direction on a case-by-case basis — there doesn't seem to be a universal strategy I can apply to them like <technical> or <articulations>.

@jaredjj3
Copy link
Collaborator

jaredjj3 commented Feb 4, 2024

For now, this is what I'm planning to do when iterating through a <measure>:

  1. When handling a <note>, attach all consecutive <direction> elements after it.
  2. When handling a <direction> without a preceding <note> in scope, attach it to the next <note> within that measure.
  3. If there is no <note> within the <measure>, create a vexflow.GhostNote and attach the <directions> to it. If this doesn't work, hack an empty TextNote with duration '256' instead.

@infojunkie
Copy link
Contributor

infojunkie commented Feb 4, 2024

Looks like a complex specification indeed. And of course, it is complex because of all the cases of annotations that exist in real-word scores.

My reading of the <direction> specification tells me the following factors would affect the positioning algorithm:

  • The optional sub-element <offset> indicates where the direction will appear relative to the current musical location (in unit of score divisions).
  • The optional attribute directive changes the default-x position of a direction.
  • By default, a series of <direction-type> elements and a series of child elements of a <direction-type> within a single <direction> element follow one another in sequence visually.
  • For applications where a specific direction is indeed attached to a specific note, the <direction> element can be associated with the first <note> element that follows it in score order that is not in a different voice.
  • Child elements of <direction-type> include attributes default-x, default-y, relative-x, relative-y that affect their individual positioning.

At this moment, I don't fully understand which direction types need to be associated with notes, versus which don't. Maybe it's just a function of where they occur in the bar, but that would need clarification. Maybe you can ask a question in the musicxml discussion forum although the new maintainer of the format has not been very responsive. You may have better luck at StackExchange Music.

Happy to continue digging if you feel this is useful to you.

@rvilarl
Copy link
Collaborator Author

rvilarl commented Feb 4, 2024

@jaredjj3 I also do not see like @infojunkie why we have to associate directions to notes. Probably the abstraction can be improved in VexFlow, for example, Coda & Segno are StaveModifiers but pedal is not.

@jaredjj3
Copy link
Collaborator

jaredjj3 commented Feb 5, 2024

@infojunkie, thank you, this is an excellent summarization (even better than ChatGPT).

  • For applications where a specific direction is indeed attached to a specific note, the element can be associated with the first element that follows it in score order that is not in a different voice.

Ah, of course! This is where I went wrong — the current vexml algorithm associates a <direction> with the first succeeding <note> regardless of <voice>.

Here's the snippet I was dealing with (the full score is in #207 (comment)):

<note>
  <voice>5</voice>
</note>
<direction placement="below">
  <direction-type>
    <pedal type="stop" line="yes"/>
    </direction-type>
  <staff>2</staff>
  </direction>
<backup>
  <duration>16</duration>
</backup>
<note>
  <voice>6</voice>
</note>

The <direction> actually needs to be associated with the previous <note>, because the next <note> of the same <voice> appears in the next measure. The MusicXML <measure> spec also permits a <direction> to be specified without any <note>.

Thanks for illuminating the <direction> spec. I'm going to enumerate the edge cases and handle them.

@jaredjj3
Copy link
Collaborator

jaredjj3 commented Feb 5, 2024

@jaredjj3 I also do not see like @infojunkie why we have to associate directions to notes. Probably the abstraction can be improved in VexFlow, for example, Coda & Segno are StaveModifiers but pedal is not.

I think this is a shortcoming of the MusicXML spec. It's very confusing to have directives that maybe associate <direction> and <note> elements. Hopefully it'll be improved in mnx.

Ultimately, yes I agree if vexflow had a way to add StaveModifier objects inline within a Voice, it would make this a lot easier. I'm going to see how hard the implementation is before submitting an FR to vexflow. Thanks for calling that out!


The more I work with vexflow, I think this project would've been significantly easier if I was able to attach non-tickables to the stave in a matrix-like fashion. I'm not asking for this to be put in vexflow since it's really invasive, but I would like to share my thoughts.

const stave1 = Stave({
  label: Text('part1', { offset: Vector.fromOrigin(-5, 0) }),
  entries: [
    // A StaveEntry is like a vertical stack of elements.
    StaveEntry([Barline({ label: Text('1') })]),
    StaveEntry([KeySignature('C')]),
    StaveEntry([Clef('G')]),
    StaveEntry([TimeSignature('4/4')]),
    StaveEntry([Note('C', { duration: 'h' })]),
    StaveEntry([Chord([Note('C'), Note('D')])]),
    StaveEntry([Barline()])
  ]
});
const stave2 = Stave(); // empty for now
const system = System({ staves: [stave1, stave2] });
system.format().draw();

or maybe you have helpful instance methods:

const stave = Stave();

// This would merge into the internal structures of the stave.
stave.insert(StaveEntry([Note('C', duration: 'q')]), { at: Fraction(3, 4) });

This would be a different goal than EasyScore and Factory. This API is not optimizing for user writability — it's optimizing for programability while giving the maximum amount of flexibility the low level vexflow APIs offer.

I'm not sure how adaptable this is to vexml and if the benefits are worth the effort. I'll just keep this in the back of my mind next time I get blocked.

@infojunkie
Copy link
Contributor

@infojunkie, thank you, this is an excellent summarization (even better than ChatGPT).

You have a low opinion of humans and a high opinion of technology 😱

@jaredjj3
Copy link
Collaborator

I'm waiting on 0xfe/vexflow#1611 before I resume this.

@jaredjj3 jaredjj3 removed their assignment Jun 2, 2024
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

No branches or pull requests

3 participants