-
Notifications
You must be signed in to change notification settings - Fork 666
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
Question: Remove TextFormatter - with Bravura and Petaluma as text #1466
Conversation
@sschmidTU @AaronDavidNewman @mscuthbert @ronyeh what do you think? |
I'm out of town for a bit, but I see 2 things are wrong with these test cases:
A better test would be to measure the length of different strings, e.g.: AVo(i)a etc. The first text would measure the consequence of kerning. I'm guessing this would be the least accurate. The last 2 represent the widest (of the ASCII characters) and the second measures a string of thin letters. The second one is more 'general case' and would measure the accuracy of a long string without the wildcard of spacing (which can always be controlled by change changing the start x). When I did this test with Smoosic, all the measurements were within a pixel (almost): 1st string 2nd string 3rd string 4th string |
I used the same font used by
There are now single spaces, the leading space on the second text is not preserved with svg. I have not managed to get it right using 'space=preserve' attribute
|
I think the leading space will be stripped, also. Try putting the space in the string for your second example.
The only font metrics we have with the distro of vexflow are PetalumaScript and Roboto Slab. If you are using a different font family, the metrics and the font won't match. Most of our test cases use 'SERIF' or 'Times New Roman' and we don't include the metrics for those. That is why your test cases are so off. Text font metrics are extracted from the font file, just like with music font. A couple of differences between text fonts and music fonts:
When you create a 'TextFormatter' object, the logic looks for the best fit of the metrics that were registered. When I return from vacay, I can add a SERIF to the text fonts, and then your test cases should match up almost exactly. Or, you can just set your font family to 'Roboto Slab'. |
We could also add a |
I did put spaces in the long string and they are not ignored.
None leading single spaces are working well. I believe that we have to avoid leading spaces, they are also ignored by FillText By the way I am also out of town for a bit. In India :) |
@AaronDavidNewman I use now a canvas to measure text, could you confirm that the speed is good? |
I have been looking further into the implementation and my recommendation would be to remove
|
I'd prefer to try to make TextFormatter more accurate/functional, vs. killing it altogether. So I will say a few things in defense of it, the 'marketing slides', I guess. Smoosic uses it quite heavily and if it is removed, I will carry it over to my private fork, so I am a little self-interested. TextFormatter was created to hold information about fonts and glyphs within fonts. The most common bit of information is the width of a block of text. But there is other information you might want. One is the width of a character vs. its advanceWidth. advanceWidth is the place the next character would go, which is what getBBox gives you, and is often what you want. But if you are trying to place text and glyphs together, as in the case of ChordSymbol, you care about the actual width. Another thing you might care about is the bearing, which is where the character starts vs. the bounding box. When trying to fit symbols above/below the '/' in a chord symbol or notation, you would want that. Likewise with the height. When you are creating something like a time signature, you want the height of the glyph and its offset which the bounding box won't give you. The same is true with superscript/subscript. When drawng text, you sometimes want the height of the tallest character, and other times the actual height of a character. There are also kerning hints which we may want both for specific letter combinations and glyph/font combinations. If we remove TextFormatter, there is nowhere for this information. We don't extract kerning hints from the font right now but we could, it has always been my intention and the tools we use (OTF) support it. The other thing, and maybe this is more of a philosophy, is that drawing and measuring seems like cheating. All the other drawing done in VexFlow is done by using the glyph metrics. TextFormatter is the equivalent for text fonts. VexFlow isn't really adding any value if all it does is draw and measure the fonts, because anybody can do that. We want to provide functionality for people to build their own music applications. And of course, it is very convenient to be able to measure something and decouple it from the rendering. The speed is part of it, but also the issue of controlling when the DOM is accessed. With the canvas trick, you need to create a DOM element outside of the drawing element, and right now there are no assumptions made beyond the stuff that is #boo. Applications that support inline text editing and dragging/resizing of text elements (like smoosic), the draw and measure approach is more cumbersome. |
This why I added |
Anyway I close it, I do not want Smoosic to diverge from vanilla VexFlow |
Let's bring it up next release. I didn't notice the changes made to Font - I thought it was still for music font glyphs. If there is duplicate functionality to TextFormatter, we can certainly remove it and I can get the data I need from Font. At the time I created TextFormatter (it was called something different then, maybe TextFont), Font was only for glyphs from the music fonts and metrics for text fonts weren't available. We might still be able to use the measure trick for the cases where we only care about the bounding box of a text block, like in Annotation. I'm a little uncomfortable adding DOM elements outside of the one created in the renderer, but maybe we can find a way around that. |
@AaronDavidNewman probably it would make more sense to have the fonts separated in MusicFont and TextFont |
Yes, that's a rabbit hole for sure (it took me over an hour just to type this) but it would be nice if we could organize font functionality for text, music or both. FontModule, FontData and FontGlyph are sort of mixed use between text and music fonts. FontMetrics is very MusicFont specific and is part of FontData. We get some commonality out of the loading functions, thanks to Ron, so it would be nice to keep those. And also converting font sizes is pretty common to both. Since any application that uses Vexflow will need to do these things, it makes sense to keep them in Font.ts. We could move the stuff out of Font.ts that is clearly only for text, and moving it either into TextFormatter or a TextFont file. Music fonts don't have font-weight and font-style for instance. And the text-specific font loading stuff. It's a rabbit hole because there are things like SANS_SERIF which is clearly text-only but is used all over the place (since most elements do some stuff with text). Same with FontInfo which is used in about 70 places, but doesn't have anything to do with music fonts. Pretty straightforward, but a lot of typing. Maybe something to do early on in the next release cycle. |
Hello all! Should we have to preprocess a font to extract the metrics?Ideally, no. We do this for performance reasons, but if we could load a font and extract the necessary metrics fast enough for the desired task, then we could avoid having to preprocess fonts. For example, opentype.js parses font files and can extracts metrics. It is possible to do the same in VexFlow at runtime, but we haven't investigated the performance impact. Rodrigo discovered another technique, which is to use the canvas methods to draw the text into an offscreen buffer to get more exact measurements. If we can do either of the above options fast enough, we would not need the TextFormatter approach. I am not actually against the idea of using canvas methods when measuring text for the SVG context. We would just need to fall back gracefully. If the rendering environment does not have a canvas (i.e., if document.createElement('canvas') fails), then we need to fall back to our current approach. I do think a more general approach would be nice. I think it's a limitation that VexFlow currently only supports text measurements for PetalumaScript & Roboto Slab out of the box. I understand there are existing apps like Smoosic that rely on TextFormatter. If we ever find a more general / performant / backward compatible solution for measuring text, we should be careful to have a pathway for Smoosic to transition to the new way of measuring text. Music Fonts vs. Text FontsIn my efforts to clean up VexFlow's handling of fonts, I came to the conclusion that "a font is a font." Yes, there are some OTF fonts that we just use for engraving music ("Music Fonts"). There are some fonts we use for drawing chord annotations or lyrics ("Text Fonts"). But there are actually SMuFL fonts out there that are music fonts but designed for display in a paragraph of text. For example: Bravura Text https://github.com/steinbergmedia/bravura/blob/master/redist/bravura-text.md The reason we preprocess the music font files is so that we can more easily scale and offset the glyphs on our staves. Aaron used the same approach when adding support for laying out chord symbols (e.g. C7) with fancy web fonts. Knowing the sizes of all the fonts ahead of time makes it a bit easier. However, this approach cannot support all fonts and all glyphs, because we need to do this preprocessing work. It would be nice to someday have a (high performance) approach where we can measure the glyphs (of Text Fonts or Music Fonts) at runtime so that we can lay out chord symbols or music engraving symbols without having to preprocess the font files. :-) If anyone wants to investigate it, go for it! I'd be happy to offer advice wherever I am able. |
Hi Ron, the canvas approach is very fast, I provided the figures. This is why CanvasContext has no cache. 🙂 |
I believe it! We can reintroduce your PR in the next release if you think
it has lots of benefits.
However, I would ask that we not break any of the demos and make sure it
falls back gracefully to the current implementation if a canvas is not
available.
…On Mon, Oct 31, 2022, 10:10 AM Rodrigo Vilar ***@***.***> wrote:
Hi Ron, the canvas approach is very fast, I provided the figures. This is
why CanvasContext has no cache. 🙂
—
Reply to this email directly, view it on GitHub
<#1466 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAB2MCMDGDEJRCNDWDJILVTWF74PHANCNFSM6AAAAAARLMPNOU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
We should probably move this conversation somewhere other than a closed PR :) When the TextFormatter has the correct font metrics, here are the results of the accuracy test. Green line is TextFormatter. In fact, if you measured each character individually using The tool that extracts the text metrics actually uses opentype.js (an older version). We could read the font data at run time, but the limitation there is you would have to be in a secure context due to CORS. The other limitation is the built-in fonts for the browser - we won't have access to this font files directly, so we would still need the textMetrics approach. Also, you'd be sending the font to the browser twice - once over xhr to parse and once for the browser as a font. This is kind of why we went with the metrics approach. Note that Bold is actually a different font-face for this font, so it needs different metrics. btw I like the accuracy test and I think I'll add another case that puts boxes around the text. |
Aaron can you open an issue and reference this closed PR? I think this is a
useful conversation... re: font measurement accuracy and speed and
backwards / forwards compatibility.
…On Mon, Oct 31, 2022, 9:11 PM Aaron ***@***.***> wrote:
We should probably move this conversation somewhere other than a closed PR
:)
When the TextFormatter has the correct font metrics, here are the results
of the accuracy test. Green line is TextFormatter. In fact, if you measured
each character individually using measureText it should be exactly the
same. The longer line for the first example is due to kerning of the 'AV'.
We could fix this by using the kerning data from the font in TextFormatter.
The tool that extracts the text metrics actually uses opentype.js (an
older version). We *could* read the font data at run time, but the
limitation there is you would have to be in a secure context due to CORS.
The other limitation is the built-in fonts for the browser - we won't have
access to this font files directly, so we would still need the textMetrics
approach. Also, you'd be sending the font to the browser twice - once over
xhr to parse and once for the browser as a font. This is kind of why we
went with the metrics approach.
[image: image]
<https://user-images.githubusercontent.com/5438280/199155289-aad304e3-69eb-4543-ab17-4daca96222b9.png>
Note that Bold is actually a different font-face for this font, so it
needs different metrics. btw I like the accuracy test and I think I'll add
another case that puts boxes around the text.
—
Reply to this email directly, view it on GitHub
<#1466 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAB2MCI4FIOXMROM4GAVJO3WGCJ63ANCNFSM6AAAAAARLMPNOU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Can we use this issue? This is what started me down the road to move some of the text measurement to use the TextFormatter instead. I can change the title, because what we want is to be able to measure text quickly and accurately, regardless of the mechanism. |
Sounds good to me. Measure text quickly and accurately!
…On Tue, Nov 1, 2022, 7:43 AM Aaron ***@***.***> wrote:
Can we use this issue?
#1459 <#1459>
This is what started me down the road to move some of the text measurement
to use the TextFormatter instead. I can change the title, because what we
want is to be able to measure text quickly and accurately, regardless of
the mechanism.
—
Reply to this email directly, view it on GitHub
<#1466 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAB2MCLAJOQBZNABONUC4KDWGEUBDANCNFSM6AAAAAARLMPNOU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
I did this, feel free to copy the parts of this discussion you want in the issue. I tried to summarize the discussion, feel free to edit that also. |
Now with Bravura as text |
New test to show the accuracy error compared to ctx.measureText.
This test also reveals that SVGContext.fillText & SVGContext.measureText are ignoring the leading spaces.