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

ModifierContext is too complex. Replace with a simpler formatter. #409

Open
Silverwolf90 opened this issue Aug 1, 2016 · 12 comments
Open

Comments

@Silverwolf90
Copy link
Contributor

Silverwolf90 commented Aug 1, 2016

A ModifierContext has a state that has 4 properties, each which maps to a Modifier.Position.

state property Modifier.Position Unit
top_text_line ABOVE staff space
text_line BELOW staff space
left_shift LEFT pixels
right_shift RIGHT pixels

Sidenotes:
text_line should be renamed to bottom_text_line.
Having different units for horizontal/vertical formatting is not ideal. But we can't escape that for now.

All these properties do is keep track of an incremented value in a specific direction. Isn't this already what the Formatter does? It advances a position forward along the x axis (albeit with complex logic to determine the amount...). It feels like the ModifierContext duplicates and conflates responsibilities since it tries to manage 4 positions at once. So what if we deprecated the ModifierContext, and replaced it with with a new, simpler formatter that takes Modifier.Position and advances a position given:

  • a Modifier.Position
  • Objects for formatting
  • Config options, ie: spacing

To replace the ModifierContext, I think we'd need 4 differently parameterized versions of these (one for each Modifier.Position).

In fact, in order to format GraceNotes within a GraceNoteGroup we use the existing Formatter and set its available width to 0 that the minimum width for each note is applies. But this is clearly a hack based on the absence of a simpler formatter.

@0xfe
Copy link
Owner

0xfe commented Aug 1, 2016

Yeah, the state units need to be rationalized for sure, but even if you use a formatter, I think you'll still need a context to keep track of the modifiers across voices. The ModiferContext does not do any formatting, it's just a container for all the modifiers in a tick (i.e., across voices), and it holds some tracking state for the independent formatters. (It also kickstarts the formatting process, but that's just for ordering, which needs to be global.)

And since each category of modifier (e.g., accidentals, fingering, articulations) have their own distinct formatting logic you'll need a way for them to both format themselves, and juxtapose alongside other modifiers.

I think a simpler (and necessary) cleanup would be to replace the various state properties with a position object that uses the same unit across all axes.

@0xfe
Copy link
Owner

0xfe commented Aug 2, 2016

Actually, thinking about this more, I think the units are pretty sane (staff spacing for vertical, and pixels for horizontal.) It actually makes the code more natural to reason about.

@Silverwolf90
Copy link
Contributor Author

Silverwolf90 commented Aug 2, 2016

I strongly disagree. Every decision about formatting should use the staff space. It is the canonical unit of modern music engraving. If one wants to change the pixel size of the staff space, it shouldn't be the burden of every formatter to keep track of that. I'd contend that the formatters should be completely independent of the pixel-to-staff-space ratio. Any resolution of staff spaces to pixels should happen at render-time -- not earlier.

It's important to note that I don't think VexFlow is ready for this at all.... yet. But this is an ideal to strive for.

@Silverwolf90
Copy link
Contributor Author

Worth noting that SMuFL expresses their bounding boxes in staff spaces.

@0xfe
Copy link
Owner

0xfe commented Aug 3, 2016

I guess I don't see how you can reason about horizontal formatting using vertical spacing. Are the ratios always fixed? Will there be scenarios that break this assumption?

(also, would love some pointers to literature about this -- I'm curious about how this is the canonical unit of music engraving.)

@Silverwolf90
Copy link
Contributor Author

See Behind Bars, page 5 - "The Stave"

The size of every notation symbol is measured in proportion to the stave size. A stave-space is the distance between two stave lines and is used as a measurement for notational symbols and spacing in this book.

Then she provides this diagram:

image

In "The Art of Music Engraving and Processing" by Ted Ross, the units are in staff spaces as well.

@Silverwolf90
Copy link
Contributor Author

Silverwolf90 commented Aug 6, 2016

Using staff spaces as the unit of measurement allows us to reason about formatting without being coupled to a specific pixel/staffspace ratio.

@mscuthbert
Copy link
Collaborator

Many MusicXML units are in tenths of a staff space ("tenths"). They are allowed to be floating point, so I wish they had chosen "spaces" to begin with, but it's an easy conversion.

What is important though is to precisely define where a staff-space begins and ends. My thought would be that a staff-space should go from the top of one staff line to the top of the next, so that a standard five-line staff's total bounding area is 4*staff-space + 1 * staff-line, rather than the space between lines (4 * staff-space + 5 * staff-line). If the staff space includes the staff-line in it, then it makes spacing out things like ledger line placement much simpler and allows for adjusting the thickness of staff lines without affecting the overall size of the staff.

@Silverwolf90
Copy link
Contributor Author

Silverwolf90 commented Aug 7, 2016

I do agree on the the staff's total bounding height (4 spaces + 1 line), but I think it's more intuitive to have the staff space go from staff line center to center. This way, both the size and boundaries of the staff space are completely independent of the staff line thickness. This also means that the top of the staff and the position of notehead on the top-most line have distinct y values (and similarly, the bottom y values). In my experience, both values are required for formatting.

@mscuthbert
Copy link
Collaborator

Center to Center might be even better, yes, agreed.

@rvilarl
Copy link
Collaborator

rvilarl commented Feb 6, 2022

After working a bit with ModifierContext I believe that the ideas here would be agood approach.

@AaronDavidNewman @0xfe what do your think? I refer to:

  • Vertical formatting with reference middle line of stave
  • Usage of stave-space for horizontal position.
  • Rename text_line to bottom_text_line

@mscuthbert
Copy link
Collaborator

I was looking for something else and came across this old, but still so useful discussion. Hope that even with @Silverwolf90 not being active right now, we can still work towards this goal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants