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

Allow staff visualizers to be split into multiple lines #79

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Cheezer1656
Copy link

Added a line parameter to the midi-visualizer element that determines how many lines the notes are split into.

Also updated some packages to fix some vulnerabilities.

@cifkao
Copy link
Owner

cifkao commented Apr 16, 2024

Looks like a cool contribution. Do you have a demo I could look at?

Also, please don't update yarn.lock/package-lock.json in the same PR. These are long diffs and it's hard to see what's going on there. I would prefer if you could make an issue listing what needs to be updated and why, and I can take care of it.

@Cheezer1656
Copy link
Author

Sure! I removed the yarn.lock and package.lock changes and added an example of the multi-lined visualizer to the demo (index.html). Is this ok?

@cifkao
Copy link
Owner

cifkao commented May 23, 2024

Sorry, it took me a while to get around to this! Thanks for the demo, I now see better what this is doing.

I agree it would be useful to have the staff visualizer split into multiple lines. However, I see a couple of issues with the approach:

  • Splitting evenly by notes (in the middle of a bar) seems inadequate:

    In sheet music, breaks should always occur at downbeats.
  • I guess the reason why we would want this feature is for the score to fit on screen without having to scroll horizontally. So ideally it should take in a desired maximum width or just fit the container responsively, rather than being based on a specified number of lines (which would require trial and error on the user part to get it right, and it would work only for one piece of music, as can be seen from the demo). (Another possibility would be to specify the number of measures per line, but even that isn't ideal as measures within one piece can have different widths.)
  • The functionality makes sense for the staff visualizer, but not so much for the other visualizer types (especially the waterfall one). For this reason, I'm not a fan of having this as a property on all visualizers.

In short, it seems intricate to do this right and it would require some interaction with the actual score rendering logic, which is not accessible from here. Therefore I think it would be preferable to add this feature directly to staffrender, which is what is used to render the scores. I think it's better to keep the visualizers here as thin wrappers around the ones defined in @magenta/music (unless we fully implement a new visualizer type here, which would also be fine).

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

Successfully merging this pull request may close these issues.

None yet

2 participants