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 Positioning API: Extract out current positioning code into stand-alone functions. #366

Open
Silverwolf90 opened this issue Jun 19, 2016 · 4 comments

Comments

@Silverwolf90
Copy link
Contributor

Silverwolf90 commented Jun 19, 2016

Currently, positioning in VexFlow is represented by various enums (eg. Modifier.Position) which map to specific code branches in each class. The user has absolutely no control over how each Position was implemented for each class.

For an example of this, take a look at StaveNote.prototype.getModifierStartXY. This method is problematic because each returned { x, y } pair comes with a non-obvious value. Calling stavenote.getModifierStartXY(Position.ABOVE, 1) gives no indication of the result of the x coordinate. Is it the stem's x value? The center of the notehead for the provided index? The center of the entire stavenote's bbox? What happens with chords with displaced notes? What about rests? None of this is obvious or configurable. Confusingly, the x value represents the "center of the non-displaced noteheads" regardless of the provided key index. And the y value is the same for Position.ABOVE and Position.BELOW.

What if we extracted these code branches into stand-alone functions?

A more flexible and powerful API might look something like:

const Position {
  H: {
    noteStem: (note) => note.getStem().getX(),
  },
  V: {
    noteStemTip: (note) => note.getStemExtents().topY;
  }
};

// Place articulation at stem tip
articulation.setInitialPosition(Position.H.noteStem, Position.V.noteStemTip);

With functions we get the advantage of composability:

const stemTipPadded = compose(withPadding(20), Position.H.noteStemTip);

higher-order functions:

const noteheadPosition =
  (position) => (keyIndex) => (chord) =>
    position(chord.getNotehead(keyIndex));

const noteheadCenter = noteheadPosition(Position.H.center);

more complex, but still low-level, positioning logic is separated from higher level classes

const centeredBetween = (startNote, endNote) => {
  const left = Position.H.left(startNote);
  const right = Position.H.right(endNote);
  const width = right - left; 
  return left + (width / 2);
}

To me, this seems much simpler, more flexible, more powerful, and more obvious.

Curious to hear thoughts on this.

@Silverwolf90 Silverwolf90 changed the title Improving Layout: What if positions were just an extendable set of functions? Improving Layout API: What if positions were just an extendable set of functions? Jun 19, 2016
@Silverwolf90 Silverwolf90 changed the title Improving Layout API: What if positions were just an extendable set of functions? Improving Position API: What if positions were just an extendable set of functions? Jun 19, 2016
@Silverwolf90 Silverwolf90 changed the title Improving Position API: What if positions were just an extendable set of functions? Improving Positioning API: What if positions were just an extendable set of functions? Jun 19, 2016
@Silverwolf90 Silverwolf90 changed the title Improving Positioning API: What if positions were just an extendable set of functions? Improve Positioning API: What if positions were just an extendable set of functions? Jun 19, 2016
@Silverwolf90 Silverwolf90 changed the title Improve Positioning API: What if positions were just an extendable set of functions? Improve Positioning API: Extract out current positioning code into stand-alone functions. Jun 19, 2016
@0xfe
Copy link
Owner

0xfe commented Jun 20, 2016

So firstly, I hate the current positioning mechanism, and really wish that I had put in well-defined anchor points from the start.

I like your idea better, because you have anchor functions instead of anchor points, which brings in a lot of benefits (like composability.) (BTW, I also think that it's easy to go crazy with this approach and have difficult-to-read higher order functions.)

I'm wondering if we can take this a step further so as positions change as other elements join the context. E.g., if I want a modifier below a note, it renders underneath other modifiers that are already there. So the anchor functions are functions of both the element and the context.

@Silverwolf90
Copy link
Contributor Author

Silverwolf90 commented Jun 30, 2016

Thinking about it more, it seems like we're missing abstractions around both grouping and positioning. A lot of code in VexFlow is dedicated to both of these things, but they're being performed in slightly different ways in each class. It's likely that a lot of logic could be consolidated.

For grouping, we have temporal groups (TickContext), and formatting groups (ModifierContext), but we don't have a notion of simple graphical groups of glyphs/text/paths.

For positioning, we're in need of a much better design than what we currently have, as mentioned in the first post.

If VexFlow had better lower-to-mid level abstractions around positioning and grouping I bet we'd be able to simplify the codebase substantially. Any graphical class which consists mainly of formatting and drawing would benefit.

Edit: clarity

@0xfe
Copy link
Owner

0xfe commented Jul 1, 2016

Yes, agreed. A few years ago, I started work on a Container class, which abstracted away the grouping logic because I wanted to build a new StaveContext for cross-stave formatting... but that fizzled. :-)

I'm all for new abstractions, especially those that will add more advanced formatting without the need for hard coded constants. An interesting experiment would be elements that know their own identities and contexts, and are responsible for positioning themselves -- e.g., at every iteration of some positioning loop, they would adjust their positions to minimize some cost function (which would include things like spacing, collisions, connections to anchor points, etc.)

How about we start collaborating on a high-level design/API for something like this? If we can do this incrementally (i.e., slowly move existing elements into the new design) that would be ideal.

@Silverwolf90
Copy link
Contributor Author

Personally, I would hesitate to think about high level ideas without a stronger low-level foundation. Part of the problem there is that the API's across objects are inconsistent and lack a common interface past preFormat/postFormat/render -- which are just lifecycle methods to trigger side-effects; they don't expose useful information.

A big improvement would be better bounding box support. Most objects don't expose one, but ideally every graphical object should expose their bounding box. From there, we can extract the common operations we're doing in every class (eg: collision avoidance, offset calculations, etc) into generic utilities which operate on bounding boxes. Once that's in place, we're in a better position to think about advanced formatting like you're describing.

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

2 participants