moonwave99/fretboard.js

Roadmap

jaredjj3 opened this issue · 14 comments

jaredjj3 commented

Thank you for the this library! I think it is extremely well made and I plan on using it after I figure out if the licensing is compatible.

What are the future plans of this project?

moonwave99 commented

Hi @jaredjj3, thank you so much for reaching out!
Let me know about how will you use it, and if you need any integration help.

Atm I have no time for improving more features.
There are a couple of bugs I would like to fix first, then perhaps I can think of some future roadmap.

jaredjj3 commented

Thanks for the update. I'm happy to help.

Since my first comment, I've been enhancing a musicXML renderer called opensheetmusicdisplay. I created a system that allows developers to hook into events related to the music notation. The design proposal for official adoption is in this issue comment and a video clip showcasing some of the events is:

Screen.Recording.2021-09-19.at.10.46.32.AM.mov

This new system lets me know what notes, frets, and strings are being played (or about to be played). My goal for this week is to wire this data into fretboard.js.

I will update this comment with some integration insights, bug reports, and/or feature requests after I finish. Perhaps this can seed a roadmap fretboard.js.

moonwave99 commented

Hey @jaredjj3 , that looks great! I have some similar examples made for abcjs, you may already have seen it.

Btw I was very interested in integrating a similar system in a music education project I was working on, but I never managed to find any open source system that was mature enough to allow for flexible authoring / event listening.

ABC notation is super easy to author, and the library is great too, but some features were not completely documented yet, plus it doesn't support tab out of the box.

Keep me updated with the progress!

jaredjj3 commented

I have great news! The integration was a breeze and a positive experience. I still need to style it, but it's functional. Here's a demo.

Screen.Recording.2021-09-22.at.9.58.38.AM.mov

NB: All reference links are pinned at commit b860d3d881dc48f4c187b14526d4f78687b1469c.

Insights

React Component Development

I created a React component which is being consumed by a page-level component.

When developing the React component, I wanted to keep the same philosophy you had for fretboard.js. I purposely avoided making the component have an expressive API. It is the callers' responsibility to map semantics to styles. This approach made it feel like a more natural wrapper around fretboard.js instead of something more transformative.

I also wanted to align with React's philosophy by maintaining a declarative API. A caller should be able to declare positions and styles, and the component will do the heavy lifting of making that happen.

At one point, I was tempted to surface the fretboard.js Fretboard API to callers. However, it felt like an error-prone approach since nothing stops multiple callers from mutating a single Fretboard instance. I ended up making a "private" hook which is only used in the component.

Using the fretboard.js Fretboard API

I ran into a couple of surprises when styling dots. I don't consider these bugs.

  • Fretboard.prototype.render and Fretboard.prototype.style are not commutative. Calling Fretboard.prototype.style before Fretboard.prototype.render does not style the dots. This is probably working as intended, but it's not explicitly documented anywhere. I don't think this is a big deal, and it could probably be resolved by tweaking the documentation.

  • The Position context that is passed to the style functions is not strongly typed. I found it awkward to keep track of it myself. I also assumed that if something was calculable, that it would be present. For example, since I'm telling fretboard.js Fretboard the tuning, fret, and string, I expected the note to be present in the style function argument. However, based on the documentation, this is working as intended:

[style()] Applies the passed properties to selected positions (via the filter function parameter).

  • After landing on a React component API, it felt awkward to pass styled filter functions instead of passing positions that have styles attached to them. This issue might be inherent to developing with React since using non-primitives props is prone to unnecessary renders and side effects. There are already two ways to style dots (Fretboard configuration or Fretboard.prototype.style), and I don't think this issue necessitates another. I also don't think the concept of a position should be coupled with style because you may want to do calculations with positions that have nothing to do with styling.

Feature Requests

  • Surface the Fretboard options in the main export. Right now, it's only accessible from @moonwave99/fretboard.js/dist/fretboard/Fretboard.

  • Consider strongly typing the Fretboard.prototype.style functions.

  • Consider allowing the Fretboard constructor to optionally take a string | HTMLElement. This would remove the need for the caller to generate a unique ID for the target element.

  • Consider updating the documentation to call out the importance of the order of calling Fretboard.prototype.render and Fretboard.prototype.style.

Bugs

I didn't find any bugs. This is an awesome library!

moonwave99 commented

Hey that looks awesome!

In order:

React

I think the best way is to have a useFretboard hook. I can even consider preparing an "official" repo / doc for it.

Some may be interested in using .children as well - that may need a dedicated component:

<Fretboard options={...}>
  <Position string={6} fret={0}/>
  <Position string={6} fret={3}/>
  ...
</Fretboard>

style vs render

I am not satisfied with the semantics of the API myself. I think it may make more sense to split the filter operation from the styling - something like:

fretboard
  .selectPositions(({ degree }) => [1, 3, 5].includes(degree))
  .style({ dotFill: 'red'})

I don't like the dots/positions name clash as well, I think position should be all over the place.

The Position context that is passed to the style functions is not strongly typed.

I think generics may help here - we could do:

fretboard.style<MyPosition extends Position>({ ... });

And expose MyPosition inside the style functions so that you have (flexible) type strength.
Atm I made everything via optional fields.

Feature Requests

Surface the Fretboard options in the main export.

Ok

Consider strongly typing the Fretboard.prototype.style functions

See above

Consider allowing the Fretboard constructor to optionally take a string | HTMLElement. This would remove the need for the caller to generate a unique ID for the target element.

it is already the case ˆˆ the el option is string | BaseType (see d3-selection deftypes).
This allows to use a ref in React btw.

I will let you know when I manage to make any changes - PRs are welcome of course.

Keep it up with the good work, looking forward to seeing some progress!

  • ❤️1
jaredjj3 commented

Thanks for taking the time out to respond! I can't commit to a timeline, but I do plan to contribute fretboard.js.

I like the React API you proposed. The child elements inherently have an order to them, and you could render different layers at a time—the last position rendered "wins" in terms of style.

<Fretboard options={...}>
  <Scale root="E" type="major" style={...} />
  <Chord chord="022100" style={...} />
  <Position string={6} fret={0} style={...} />
  ...
</Fretboard>

Ultimately, I think the higher-level components are composed of Position elements, which the Fretboard component can figure out. After all, the elements are just glorified object wrappers in this context. This could be helpful in expressive use cases, so I'm going to try to do it.

I also like the selection API you proposed because it decouples the concern of rendering and selecting. One of the DX tradeoffs of this approach is the loss of Fretboard method chaining when styling since it seems like you'll need some intermediate object, but I think it's worth it.

fretboard.style<MyPosition extends Position>({ ... });

That generic would certainly clean things up, but I think the current state is good enough for the time being. It seems like a constant challenge to balance the caller's flexibility and type strength throughout the project, so I would rather wait to see if the API evolves into something that doesn't need generics.

Ultimately, each position has some contextual metadata. Some metadata is readily calculable (e.g. f(tuning, fret, string): pitch) while other metadata is conditionally calculable such as scale degree (i.e. does Fretboard know about a scale?). I don't think Fretboard should calculate this metadata, but this is just my gut feeling.

On the other hand, I'm having trouble discerning if it's a good idea to allow metadata to be attached to each position within the library or not. After all, the caller can do this on their end, using [fret, string] as a key since they're always available in the position context, but it may not be obvious to adopters. I need to think about this more.

jaredjj3 commented

I was able to achieve this React component API:

<Fretboard opts={fretboardOpts} tuning={tuning}>
  {measurePositions.map(({ string, fret }) => (
    <Fretboard.Position string={string} fret={fret} />
  ))}
  {pressedPositions.map(({ string, fret }) => (
    <Fretboard.Position string={string} fret={fret} style={pressedStyle} />
  ))}
</Fretboard>

The hardest part was creating a mechanism to associate a position with a single style. The reason why I imposed this constraint was to prevent styles from flashing when multiple styles were applied to a single position. While writing this, I realize that this may be the wrong solution because you might want to purposely apply partial styles at different layers. For example, the positions in the key of interest get a certain stroke color, while the pressed positions get a fill color, etc. I'll revisit this.

Another challenge was preventing unnecessary renders due to the inability to memoize children. To accomplish this, I added an abstraction encapsulating children and only updated that abstraction if it deeply changed between renders. The complexity overhead for this optimization is small, so I decided it was worth it.

I think the higher-level components are composed of Position elements

Now that there's a concrete solution, I want to elaborate on this from the previous comment. If you wanted to add something like a <Fretboard.Scale> component, then this hook would need to be updated to transform the props of a <Fretboard.Scale> to a StyleTarget[].

moonwave99 commented

Yeah React makes things unnecessary complex for real-time stuff like this - a more imperative API works better, at least a direct event-bus / pubSub strategy.

Layers: I thought about that, but since I haven't got a specific use case yet, I haven't worked on it.

If you don't want to get mad with styles, you can just pass a class attribute to the pressed positions, et voilà presto style them via CSS - what do you think?

jaredjj3 commented

Layers: I thought about that, but since I haven't got a specific use case yet, I haven't worked on it.

I can see myself using this to make guitar exercises in the near future. The caller specifies a style merging strategy: 'merge' | 'first' | 'last' which means merge the style attributes, pick the first style seen, or pick the last style seen respectively. Next, a style simulator applies that strategy to each of the styles and positions in order without touching the DOM and computes a Map<Style, Position[]>. Lastly, the map is transformed into style filters Array<{ predicate: (position: Position) => boolean, style: Style }> and applied to the Fretboard using the existing fretboard.js API, which I already know how to do.

I don't think the concepts of StyleLayer or StyleSimulator need to be part of a public API. They're just implementation details of merging styles. This seems easy enough to do, I'll give it a try.

If you don't want to get mad with styles, you can just pass a class attribute to the pressed positions, et voilà presto style them via CSS - what do you think?

This is the better solution for simple use cases. There's an ambiguous edge case when using multiple <Position> elements with the same fret and string, but different classNames (which className(s) are honored?). However this is no different than the styling problem now.

jaredjj3 commented

I got it to work for scales! The latest component code is here. Most of the interesting code is tucked away in helpers.ts

Here's an example of a merged styling layers use case (albeit poorly styled):

Screen.Recording.2021-09-23.at.8.49.06.PM.mov

Notes within the scale are always outlined in green. However, notes outside of the scale are outlined in red. What an intuitive way to learn about tension!

jaredjj3 commented

I've written tests for the component, so I'm closing the integration story on my end. I'm extremely satisfied with the way it came out. Thank you for all of your help!

Feel free to continue the discussion or close the thread.

jaredjj3 commented

I wanted to let you know that I've done a soft release of stringsync. Anyone can access it at https://stringsync.com. You may have to hard refresh your browser. I'm using the implementation we discussed in this thread.

I'd also like to know if you're interested in me adding the concept of guitar slides to fretboard.js. The implementation from the screenshot is a bit of a hack, but it is straightforward to officially implement.

image)

moonwave99 commented

Hey @jaredjj3 , that looks neat!

I like your implementation of slides, you could try to style the target positions lighter before reaching them - a bit of opacity may do.
Proper articulation display requires a lot of UX study I think - bends would be exciting for instance! Showing the target note (on the same string) while moving the actual fretted dot up or down, etc...

Most of all I think you have done an impressive work, and I am happy to have supported it somehow. Keep me posted!

And great guitar playing too!

jaredjj3 commented

Proper articulation display requires a lot of UX study I think - bends would be exciting for instance! Showing the target note (on the same string) while moving the actual fretted dot up or down, etc...

I agree. I found myself running into a lot of cases that aren't so straight forward to represent:

  • representing slide direction (arrow?)
  • sliding to/from "nothing" (color gradient towards the direction?)
  • showing when a slide is actually happening (opacity?)

I think context matters, too. For example, if you're in an dynamic context like stringsync, maybe you want to represent articulations differently than a static context. What I've seen in video games is making animations easily interpolatable. You provide the animation renderer an alpha value between 0 and 1, which describes the extent of the animation. Perhaps that could be leveraged to distinguish how to represent an articulation in static and dynamic contexts.

I think I should see how I concretely answer all these questions before proposing formal adoption within fretboard.js. I'm happy to make a PR for slides now if you think otherwise.

Most of all I think you have done an impressive work, and I am happy to have supported it somehow. Keep me posted!

Thank you for the library! I'd really like to give back by contributing directly to fretboard.js, so I will spend some time over the next few weeks to see if I can identify any opportunities.