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

VexFlow 5 #1498

Closed
ronyeh opened this issue Dec 23, 2022 · 46 comments
Closed

VexFlow 5 #1498

ronyeh opened this issue Dec 23, 2022 · 46 comments
Labels

Comments

@ronyeh
Copy link
Collaborator

ronyeh commented Dec 23, 2022

List your suggestions / requests in this thread!

Here are some of mine:

  • The master branch is renamed to main.
  • camelCase and UpperCamelCase / PascalCase throughout the code base.
  • Support all open SMuFL fonts (that are free for us to redistribute).
  • Transition to a VexFlow organization on GitHub: https://github.com/vexflow/vexflow
  • Host a font comparison tool (either on vexflow.com or github pages) so developers can quickly compare VexFlow-rendered scores between SMuFL fonts. (Maybe base it on our current image comparison tool).

main branch

The git command line now defaults to main, and many top projects have switched over to using main (e.g., TypeScript, React, QUnit, jQuery, Stable Diffusion)

See: https://github.com/github/renaming

For now, we will keep VexFlow version 4.x in the master branch for any important bug fixes. Eventually, we can tag the final long-term support version of 4.x and then retire / delete the master branch.

camelCase

For camelCase conventions, we follow the Google Typescript style guide:

https://google.github.io/styleguide/tsguide.html#identifiers

If anything is still unclear, we fall back to:

https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/capitalization-conventions

As an additional fallback, we can use this guide for contributors to the TypeScript compiler (even though Microsoft says to not use it):

https://github.com/microsoft/TypeScript/wiki/Coding-guidelines

SMuFL fonts

We will not redistribute commercial SMuFL fonts, but we can try our best to make VexFlow 5 compatible with them.

We will make it easy for a developer to choose which font(s) to include in their final vexflow build.

VexFlow organization

As the community grows, we can indicate our long term support for this library by housing the official repository under a GitHub "organization". We would want to get our founder Mohit's blessing for this 🤞😁. Maybe we can designate Rodrigo as the lead developer, since he has been doing much of the work for the past almost-two years (see: PR #851).

@ronyeh ronyeh added the 5.0 label Dec 23, 2022
@ronyeh ronyeh pinned this issue Dec 23, 2022
@mscuthbert
Copy link
Collaborator

I've thought about the organization thing before. If Mohit is good with it, then I think going for it is the best thing for the project. (but it's also cool if he's not. I haven't moved music21 out of "cuthbertLab" yet, but it's clearly bigger than just my lab now. It's something to start talking about).

For master to main -- I wish github made it even easier, but something to consider. Again something I've wanted to do, but it's a bit hard.

For camelCase, etc. -- I'd do it in two stages -- (1) make all public interfaces conform to it (2) make private variables/internal conform. They can be done in either order, but definitely do them separately, since they're really two different issues. Note that underscore_case is generally considered more readable for people who are not fluent in English and people who use screen-readers (blind programmers, etc.). In music21 because we started with camelCase (a remnant of Perl, which was "wrong" in the Python m21 version but is now "right" in music21j) we've kept with it, but I've started moving/writing new code with private variables to underscore case for the accessibility. If you want a strong distinction between public and private interfaces then doing public as camelCase and private as underscore_case works very well.

@mscuthbert
Copy link
Collaborator

I think that a significant (at least internal) change that I'd like to look at for v5 is passing and storing Classes instead of Structs in more places. I've noticed that, for instance, TimeSigNote creates a TimeSignature object in order to parse the information given to it, takes the struct from it, and then throws out the original TS object. Same happens with Clefs, etc. in a lot of places, so that there's not an opportunity to use the useful methods on those objects later. I think that the use of many structs in internal code is one of the main reasons why we're still lacking in getters and setters for a lot of protected class properties.

Also hoping that some of the formatter code that expanded between 1.2 and 4.2 can be abstracted out into smaller, more testable code. Turning on and off different parts of the format code would be amazing.

VF 4 has been amazing! 5 should be just as great. Thanks!

@mscuthbert
Copy link
Collaborator

Another change I'd like (primarily internal so could go in 4.3 etc.) is to rename any attributes that are misleading about what they contain. For instance:

Formatter().modifierContexts = []

looks like it should be a list of ModifierContext objects. Instead it's a list of AlignmentModifierContextses structs which themselves have ways of storing/retrieving ModifierContext objects.

(btw -- I'm in awe of https://github.com/0xfe/vexflow/wiki/How-Formatting-Works thanks Ron! But it's definitely a place where we can keep adding more docs (in the code too) -- I'm still not 100% sure of what a resolutionMultiplier is or why it needs to be calculated at the point that it is calculated).

@ronyeh
Copy link
Collaborator Author

ronyeh commented Jan 1, 2023 via email

@rvilarl
Copy link
Collaborator

rvilarl commented Apr 3, 2023

@mscuthbert @ronyeh @AaronDavidNewman @sschmidTU I have created a branch vexflow5 to share with you how I am progressing on using fonts directly.

The size of vexflow.js is 528K compared to the 995k of the one in master. More than 400k less. :)
And gonville is still in, it will be less once the gonville metrics are unified.

@sschmidTU
Copy link
Contributor

@rvilar Nice! But one will have to add the OTF of one of the fonts to this build, right? So how large is build+font if you add one of the default fonts?

@rvilarl
Copy link
Collaborator

rvilarl commented Apr 5, 2023

No the fonts are either installed in the computer or downloaded as OTF directly from a file or from internet using FontFace.

@sschmidTU
Copy link
Contributor

That's what I was asking for. We can't presume that these fonts are installed on every user's computer, right? So we should calculate the final size of vexflow for usage as the release + the OTF for the font you're using, if you're only using one font.

@rvilarl
Copy link
Collaborator

rvilarl commented Apr 5, 2023

Well the music fonts will be downloaded from a cdn or similar. They are normally loaded once and stay in the browser cache. The size will depend on the font and the format woff2, otf,...

@rvilarl
Copy link
Collaborator

rvilarl commented Apr 5, 2023

The same as the text fonts today.

@sschmidTU
Copy link
Contributor

sschmidTU commented Apr 5, 2023

I still wanted to know the size of the main OTFs, so I looked them up:
GonvilleSmufl - 210KB
Bravura_1.392 - 501KB
BravuraText_1.393 - 1208KB
(from the current state on master, tools/fonts/@ directories)

One more reason to use Gonville.
Do you need BravuraText as well to use Bravura in Vexflow 5?

Not everyone wants to download otf files from a CDN for their web app (which might be offline, e.g. Android App). And still it's bandwidth that's used to load vexflow (even if it's just once, in case the browser caches it), since vexflow doesn't work without a font, so I would include these sizes in the full vexflow build size, at least for looking at maximum bandwith for first full load.

@rvilarl
Copy link
Collaborator

rvilarl commented Apr 5, 2023

As size I can only provide the one of vexflow.js as the fonts is an user decision. BravuraText is recommended with Bravura but you could simply use Arial if you wanted. An option is to load WOFF2 rather than OTF, I will look into the sizes. The fonts files can be placed also in the Web server they do not have to come from a CDN.

@ronyeh
Copy link
Collaborator Author

ronyeh commented Apr 5, 2023

@sschmidTU BravuraText is not needed for deploying a vexflow.js that does not contain any glyphs. You would just need the OTF or WOFF file for the music symbols, which are contained in GonvilleSmufl or Bravura_1.392 or Petaluma.

BravuraText is for typesetting symbols in text-heavy documents, like if you were to write a Word doc for a masters thesis about music, you could use BravuraText to include the symbols in your document.

@ronyeh
Copy link
Collaborator Author

ronyeh commented Apr 5, 2023

@rvilarl In VexFlow 5 we would need to indicate the total download sizes including the font that the user chooses.

VexFlow Library + Bravura Font File = ???
VexFlow Library + Gonville Font File = ???
VexFlow Library + Petaluma Font File = ???

etc....

@sschmidTU The main benefit of separating the font file from VexFlow is that you could load the WOFF/OTF separately and use it on your webpage or with another library, and the font file would not have to be downloaded a second time when your page loads VexFlow.

Currently, if you use a different library that downloads the Bravura OTF, you are making the user download Bravura twice because VexFlow includes all the Bravura glyphs.

@rvilarl maybe someday we need a tool that allows the developer to easily create a subset of Bravura. If they only use a small subset of common music symbols, the tool could create a subset Bravura that includes only the symbols used in their scores.

@sschmidTU
Copy link
Contributor

sschmidTU commented Apr 5, 2023

Yes, ronyeh, I knew most of the benefits of using OTF files (still, thanks), but as I was saying from the beginning, I want to know the total size including the OTF file(s), because I still think the comparison of this total size with e.g. Vexflow 3 or 1.2.93 is interesting and relevant. How much space/KB does it take to include vexflow in my (say, offline) project?
So, there are two different topics, why and how OTF files are used, and how large the total file/build/download sizes are including OTFs, and I'm currently rather interested in the second one.

@ronyeh
Copy link
Collaborator Author

ronyeh commented Apr 5, 2023

We already have a version in VexFlow 4.1 that can load fonts dynamically.

https://cdn.jsdelivr.net/npm/vexflow@4.1/build/cjs/vexflow-core.js

Google chrome inspector says it is 93.4 KB over the network (with Content-Encoding: br).

The actual uncompressed file size after it is downloaded is 345 KB.

Add Bravura OTF: + 513 KB

Add Bravura WOFF2: + 247KB

Add GonvilleSmufl OTF: + 219KB

@ronyeh
Copy link
Collaborator Author

ronyeh commented Apr 5, 2023

The vexflow version that bundles all fonts is at:

https://cdn.jsdelivr.net/npm/vexflow@4.1/build/cjs/vexflow.js

315KB compressed
983KB uncompressed

@ronyeh
Copy link
Collaborator Author

ronyeh commented Apr 5, 2023

Gonville only: https://cdn.jsdelivr.net/npm/vexflow@4.1/build/cjs/vexflow-gonville.js

144KB compressed
498KB uncompressed

@rvilarl
Copy link
Collaborator

rvilarl commented Apr 5, 2023

We already have a version in VexFlow 4.1 that can load fonts dynamically.

https://cdn.jsdelivr.net/npm/vexflow@4.1/build/cjs/vexflow-core.js

Google chrome inspector says it is 93.4 KB over the network (with Content-Encoding: br).

The actual uncompressed file size after it is downloaded is 345 KB.

Add Bravura OTF: + 513 KB

Add Bravura WOFF2: + 247KB

Add GonvilleSmufl OTF: + 219KB

GonvilleSmufl WOFF2: +21KB

@mscuthbert
Copy link
Collaborator

One reason for sending as core and being able to load a font separately is that a site may use the font in Vexflow and outside it. For instance (no big secrets here): Artusi loads Bravura separately to use in diagrams and questions and then also loads a music font in Vexflow. If we could consolidate them so we load one font once, that would save 500KB.

Of course it's all moot once you embed a single video into the page; then all your other work on space savings is a rounding error.

@rvilarl
Copy link
Collaborator

rvilarl commented Apr 6, 2023

I have committed GonvilleSmufl.woff2 together with a converter convert.py to master

@rvilarl
Copy link
Collaborator

rvilarl commented Apr 12, 2023

@mscuthbert @ronyeh @AaronDavidNewman @sschmidTU I have created a branch vexflow5 to share with you how I am progressing on using fonts directly.

The size of vexflow.js is 528K compared to the 995k of the one in master. More than 400k less. :) And gonville is still in, it will be less once the gonville metrics are unified.

Now without Gonville 392K, more than 600k less :). I will provide in the end the overall traffic including fonts.

@rvilarl
Copy link
Collaborator

rvilarl commented Apr 24, 2023

I have made a step forward making all the music fonts glyphs info and metric common and now the library has been reduced to 318K :)

Now I am able to use other SMUFL just providing the font name (font must be installed)

Two new fonts with Bach demo

Gootville
pptr-Bach_Demo Minuet_1 Gootville
MuseJazz
pptr-Bach_Demo Minuet_1 MuseJazz

@mscuthbert @AaronDavidNewman @ronyeh @sschmidTU @jaredjj3 There are problems to resolve, but it is looking good, isn't it? :)

@mscuthbert
Copy link
Collaborator

A little boxy no? :-D I don't have the fonts installed in my browser.

Screenshot 2023-04-25 at 10 56 06

@rvilarl
Copy link
Collaborator

rvilarl commented Apr 25, 2023

Yes you are right 🤣🤣 very boxy if the fonts are not installed.
The browser recognizes the fonts installed in the computer.

@rvilarl
Copy link
Collaborator

rvilarl commented Apr 25, 2023

PNG versions
Gootville
pptr-Bach_Demo Minuet_1 Gootville svg
Musejazz
pptr-Bach_Demo Minuet_1 MuseJazz svg

@rvilarl
Copy link
Collaborator

rvilarl commented May 9, 2023

@sschmidTU the network load. The prevexflow5 vexflow.js is now 290K

The fonts below, thanks @ronyeh. With GonvilleSmufl 290k + 24,5k = 314,5K

image

@rvilarl
Copy link
Collaborator

rvilarl commented May 9, 2023

How the fonts look... Thanks again @ronyeh :)

image
image
image
image
image
image
image
image
image
image

@rvilarl
Copy link
Collaborator

rvilarl commented May 9, 2023

I just pushed a refactor of accidental and the library is now 274K :), @sschmidTU with GonvilleSmufl now less than 300k! I am using now filltext to add the parenthesis on the cautinary accidentals. They are also more compact.

image

@ronyeh
Copy link
Collaborator Author

ronyeh commented May 11, 2023

Once we wrap up #1561, are there any other issues you want to resolve for version 4.2, @rvilarl ?

Maybe we can do a review of all the output and only fix tiny visual bugs... so that we can ship 4.2.

Then, it would be a good time to temporarily "freeze" development on 4.x so we can move it to an organization as 5.0 dev branch. Then we can submit each of your new improvements as separate PRs to the 5.0 main branch.

Does that sound OK?

@rvilarl
Copy link
Collaborator

rvilarl commented May 11, 2023

We can even leave #1561 out
The rotation is only really needed for using OTFs directly

That sounds very good.

@rvilarl
Copy link
Collaborator

rvilarl commented May 11, 2023

prevexflow5 now uses the glyphs gClef8va gClef8vb fClef8va fClef8vb dierctly this reduces the complexity of the code and I believe that it looks better. Also adding support for the 15ma/b variants is trivial.

image

@rvilarl
Copy link
Collaborator

rvilarl commented May 18, 2023

Down to 269K :) and almost there to merge font and glyph.

@rvilarl
Copy link
Collaborator

rvilarl commented May 19, 2023

Glyph is out, camelCase in: 264k :)

@ronyeh
Copy link
Collaborator Author

ronyeh commented May 19, 2023 via email

@rvilarl
Copy link
Collaborator

rvilarl commented May 21, 2023

@ronyeh preVexflow5 in my fork.

https://github.com/rvilarl/vexflow/tree/preVexflow5

@rvilarl
Copy link
Collaborator

rvilarl commented Jul 2, 2023

How do we want to proceed with Vexflow5? Are we moving to an organization? I am about to publish what could be the first alpha release. 245K. Do we want a branch or a new repo?
@ronyeh @mscuthbert @sschmidTU @0xfe @AaronDavidNewman @gristow

@0xfe
Copy link
Owner

0xfe commented Jul 2, 2023

Hey Rodrigo, I created the github.com/vexflow organization last year and invited some of you as maintainers. Looks like not everyone accepted, however Ron has been keeping it updated. I'll resend the invitations. Please accept. (Also please make sure you have 2FA included in your github accounts.)

@rvilarl
Copy link
Collaborator

rvilarl commented Jul 3, 2023

Invitation accepted. I have also placed the work I have been doing in the branch vexflow5

https://github.com/vexflow/vexflow/tree/vexflow5

I believa that it is very advanced but with some bugs to resolve.

@rvilarl rvilarl closed this as completed Jul 3, 2023
@ronyeh
Copy link
Collaborator Author

ronyeh commented Jul 3, 2023

Hey, I'm reopening this thread just so I can document the migration from the VexFlow 4 repository to the VexFlow 5 repository.

Thanks Rodrigo for your work pushing VexFlow forward.

Here's my plan. Let me know if this sounds OK:

  1. Add a few more npm run scripts to address the comments in Visual Regression tests doc issues #1508
  2. Release 4.2.2 which includes the recent PR fix: extra space before seconds with modifiers #1580 from @gristow
  3. Clone this repo and push it up to the VexFlow 5 repo, but rename the master branch to main, as we discussed previously.
  4. Start sending in PRs to the VexFlow 5 repo (from my branch and also from your branch).
  5. We will leave issues as is in this repo, but if it is something we want to solve in VexFlow 5, we can copy the issue over and reference the issue in this repo. Once the VexFlow 5 issue is closed, we can also close the issue in this repo (but reference the related VexFlow 5 issue).

@rvilarl I think we still need a short wiki document highlighting the main changes in 4.2 that you worked on. This will help people understand why they should upgrade from 4.1 to 4.2, and what to expect when doing so.

@ronyeh ronyeh reopened this Jul 3, 2023
@ronyeh ronyeh unpinned this issue Jul 3, 2023
@ronyeh
Copy link
Collaborator Author

ronyeh commented Jul 3, 2023

I released VexFlow 4.2.2 with the most recent PRs. Let me know if you encounter any issues.

I started the VexFlow 5 repository from this recent commit:

vexflow/vexflow@58206a6

58206a6

The vexflow/vexflow and the 0xfe/vexflow repos both share the same commit hash 58206a6.

The main branch is now called main.

This is a detached fork, which has benefits and drawbacks. The main benefit is we can treat this as a "new" product and not worry as much about major breaking changes. This will enhance Rodrigo's ability to explore support for all the SMuFL fonts, etc. The main drawback is that this repo does not have as many stars/watches/forks since we are just starting out. But hopefully, VexFlow 5+ will be awesome enough someday that we will be able to convince people to switch over!

@rvilarl Can we move your vexflow5 branch into your own personal repository? We have multiple maintainers in the vexflow organization, so it is possible that we will accidentally delete each others' work if we all store our working branches in the organization's repo.

I think the best approach is to stick with a single branch main. We can all submit PRs to this branch like before.

On my own personal GitHub account, I renamed my vexflow repo to vexflow4, and then I forked this new repo. So now my vexflow repo points to the new VexFlow 5+.

@ronyeh ronyeh pinned this issue Jul 3, 2023
@rvilarl
Copy link
Collaborator

rvilarl commented Jul 3, 2023

@rvilarl I think we still need a short wiki document highlighting the main changes in 4.2 that you worked on. This will help people understand why they should upgrade from 4.1 to 4.2, and what to expect when doing so.

@ronyeh please point me to an example in the wiki to follow the same approach.

@rvilarl
Copy link
Collaborator

rvilarl commented Jul 3, 2023

@rvilarl Can we move your vexflow5 branch into your own personal repository? We have multiple maintainers in the vexflow organization, so it is possible that we will accidentally delete each others' work if we all store our working branches in the organization's repo.

To do the changes on a PR based approach will take a lot of time. I suggested to review by looking at visual changes. I have placed the branch here to let you all look at the approach and get a general feedback. The org repo is also not forkable.

@ronyeh
Copy link
Collaborator Author

ronyeh commented Jul 3, 2023

@rvilarl I think we still need a short wiki document highlighting the main changes in 4.2 that you worked on. This will help people understand why they should upgrade from 4.1 to 4.2, and what to expect when doing so.

@ronyeh please point me to an example in the wiki to follow the same approach.

https://github.com/0xfe/vexflow/wiki/Changelog-ver-4.0

https://github.com/0xfe/vexflow/blob/master/CHANGELOG.md

It doesn't have to follow this format. Just add a bullet list of what you believe are the most important features you contributed to this 4.2 release. Since it is a wiki page, we can also help out. You can even include links to a few important PRs.

@ronyeh
Copy link
Collaborator Author

ronyeh commented Jul 3, 2023

@rvilarl Can we move your vexflow5 branch into your own personal repository? We have multiple maintainers in the vexflow organization, so it is possible that we will accidentally delete each others' work if we all store our working branches in the organization's repo.

To do the changes on a PR based approach will take a lot of time. I suggested to review by looking at visual changes. I have placed the branch here to let you all look at the approach and get a general feedback. The org repo is also not forkable.

Sorry about that. It is now forkable.

I would recommend doing the PR approach (the classic way of submitting PRs from our forks to the VexFlow organization repo).

I think it would lend extra credibility to your hard work if the changes were publicly reviewed & discussed (and at least looked at by another set of eyes).

@ronyeh
Copy link
Collaborator Author

ronyeh commented Jul 4, 2023

Going to close this one, as active work on version 5+ as moved to: https://github.com/vexflow/vexflow/

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

No branches or pull requests

5 participants