-
Notifications
You must be signed in to change notification settings - Fork 307
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
Horizontal/Vertical shift threshold #106
base: main
Are you sure you want to change the base?
Conversation
…3 pixel shifts in text
… original pixel does not match
Looks like the CI only failed due to some basic linting mistakes. |
…nt rendering differences
@spartanatreyu , Thanks for pointing out my neglecting the CI tests. I fixed the linting issues, and then added a test case showing how the pixel shifting due to Chrome font rendering can be mitigated with my two new parameters:
|
@mourner do you think this PR is worth merging, now that it has tests included? |
This PR would solve many of my issues, totally worth for me 👍 |
@tybrd916 this looks like a useful addition, I'm sorry that I haven't had the time to review closely yet. On a cursory glance, I'm only worried about possible performance issues (the check seems very expensive, even though if it's disabled by default), and also potential conflicts with another incoming PRs like #113. I'll take a closer look when I have a chance. |
Agreed the horizontal/vertical shift pixel checking does increase computational effort. One optimization I did was to only do the extra effort of looking at neighboring/shifted pixels when the original pixel fails to match given the other the threshold and anti aliasing parameters. So for regression testing, almost identical images do not have significantly higher computation effort. But comparing too very different images is significantly more computationally expensive |
Hey fellow Ukrainian @mourner 👋 🇺🇦 We are using pixelmatch in Lost Pixel - it's an open source vis reg tool that we built with a friend and the only thing which drives me crazy is the font flakiness even when ran inside docker - I'd love this PR to land into pixelmatch so we can try and reduce that flakiness. @tybrd916 if you'd need any help with this - ping me, I am excited to see how the thresholds you introduce could mitigate anti-aliasing problems in the machine browsers! thanks for your work! |
@mourner , is there anything else I can do to help prove out this PR? It's amazing how much adoption your pixelmatch solution has, millions of downloads per week! |
I'd really like to see this merged. |
Proposed addition of two parameters to popular
pixelmatch
library:Setting these parameters > 0 makes
pixelmatch
do additional checking for neighboring pixels within plus-or-minus horizontal/vertical "shift pixels" to avoid false-positives when the browser misaligns text by a few pixes.Useful for
jest-puppeteer
testing screenshot matching where Chrome/Firefox text rendering is slightly shifted even on the same machine like this difference with originalpixelmatch
: