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

Improve Curve API #1607

Open
jaredjj3 opened this issue Jan 12, 2024 · 3 comments
Open

Improve Curve API #1607

jaredjj3 opened this issue Jan 12, 2024 · 3 comments

Comments

@jaredjj3
Copy link
Contributor

In stringsync/vexml#201, I'm using vexflow to draw slurs encoded in MusicXML. I have a few asks for the Curve API.

  1. Fix the types. Curve objects can be partial, but the constructor forces you to specify Note instead of Note | undefined.

  2. Allow me to specify the curve opening direction. 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 in CurveOptions like openingDirection?: 'up' | 'down' | undefined.

  3. Make vexflow dynamically calculate 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 calculate control points.

@AaronDavidNewman
Copy link
Collaborator

I've been thinking this for a long time, and even started on it a couple of times but didn't follow through (in fact, I think this may be an issue somewhere, if you look through the list). I think we should create a different curve object, instead of trying to patch the existing one. Because part of what is broken is the API.

  1. Actually you can do this now by just specifying one endpoint. But it would be nice to have it more clear, maybe you're already saying that.
  2. Yes, the existing interface really doesn't work. And it's very problematic when parsing musicxml, which actually lets you specify the shape you want, and you don't have the beaming information yet. We should allow 'up', 'down', and 'auto'.
    2.5 I'd add, allow you to specify the pitch on either end. Because sometimes you want the endpoint to be a note in a chord.
  3. Either this, or maybe a static method that calculates good control points for you, based on all the notes in between. This is actually a lot harder than you might think, because you don't always know what is between the endpoints. I think this might be one of those things that might not work under all circumstances, so the user might still need to tweak them. Having the option of setting the control points explicitly is something we would want to retain, because a UI would let you move them around. Also, when parsing music from other formats, the control points may be specified already.
    3.5. I would add to this, we should be able to specify 2 or 3 sets of control points. You would want 3 when the phrase is moving between staves (and the curve has an 'S' shape.

@jaredjj3
Copy link
Contributor Author

FWIW, I documented some existing curve behavior in stringsync/vexml#201 (comment) and stringsync/vexml#201 (comment).

Thanks for the discussion so far @AaronDavidNewman, I think we're pretty much in agreement. Just a few comments and clarifications:

I think we should create a different curve object, instead of trying to patch the existing one.

Yes, I think this is the better approach, since you won't have to worry about regressing the existing implementation and juggling old and new option compatibilities/interactions.

  1. Actually you can do this now by just specifying one endpoint.

Right, but my point is if you're using TypeScript, you have to construct it in a manner that's not compatible with strict: true:

new Curve(startNote, undefined as any, curveOptions);
new Curve(undefined as any, stopNote, curveOptions);

or in my case since startNote and stopNote are dynamically calculated:

new Curve(startNote as any, stopNote as any, curveOptions);

maybe a static method that calculates good control points for you, based on all the notes in between

I like this approach. It reinforces that the caller is responsible for defining control points that works in the graphical context.

When I made this issue, I didn't think about the UI editing use case but it makes sense. It looks like musicXML also supports bezier curve data, so there will be a place for control point data to live.

@AaronDavidNewman
Copy link
Collaborator

I think having a slur that ends at the end the line doesn't mean the slur doesn't have an end note. There is still a 'last note of the slur', and it may not be in the same measure. Like if you wanted to slur from the endpoints shown below (for some reason). You would render it as 2 different slurs, the first one would end at the end of measure 11, so there should to be some way to indicate that. The second slur would be from the beginning of the bar to the last note in the slur in measure 14. So I think 'terminate start on stave', 'terminate end on stave' would be a options for the slur. (if the phrase went on for the whole line, it could be both, but that's an extreme case).

Most vexflow logic can safely assume everything happens in the same stave, but for slurs that is definitely not the case.

I haven't completely thought this part out, but I think you might want to pass in all the notes in between. Then the logic can calculate the control points from the notes that are there, based on stem directions and Y positions. You could pass in any number of notes, the first/last X position would be the start, end (or we could require that the notes be passed in order).

Passing in an array also handles the case of a slur that is from the first/last note in the line to the end of the line. An array can contain a single note, and you would set the 'terminate on start/end note' modifier.

image

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

2 participants