-
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
Remove textformatter, associated metrics and cache. #1546
Conversation
I'll have to investigate it a little more. A few of these looked slightly worse to me on your branch vs. what I have. I'm not sure, though, if your changes caused this. It looks like you are getting a larger value for the y extent in some cases. Other than these, I don't notice a difference in performance or visually. I checked stringnumber, tabtie, and some other text-placement and they seem the same. |
@AaronDavidNewman what do you get running flow.html? |
You guys have fast computers. This doesn't prove anything, though. Most of these test cases don't use the text formatter very much. Most of the CPU time, from what I've measured in the past, is spent rendering glyphs. And the unit test cases are for visual regression, not for measuring performance. So I don't see any evidence that the canvas measuring method performs any better or is any more accurate. I don't see that it is any worse, except in the cases I've pointed out. Here's another one: Gaps between the glyphs and the text tend to be either too large or too small. I don't know if this is a problem with the measurement method you are using, or just a bug in the changes you made to chord symbol. Possibly you are using the full bounding box, when what you really want for the text is the advanceWidth. |
@AaronDavidNewman it seems that we are using a different master mine has 9942 tests. |
It looks like you are appending some horizontal space at the end of each block, somehow. And it seems to be worse, relatively, at smaller font sizes. That poor '11' in the last row... There's extra vertical space, too, in the annotation examples. |
@AaronDavidNewman I have removed the scales |
@AaronDavidNewman I believe that I solved the problem with the poor '11' I have increased a bit the spacing but this could be reduced, what do you think? |
Some things are better, some are worse. Now there's a huge gap between the '#' and the '7'. I think what you are doing is not going to work. You have thrown out a key piece of information that you need to format text properly, which is the advanceWidth. For instance, if you measure 'F7' you end up with a smaller width than if you measured 'F' and '7' individually. This is why the F7 chord looks much better than the Ab7(#11). You might be able to work around it by using the following trick: whenever you measure a single letter, measure it as double, e.g. instead of 'F' measure 'FF and divided the size by 2. This will give you something closer to the actual width of the letter 'F'. I suspect that you're still not going to do as well as using the actual metrics information, and preferably the hints as well, from the font itself. I feel like you would have more success going in the opposite direction - enhance the metrics information for the special cases, and not just for text. But that's outside the scope of this PR so I'll stop... |
@AaronDavidNewman I am trying to move to runtime calculations rather than tables in order to allow in Vexflow5 the usage of fonts without having to modify the code. Your hints on elements to improve is helping me a lot thanks! I will not merge the PR if I do not get it right but I think that it is worth trying to achieve the Vexflow 5 vision: "Font independent code" |
@AaronDavidNewman I think that now the chords are fine. |
@AaronDavidNewman thanks for the feedback and patience! The different sizes are related with removing the scales of the chord symbols from the metric files. The accidental sharp was scaled to 0.75. Setting in back (only the sharp) this is the result |
Definitely moving in the right direction, although now the distance from '#' to the '1' is creeping up again. It doesn't looks like you've pushed your changes, so I can't evaluate the other test cases. On the chord symbols test cases, the text formatter version is consistently the same speed or a little faster than the non-chord symbols version. So the measure version is definitely not faster, but probably not noticeable. |
@AaronDavidNewman I wanted to avoid different scales in order to keep the creators ratio between the different glyphs. In master there are scales between 1 and 0.6 for the chords. Most of them are 0.8. I will change to 0.8 for all and see how it goes. |
@AaronDavidNewman now pushed with a general scale of 0.8 |
Just checking in with @AaronDavidNewman Is the general idea of removing TextFormatter OK with you? I feel like your app is the one that relies on this the most. Is there anything that Rodrigo's PR misses, that would break your app if we removed the text font metrics and glyphs? I feel like in an ideal VexFlow, we would be able to rely on external OTF or web font files, and that VexFlow wouldn't need to embed the paths. Let us know your concerns! It's important to flesh out all the pros and cons here. |
@ronyeh thanks for considering my thoughts :) Removing TextFormatter will definitely break Smoosic temporarily, but I don't think that should be a deciding factor. I can either move it into Smoosic itself, or change it to use the measurement method in my text editing features. The latest changeset (pulled today) looks much better. All the fonts look pretty good except Petaluma. There is still a lot of space to the right of '#' and to the left of '1'. Is this something we can still have font-specific metrics on? The 'A' for instance in Petaluma is not x-symmetrical. Usually you can kern superscripts with 'A' but not much with that font. And this might be something you don't get from just measuring the bounding box. The size of the glyphs looks much better. And there are a few examples that even look a little better in the new way. For instance: your branch, looks more centered to me. I don't think I spent enough time with augmented chords: |
@AaronDavidNewman do you prefer to delay this PR to 5? We will need it there if we push the idea of hanlding text fonts and music fonts on a common way. |
This seems like a big enough change that we should consider delaying it.
That way Aaron can use 4.x without major changes and then decide if he
wants to move to 5.0.
I guess that's what versioning is all about 😁... Indicating to our users
that there are likely breaking changes.
…On Sat, Apr 8, 2023, 2:07 AM Rodrigo Vilar ***@***.***> wrote:
@AaronDavidNewman <https://github.com/AaronDavidNewman> do you prefer to
delay this PR to 5? We will need it there if we push the idea of hanlding
text fonts and music fonts on a common way.
—
Reply to this email directly, view it on GitHub
<#1546 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAB2MCMW3QSODI3XPZ3SG53XAETDVANCNFSM6AAAAAAWJTW7UE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@AaronDavidNewman @ronyeh this is the concept that I have in mind for kerning: "letting the internet browser do it" https://jsfiddle.net/mhv9L3xw/ the accidentals are however shifted when using the font directly. |
Browsers get their kerning rules from hints embedded in the font file. Wouldn't that require all the glyphs, text and music, to be in the same font file? And also that you embedded the formatting rules in those files. |
No, you can provide several comma separated fonts to font-family and the browser will take care. I believe that the browser will also look up the first font that provides the glyph and we will be able to remove the |
fixes #1459
fixes #1241
Trying to reduce the metrics I go again into the topic of removing TextFormatter and using the canvas funtion. There are three mayor benefits: