-
Notifications
You must be signed in to change notification settings - Fork 668
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
Comments
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 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. |
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. |
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. |
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.) |
See Behind Bars, page 5 - "The Stave"
Then she provides this diagram: In "The Art of Music Engraving and Processing" by Ted Ross, the units are in staff spaces as well. |
Using staff spaces as the unit of measurement allows us to reason about formatting without being coupled to a specific pixel/staffspace ratio. |
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. |
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. |
Center to Center might be even better, yes, agreed. |
After working a bit with @AaronDavidNewman @0xfe what do your think? I refer to:
|
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. |
A
ModifierContext
has astate
that has 4 properties, each which maps to aModifier.Position
.state
propertyModifier.Position
top_text_line
ABOVE
text_line
BELOW
left_shift
LEFT
right_shift
RIGHT
Sidenotes:
text_line
should be renamed tobottom_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 theModifierContext
duplicates and conflates responsibilities since it tries to manage 4 positions at once. So what if we deprecated theModifierContext
, and replaced it with with a new, simpler formatter that takesModifier.Position
and advances a position given:Modifier.Position
To replace the
ModifierContext
, I think we'd need 4 differently parameterized versions of these (one for eachModifier.Position
).In fact, in order to format
GraceNotes
within aGraceNoteGroup
we use the existingFormatter
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.The text was updated successfully, but these errors were encountered: