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

Render lilypond33c #201

Merged
merged 5 commits into from
Jan 15, 2024
Merged

Render lilypond33c #201

merged 5 commits into from
Jan 15, 2024

Conversation

jaredjj3
Copy link
Collaborator

@jaredjj3 jaredjj3 commented Jan 7, 2024

This PR renders lilypond33c. It supports the slur[placememt] attribute.

I decided to keep the solution as bare bones as possible while 0xfe/vexflow#1607 is deliberated. Eventually, we need to honor the bezier curve attributes in the <slur> element. These translate to control points in vexflow.

Divergences

  • lilypond seems to omit a slur in the second measure.
  • lilypond does not honor the slur[placement] attribute.
  • vexml's slur collides in measure 2.
  • Guitar Pro 7 incorrectly clobbers slurs.

lilypond

image

vexml

image

osmd

image

MuseScore 4

image

Guitar Pro 7

image

Soundslice

image

@jaredjj3 jaredjj3 self-assigned this Jan 7, 2024
@jaredjj3
Copy link
Collaborator Author

jaredjj3 commented Jan 7, 2024

@rvilarl is there a way to tell a vexflow.StaveTie to render from the tip of a note stem instead of the note head?

@rvilarl
Copy link
Collaborator

rvilarl commented Jan 8, 2024

@rvilarl is there a way to tell a vexflow.StaveTie to render from the tip of a note stem instead of the note head?

@jaredjj3 you can do that with Curve

@jaredjj3
Copy link
Collaborator Author

jaredjj3 commented Jan 8, 2024

I see... vexflow.StaveTie really is for just ties, which are for notes of the same pitch. I thought ties were for general use based on https://github.com/0xfe/vexflow/wiki/Tutorial#step-5-ties--run-. Thank you so much!

@rvilarl
Copy link
Collaborator

rvilarl commented Jan 8, 2024

I see... vexflow.StaveTie really is for just ties, which are for notes of the same pitch. I thought ties were for general use based on https://github.com/0xfe/vexflow/wiki/Tutorial#step-5-ties--run-. Thank you so much!

They do not have to be notes of the same pitch, but the ties have to go from notehead to notehead. For example, you cannot do that with a curve:

image

@jaredjj3
Copy link
Collaborator Author

jaredjj3 commented Jan 9, 2024

Leaving some notes here for implementation.

  • Slurs are represented as Curve in vexflow.
  • Curves need one or two notes to render.
  • Curves may have one note when it is interrupted by a new System.
  • Curves specify the position that they are rendered: either at the note head or the outer edge of the note stem (if it exists).
  • The bezier curve orientation seems to be a f(curve.y0, curve.y1). It can be controlled with the invert option.
  • Curves can specify control points that affect that shape of the curve.

I need an algorithm for determining curve options. I propose the following:

  1. Calculate the slur placement using the slur[placement] or the positions of the start and end notes. When the voice ascends or ends up on the same pitch, the placement should be above. Otherwise, the placement should be below.
  2. Calculate the stem directions of the start and stop notes. The possible outcomes are up, down, none, and undefined.
  3. Calculate the curve positions based on the stem directions.
  4. Infer the orientation from the curve start and stop coords. The possible outcomes are concaveup and concavedown.
  5. Invert when placement is above and orientation is concaveup or when placement is below and orientation is concavedown.

@jaredjj3
Copy link
Collaborator Author

After experimenting a bunch, I'm coming to the conclusion that I'll need to calculate the control points of the bezier curve for slurs. There's some prior art in OSMD, but this is an expensive detour right now.

I'm going to try to find a simple solution that works for lilypond33c, and then a more complete solution later.

@rvilarl
Copy link
Collaborator

rvilarl commented Jan 10, 2024

After experimenting a bunch, I'm coming to the conclusion that I'll need to calculate the control points of the bezier curve for slurs. There's some prior art in OSMD, but this is an expensive detour right now.

I'm going to try to find a simple solution that works for lilypond33c, and then a more complete solution later.

Let me know if some logic should be added to vexflow

@jaredjj3
Copy link
Collaborator Author

@rvilarl, yes actually, that would be great. I feel like Curves should work in most cases and that I'd only have to specify the control points in exceptional cases. Would you let me know if you have capacity to work on this in vexflow please? If so, I'll pause on this vexml implementation and wait for what you come up with. Let me know if you'd like me to file a bug against vexflow.

  • The constructor has a type bug. Curve objects can be partial, but the constructor forces you to specify Note instead of Note | undefined.

  • In CurveOptions, I'd like the option specify whether I want the Curve opening to be up or down. Otherwise, I have to mimic the calculation that vexflow does to figure out the direction of the opening, and my implementation will know too much about how vexflow works. Maybe a property like openingDirection?: 'up' | 'down' | undefined.

  • The biggest ask I have is that vexflow dynamically calculates cps when not provided one in CurveOptions. Currently, it will just use the default cps that the Curve constructor sets. As I was migrating slurs from StaveTie, I thought it would be a similar experience where Curve would just mostly work. However, I had no idea how cps were control points that influenced the shape of the curve until I opened up the Curve implementation and studied it. It probably would be a good experience for vexflow users to not have to specify this.

@rvilarl
Copy link
Collaborator

rvilarl commented Jan 10, 2024

@jaredjj3 please raise the issue and wait.

@jaredjj3
Copy link
Collaborator Author

jaredjj3 commented Jan 14, 2024

I think I can get away not calculating control points. I played with vexflow ascending and descending voices with slurs placed above and below: https://jsfiddle.net/jco31qxn/155/ edit: added same-pitched voices https://jsfiddle.net/jco31qxn/184/

I just need to figure out the invert: true cases. I think it's a f(from.y, to.y, from.stemDirection, to.stemDirection, from.curvePosition, to.curvePosition). I might just hardcode the cases.

@jaredjj3
Copy link
Collaborator Author

src https://jsfiddle.net/jco31qxn/184/

slur placement above, voice ascending

image
slant stem0 stem1 invert
up up up true
down up down false
up down down false

slur placement above, voice descending

image
slant stem0 stem1 invert
down down down false
up down up true
down up up true

slur placement below, voice ascending

image
slant stem0 stem1 invert
up up up false
down up down true
down up up true

slur placement below, voice descending

image
slant stem0 stem1 invert
down down down true
up down up false
down up up false

slur placement above, voice neutral

image
slant stem0 stem1 invert
none up up true
none down down false

slur placement below, voice neutral

image
slant stem0 stem1 invert
none up up false
none down down true

@jaredjj3
Copy link
Collaborator Author

Ok, I think I figured out how to calculate the curve opening direction. In https://jsfiddle.net/jco31qxn/212/ I set invert: false for all the curves and I played with some configurations.

  • when the from and to notes are defined, the curve opens the same direction as the to note's defined stem
  • when only one note is defined, the curve opens the same direction as the note's defined stem
  • when the stem of the critical note is undefined, the curve behaves according to the direction of the auto stem
image

@jaredjj3 jaredjj3 marked this pull request as ready for review January 15, 2024 14:17
@jaredjj3 jaredjj3 merged commit 41a2e3a into master Jan 15, 2024
1 check passed
@jaredjj3 jaredjj3 deleted the lilypond33c branch January 15, 2024 14:17
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