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

Usage of FontFace rather than paths #1479

Closed
wants to merge 4 commits into from
Closed

Usage of FontFace rather than paths #1479

wants to merge 4 commits into from

Conversation

rvilarl
Copy link
Collaborator

@rvilarl rvilarl commented Nov 27, 2022

fixes #1127
fixes #1459

Personally I see two main benefits:

  • the library is smaller.
  • the produced SVG are also smaller.
-rw-r--r--. 1 rodrigo rodrigo 1911241 nov 27 22:06 reference/cjs/vexflow-debug.js
-rw-r--r--. 1 rodrigo rodrigo 1380174 nov 27 22:15 build/cjs/vexflow-debug.js

StaveNote with two noteheads with one accidental each.

<g class="vf-stavenote" id="vf-auto1300">
 <g class="vf-notehead" id="vf-auto1348" pointer-events="bounding-box">
  <text stroke="none" x="258.331" y="120"></text>
  <text stroke="none" x="232.347" y="120"></text>
 </g>
 <g class="vf-notehead" id="vf-auto1349" pointer-events="bounding-box">
  <text stroke="none" x="258.331" y="110"></text>
  <text stroke="none" x="245.339" y="110"></text>
 </g>
</g>

@ronyeh ronyeh changed the title Usage of FaceFont rather than paths Usage of FontFace rather than paths Nov 28, 2022
@ronyeh
Copy link
Collaborator

ronyeh commented Nov 28, 2022

As this is a big PR, it'll take time to properly review. Thanks for looking into this though!

Can you write up some extra info in this thread to help us with this review? What are the main tradeoffs?

This handles all of our font needs, whether we are using it to render chord symbols or music engraving symbols??

@rvilarl
Copy link
Collaborator Author

rvilarl commented Nov 28, 2022

As this is a big PR, it'll take time to properly review. Thanks for looking into this though!

Can you write up some extra info in this thread to help us with this review? What are the main tradeoffs?

The changes are simple and the review will be much quicker than what you anticipate :)

  • Concept: xxx_glyphs.ts files containing no path ('o' member) contains now a 'unicode' value. These unicodes will be printed with FillText.
  • Font has two new methods to measure text: textMetrics and measureText. Those replace TextFormatter. This is related to most of the changes. This method is quick enough and a text cache is no longer required.
  • Images generation with jsdom, I only managed to get the fonts loaded by node using RegisterFont.

This handles all of our font needs, whether we are using it to render chord symbols or music engraving symbols??

Yes both, and also, to write text. The SMuFL OTF files include characters for many languages (hindi,...)

@mscuthbert
Copy link
Collaborator

I don't ever manipulate the points of the path after its drawn, so that won't be a breaking change to me, but I do tend to change the fill-color (and opacity, etc.) of the SVG. Is that done with the same interface in this PR? It looks like an incredible space savings (especially since I can then load one copy of Bravura [or Gootville] for the whole site instead of two, but I wonder if it's backwards compatible enough to go into v4 or if it's more of a v5 thing?

@rvilarl rvilarl added the 5.0 label Nov 30, 2022
@rvilarl rvilarl marked this pull request as draft December 6, 2022 17:37
@rvilarl
Copy link
Collaborator Author

rvilarl commented Dec 6, 2022

Draft until #1476 is resolved, I want to integrate this PR in this one.

@rvilarl rvilarl mentioned this pull request Dec 22, 2022
@rvilarl rvilarl closed this Dec 27, 2022
@rvilarl rvilarl deleted the refactor/font branch December 27, 2022 08:11
@rvilarl rvilarl restored the refactor/font branch December 29, 2022 01:21
@rvilarl rvilarl reopened this Dec 29, 2022
@@ -84,15 +83,15 @@ export class Annotation extends Modifier {
let maxRightGlyphWidth = 0;
for (let i = 0; i < annotations.length; ++i) {
const annotation = annotations[i];
const textFormatter = TextFormatter.create(annotation.textFont);
const measure = Font.measureText(annotation.text, annotation.textFont!);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps refactor from measure to measurement? measure has a very specific implication in music applications.

@rvilarl rvilarl closed this Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants