-
Notifications
You must be signed in to change notification settings - Fork 669
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
svg:MeasureTextCache: flow.html: The cached glyph width depends on the order of the tests. #1241
Comments
yikes, thanks for catching this @h-sug1no! |
I'll take a look this week. I'm not familiar with the measure text cache but can investigate it. How did you notice this bug, by the way? Have you also been running the command line tests in parallel? We should get more of us testing the parallel visual diffs, and then promote it to be the default! |
Ah...though my puppeteer visual-regression-test branch is still on going, but it can generate svg(and png (generated by UA from svg data)) files. I found this problem when I was testing my branch. you can test my branch as follows: clone and checkout: git checkout vrtest-puppeteer-backend-1
npm install
npm audit fix
npm install --save-dev puppeteer
npx grunt && npm run reference
VF_GENERATE_OPTIONS="--backend=pptr" npm run generate:current
VF_GENERATE_OPTIONS="--backend=pptr --parallel=1" npm run generate:reference
npm run diff:reference then you can get the following errors:
I have no time to fix my branch until the end of next month.... But if you need to fix, Please feel free to fork my branch. |
I investigated a bit. It might be because some of the test cases call Currently, when we scale a SVG context by 1.5X, we modify the viewBox attribute. I think the reasoning is that it should operate similarly to CanvasContext. If the SVG is scaled up, its containing box will be the same size, but we "zoom into" the image. A different approach would change the SVG element's transform: But if we wanted to preserve the cropping behavior of the CanvasContext, we would need to wrap our SVG element in a parent div tag with Here's an example where you can play around with the SVG's viewBox, transform, and parent DIV: https://jsfiddle.net/om72h39k/ As you modify the viewBox or scale transform, you'll see that the reported width of the ✕ symbol changes slightly. I think it's just regular floating point error.
See the test cases in: Even when I round the widths to 1 decimal place, I get discrepancies like this:
This is because some test cases set the scale to 1.5x, and then others set the scale to 1.0x. When we ask the browser to measure the text, the floating point error shows up. This means that if we change the ordering of test cases, or if we run tests in parallel, the exact widths won't match up. What should we do?
Can you verify that the failing test cases are when:
|
Sorry I didn't see this issue earlier, I've not had much time for personal projects recently. I added the glyph measurement cache because it speeded up the unit tests by about 20%. Without the cache, every single call to measureText on a CanvasContext will cause a document reflow. I'm all for deleting the cache if its buggy. I actually have a branch that I'm working on that factors texture measurement out into a separate class. That way, we can use a hidden (or offscreen) canvas to measure text even when the actual rendering is done using SVG. I can send a PR deleting the cache later in the week. |
I don't think we need to delete the cache just yet. The measurement numbers are basically the same. :-) It only affects the visual regression tests if we run them out of order (or in parallel). On a given score in the real world, no one will notice if something is off by 0.1 pixels. Plus, this only happens if someone has two scores at different context scales rendering the same text. I like your idea of using a offscreen canvas to measure text. That way, the helper canvas can always be the same scale, even if we scale the output canvas or SVG. |
Just a quick idea: Can't we save the pixel value for 1.0 scale or something and restore it later? |
{TabSlide › Simple TabSlide › SVG + Bravura} (Assume that all tests that use the MeasureTextCache are affected)
with this dirty patch:
multiple problems(entry.width and entry1.width should always be the same, right?) are detected, as follows:
The text was updated successfully, but these errors were encountered: