better way to maintain the `producer` and `renderer` modules

[rvilarl]

Thanks for adding support for this! Now that we have some depth, do you see a better way to maintain the producer and renderer modules?

Originally posted by @jaredjj3 in https://github.com/stringsync/vexml/pull/25#pullrequestreview-943352839

[jaredjj3]

The message types are the contract that binds the producer and renderer together. If we can make those types more stable, then we can independently work on the producer and renderer. Do you have any idea of how many more message types we need to cover let's say 80% of use cases? I'm looking for an order of magnitude and a level of uncertainty.

[rvilarl]

I would say around 50 messages. I have come to this number marking the VexFlow files that I believe might end up being a message, You also suggested the past to make a class for each message.

[jaredjj3]

I have come to this number marking the VexFlow files that I believe might end up being a message

Would you show me an example?

You also suggested the past to make a class for each message.

I like what we've landed on so far. The messages should be "dumb". Any message-related behavior should be encapsulated in a MessageReceiver.

[rvilarl]

messages.txt

43 and I decided to round up. It is just a quick exercise.

[jaredjj3]

Ah, now I understand what you mean. Here's what I think we need to do:

• Map each Vexflow file to a list of MusicXML elements
• Determine what data we need from each MusicXML element to satisfy Vexflow, and those become our messages

Thoughts?

[rvilarl]

Sounds good. There are some one to one obvious elements that we should implement and that map pretty good. I would say that we should do as a minimum the sunset that allows to play the score:

• stave keys
• stave bars
• tuplets

[jaredjj3]

I am looking into how to resolve the chords in a sensible way. I want to go for scores with several parts then. I believe that both changes may have a big impact.

I'm continuing the maintainability discussion here. Please don't feel obligated to respond to my thoughts. I'm dumping them to possibly revisit later, and they're not particularly well organized.

---

I have two ideas that I'd like to experiment with:

1. Use state machines to update context, decide what next node to deeply dissect (Cursor is only top-level nodes), and emit messages when the context and the state machine event warrant it.
2. Fragment message production so that it's easier to work on when there are multiple authors.

For example, this code is what happens when a <note> element is encountered:

https://github.com/stringsync/vexml/blob/1b5719625bddb0506bbb67b04a21b18fcc4b8077/src/producer.ts#L77-L170

The main state machine could invoke a child state machine (xstate docs). In our example, we could have rest and audible child state machines. The context object that would get updated would be slices of the variables currently available in the Producer.getMessages closure. Perhaps we don't even need Cursor anymore.

The same principle applies for the Renderer. The difference is that our events are vexml Message objects instead of element nodes.

[jaredjj3]

Going to start working on this.

[jaredjj3]

@rvilarl, I'd like to reflect on https://github.com/stringsync/musicxml/issues/1#issuecomment-1058066180 and our general approach. Please don't change anything you're doing now. The work you're doing will continue to shape the evolution of this library.

No pressure to continue the discussion, but I'm always happy to hear your thoughts.

Problem

The message pattern was heavily influenced by SAX parsing (example). This approach is conducive to state machines, as a SAX event can trigger transitions and side effects.

However, after studying Renderer, I don't see the rendering state machine approach scaling very well. The main drawback is that there is a lot of state you have to track, which can be difficult to maintain.

I think this is a consequence of MusicXML elements having a lot of forks in them. For example, see the possible contents of <note>. The message approach also means you have to store state while you wait until you know which fork you land on.

It's not all bad. In summary, here's what we got with the current approach:

Pros

• Message receivers don't have to navigate deeply nested XML structures.
• MusicXML interpretation is decoupled from consumption, allowing each side to be developed independently.
• Messages are trivially serializable. I envisioned that this would enable things like shared document editing.

Cons

• Domain complexity is offloaded to the message receiver. Message receivers don't have to worry about XML, but they still have to do something meaningful with the messages.
• Message abstractions can only fully cover the MusicXML spec if they contain as much or more information as the MusicXML spec. However, it's worth noting that we don't need 100% MusicXML coverage for rendering.
• Abstracting messages is hard. Flattening the MusicXML spec means we have to continuously decide what level of granularity to abstract to. Right now, decisions are driven by how EasyScore works, but this may not work well for all use cases.

Proposal

The state that we need for rendering is already in the MusicXML document. I propose replacing message production and handling with a view layer that wraps the MusicXML document. The view layer has the following traits:

• It is stateless. All non-computed data needed for rendering should be queried from the MusicXML document as needed. Think of the get* methods in the NodeHandler subclasses today (example).
• It provides convenient typesafe handles for navigating musical notation structures that are encoded in MusicXML. Callers (renderer, in our case) don't need to know anything about MusicXML, but they do need to understand how vexml organizes music.
• It exclusively performs readonly operations. I think the key to #66 is to make the renderer responsible for marrying VexFlow objects with MusicXML. Therefore, the proposed view layer will need to expose a list of "authoritative nodes" or something similar to make the connections possible by consumers.

The ultimate goal is to reduce the amount of state juggling that has to be done in Renderer, hopefully making it easier to maintain and test.

I'll be testing this idea in experiment/view branch. Again, for now, you don't need to change your trajectory. I'll abandon this idea if it doesn't really make the codebase more maintainable.

[rvilarl]

Sounds great, I hope that it maskes our lib simpler.

[jaredjj3]

@rvilarl, I've been using your rendering work to navigate this new approach. Now that I've made progress on https://github.com/stringsync/vexml/issues/28#issuecomment-1430020367, I'm pretty confident that is a much more maintainable approach.

When you get a chance, would you take a look at the experiment/view branch so far and let me know the concerns you have?

[rvilarl]

@jaredjj3 the files look simple and good commented. The structure is more flat and this once again makes think simpler. I am not a TS expert and some declarations scare me, I will learn.

The app is not working though, I gues that this is WIP and expected.

No concerns once it works :)

[jaredjj3]

Hey @rvilarl, I'm currently trying to get 01a-Pitches-Pitches.xml to look reasonable on the experiment/view branch. I ran into a formatting issue that I need help with. I see that you've contributed a lot to VexFlow, so I'm leaning on your expertise here.

I found out that we're getting "free" padding as a side effect of unconditionally adding a voice for direction notes. In renderer.ts, if you comment out adding direction notes in L245 and adding vexflow.TextNotes in L339, you will see that the notes will not have any padding.

I know I can fix this by setting the measure width, but I'd like to get this to work using the auto width feature of vexflow.System. I tried adding padding by preformatting tickables using vexflow.Formatter.SimpleFormat, but it doesn't seem to have an effect on the rendering. I suspect that the vexflow.System.format call is overriding the vexflow.Formatter.SimpleFormat changes, but I haven't been able to pinpoint it yet.

How do I add padding to each note?

[rvilarl]

Hi @jaredjj3, I thought that adding the direction notes with no text would have no effect. Padding should be added to the system. You can use Formatter.preCalculateMinTotalWidth to get the minimal, decide how much you want to add and set the width.

[jaredjj3]

@rvilarl, I tried what you suggested, but it still doesn't look right. Would you show me what you mean, please?

https://jsfiddle.net/jaredjj3/81uen7hm/46/

[rvilarl]

@jaredjj3 preCalculateMinTotalWidth returns the space used by the voice. You need to add

stave1.getNoteStartX() which considers the space required for the clef and time signature VF.Stave.defaultPadding which is the default padding

now working

https://jsfiddle.net/6fntjqm8/

[jaredjj3]

Thanks, @rvilarl! I was able to come up with a reasonably fast way to render responsively. There's still a lot I need to do, but this approach is showing more promise.

https://user-images.githubusercontent.com/19232300/221427955-368d69f2-cf68-4e8b-97c9-066105f40ad8.mov

[rvilarl]

Wow this is quick!

[jaredjj3]

@rvilarl, just giving you a pulse check, I got busy with some other things. I want to try to finish this refactor this weekend. Don't let this block you on trying other things on the main branch!

[rvilarl]

@jaredjj3 I am still there! I am tryin to get a new vexflow release through

[jaredjj3]

After wrestling with the vexflow.Factory API for a while now, I've come to the conclusion that it doesn't offer the flexibility we need for vexml. Particularly, it's been difficult calculating how much width a vexflow.System takes, and figuring out the (x, y) coordinates based on that (if there's not enough width on the current line, you need to create a new line). preCalculateMinTotalWidth gets me part of the way, but it's missing how much space a clef and a timesignature may take.

I'm going to switch to using the Native API, and see if that makes things easier. I'm aware that I'll be reimplementing a lot of the vexflow.Factory functionality, but I think I need the flexibility.

[jaredjj3]

@rvilarl, take a look at this fiddle. Why does formatter.preCalculateMinTotalWidth() return 62.69704? Using that width would cause all the notes to stack on top of each other.

[rvilarl]

formatter.preCalculateMinTotalWidth() does not allow space for the clef and the time signature. What are you missing with the factory interface? The native one is more complicated and VexFlow aims to make the factory interface the standard one.

[jaredjj3]

Ah, I see, thank you. Is there a standard way to calculate the width of all the stave modifiers?

What are you missing with the factory interface?

The problem I'm trying to solve early on is being able to responsively render measure lines based on the renderer width. This is particularly important since this is a library that targets the web.

The biggest issue is being able to "prototype" a system. For example, I want to be able to instantiate a system, calculate its width with autoFormat: true, then maybe re-render the system somewhere else based on whether or not I have enough width budget on the current line. However, when I format a system, there seems to have a lot of side effects that lock me into a system, preventing me from changing its spatial attributes. I only have an experimental understanding of this, so please correct me if I'm misunderstanding something.

I've tried extending vexflow.Factory within vexml, but this ultimately felt like I was reaching into the internals too much. I think it'd be better if we keep vexml less coupled so that changes to vexflow.Factory don't break vexml.

[rvilarl]

Ah, I see, thank you. Is there a standard way to calculate the width of all the stave modifiers?

You have to add stave.getNoteStartX() plus padding

What are you missing with the factory interface?

The problem I'm trying to solve early on is being able to responsively render measure lines based on the renderer width. This is particularly important since this is a library that targets the web.

The biggest issue is being able to "prototype" a system. For example, I want to be able to instantiate a system, calculate its width with autoFormat: true, then maybe re-render the system somewhere else based on whether or not I have enough width budget on the current line. However, when I format a system, there seems to have a lot of side effects that lock me into a system, preventing me from changing its spatial attributes. I only have an experimental understanding of this, so please correct me if I'm misunderstanding something.

I've tried extending vexflow.Factory within vexml, but this ultimately felt like I was reaching into the internals too much. I think it'd be better if we keep vexml less coupled so that changes to vexflow.Factory don't break vexml.

If I understand correctly, you want to be able to call Format several times on System, right?

[jaredjj3]

Right, I think that would accomplish what I need to do.

[rvilarl]

OK I will look into that :)

[jaredjj3]

OK I will look into that :)

Thinking about it for a bit, really what I need is a way to pre-calculate the width of a system without committing it to the Factory store. This is a more pointed approach, and I imagine it's less cumbersome than juggling all the state that format() touches.

Maybe the ability to make a virtual/calculation-only Factory could work here? Right now, there's a hard requirement to have a DOM element to use Factory.

[rvilarl]

Could you provide a piece of code to show hoe you would use System?

[jaredjj3]

I ended up coming up with an example of the caller, but I think this illustrates the point. transform() would contain a method that may need to format() a System more than once.

const MAX_WIDTH = 480;

let remainingWidth = MAX_WIDTH;
for (let ndx = 0; ndx < systems.length; ndx++) {
  const currentSystem = systems[ndx];
  const previousSystem: System | null = systems[ndx - 1] ?? null;

  const requiredWidth = getRequiredWidth(currentSystem);
  const hasEnoughRemainingWidth = remainingWidth >= requiredWidth;
  if (hasEnoughRemainingWidth) {
    // transform wraps the system to add useful transformation methods to vexflow.System
    // it would be the thing that uses `format()`
    transform(currentSystem).moveToEndOf(previousSystem);
  } else {
    if (previousSystem) {
      transform(currentSystem).moveToNextLine();
      transform(previousSystem).stretchSystemToFillLine();
      let remainingWidth = MAX_WIDTH;
    } else {
      console.warn(`not enough container space for system: ${currentSystem}`);
      transform(currentSystem).setWidth(MAX_WIDTH);
    }    
    remainingWidth = Math.max(0, remainingWidth - requiredWidth);
  }
}

[rvilarl]

@jaredjj3 could you modify the jsfiddle to show want you want to do and get it to fail

https://jsfiddle.net/ronyeh/xure9svb/

[jaredjj3]

@rvilarl, sorry that I've kept you waiting here, I haven't been able to figure out this resizing issue for a while. Ultimately, I do think Factory has a place in the vexflow ecosystem, but not so much in vexml because we need maximum flexibility.

I'm going to set aside time to try to figure out this issue. If I can't figure it out, I'll defer it, copy the rest of the functionality you've made so far, and then get your review to merge. That way, you can continue to march forward independently of me trying to figure out this problem. Especially since it looks like you're almost done with adding different music fonts in vexflow :)

[jaredjj3]

@rvilarl, I was able to figure out resizing! The difference between this version and the video in https://github.com/stringsync/vexml/issues/28#issuecomment-1445421867 is that I'm able to account for beginning modifiers for each line as well as resize the lines to fit the renderer width. These are also the fastest rendering times, which is a good sign.

The difficulty stemmed from formatting side effects on the setters for different vexflow objects, so I created data containers that have side effects I could manage. I made it so that I can create vexflow objects not tied to anything whenever I want.

There was also the other problem when I wanted to format things multiple times, but vexflow objects don't support this well. For example, when stretching the width of each system to make a line fit, I may need to change the width of some vexflow.Staves or even move their position (overflow to next line), then reformat. Ultimately, the data containers could be changed freely, so I lost the need to reformat.

The next steps are

1. Distribute the formatting logic across the data container classes (instead of everything in the Vexml class).
2. Reach feature parity with master.
3. Improve unit test coverage.

When you get a chance, would you please take a look at the experiment/view branch and let me know if you have any concerns so far? I'm hoping that this imperative approach is much easier to maintain than the "event"-based approach we have been doing before. I'm hoping to submit for review in a week or two.

https://github.com/stringsync/vexml/assets/19232300/83f3c910-24c1-4d93-adbe-e9a779d3b48e

[rvilarl]

Thanks