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

Remove textformatter, associated metrics and cache. #1546

Closed
wants to merge 4 commits into from
Closed

Remove textformatter, associated metrics and cache. #1546

wants to merge 4 commits into from

Conversation

rvilarl
Copy link
Collaborator

@rvilarl rvilarl commented Mar 27, 2023

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:

@rvilarl
Copy link
Collaborator Author

rvilarl commented Mar 27, 2023

Accuracy example:

current (boxes around A,B,... are more accurate)
pptr-Stave Volta___Modifier_Measure_Test Bravura svg_current
reference
pptr-Stave Volta___Modifier_Measure_Test Bravura svg_reference

@AaronDavidNewman
Copy link
Collaborator

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.

With text formatter:
image

on your branch:
image

with text formatter (ref):
image

on your branch:
image

@rvilarl
Copy link
Collaborator Author

rvilarl commented Mar 28, 2023

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.

master
image
current (branch)
image

@AaronDavidNewman what do you get running flow.html?

@AaronDavidNewman
Copy link
Collaborator

You guys have fast computers.

Your branch.
image

vexflow branch.
image

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:

your branch:
image

reference:
image

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.

@rvilarl
Copy link
Collaborator Author

rvilarl commented Mar 28, 2023

@AaronDavidNewman it seems that we are using a different master mine has 9942 tests.
I have been looking into this topic and I will commit a change later today.

@rvilarl
Copy link
Collaborator Author

rvilarl commented Mar 28, 2023

branch with latest commit

pptr-ChordSymbol Chord_Symbol_With_Modifiers Bravura svg_current

@AaronDavidNewman
Copy link
Collaborator

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.

@rvilarl rvilarl mentioned this pull request Mar 29, 2023
@rvilarl
Copy link
Collaborator Author

rvilarl commented Mar 29, 2023

@AaronDavidNewman I have removed the scales

@rvilarl
Copy link
Collaborator Author

rvilarl commented Mar 29, 2023

@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?

pptr-ChordSymbol Chord_Symbol_With_Modifiers Bravura svg_current

@rvilarl rvilarl mentioned this pull request Mar 29, 2023
@AaronDavidNewman
Copy link
Collaborator

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...

ref:
image

your branch:
image

ref:
image

your branch:
image

gap on the Y side is still bigger than it needs to be:
ref:
image

your branch.
image

@rvilarl
Copy link
Collaborator Author

rvilarl commented Mar 30, 2023

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"

@rvilarl
Copy link
Collaborator Author

rvilarl commented Mar 30, 2023

@AaronDavidNewman I think that now the chords are fine.

pptr-ChordSymbol Chord_Symbol_With_Modifiers Bravura svg_current

@AaronDavidNewman
Copy link
Collaborator

The formatting looks much better. My only complaint now is that the glyphs (# in particular) are too big relative to the text, and the baseline seems lower relative to the text. There is less standardization on this, however. In the case of the large '/' I think the bigger version looks better.

Current 0xfe master:
image

Your branch:
image

This may have been going on with Petaluma for a while, I can't tell if this has to do with your change.
Latest master:
image

My Smoosic branch based on an earlier 4.x release:
image

Your branch:
image

@rvilarl
Copy link
Collaborator Author

rvilarl commented Mar 31, 2023

@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

pptr-ChordSymbol Chord_Symbol_With_Modifiers Bravura svg_current

@AaronDavidNewman
Copy link
Collaborator

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.

@rvilarl
Copy link
Collaborator Author

rvilarl commented Apr 1, 2023

@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.

@rvilarl
Copy link
Collaborator Author

rvilarl commented Apr 1, 2023

@AaronDavidNewman now pushed with a general scale of 0.8

pptr-ChordSymbol Chord_Symbol_With_Modifiers Bravura svg

@rvilarl rvilarl added the 4.2 label Apr 3, 2023
@ronyeh
Copy link
Collaborator

ronyeh commented Apr 5, 2023

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.

@AaronDavidNewman
Copy link
Collaborator

@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.

image

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:

ref:
image

your branch, looks more centered to me. I don't think I spent enough time with augmented chords:

image

@rvilarl
Copy link
Collaborator Author

rvilarl commented Apr 8, 2023

@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.

@ronyeh
Copy link
Collaborator

ronyeh commented Apr 8, 2023 via email

@rvilarl rvilarl added 5.0 and removed 4.2 labels Apr 8, 2023
@rvilarl
Copy link
Collaborator Author

rvilarl commented Apr 9, 2023

@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.

@AaronDavidNewman
Copy link
Collaborator

"letting the internet browser do it"

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.

@rvilarl
Copy link
Collaborator Author

rvilarl commented Apr 22, 2023

"letting the internet browser do it"

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 lookupGlyph method.

@rvilarl rvilarl closed this May 7, 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