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

[p5.js 2.0 Beta Bug Report]: Update image diff algorithm in snapshot tests to be less tolerant #7496

Open
1 of 17 tasks
davepagurek opened this issue Jan 24, 2025 · 2 comments

Comments

@davepagurek
Copy link
Contributor

Most appropriate sub-area of p5.js?

  • Accessibility
  • Color
  • Core/Environment/Rendering
  • Data
  • DOM
  • Events
  • Image
  • IO
  • Math
  • Typography
  • Utilities
  • WebGL
  • Build process
  • Unit testing
  • Internationalization
  • Friendly errors
  • Other (specify if possible)

p5.js version

2.0 beta 1

Web browser and version

N/A

Operating system

N/A

Steps to reproduce this

I was just working on #7495, and found that the change I made to src/shape/custom_shapes.js line 398 did not cause existing visual tests to change:

- points.unshift(this.vertices.at(-1), prevVertex);
+ points.unshift(this.vertices.at(-1));

Snapshot tests should have low enough tolerance that this change would cause a test to fail. However, simply reducing the shiftThreshold or even switching to the mapbox pixelmatch package caused many false negative failures on CI. This is because rendering on different platforms is different enough that it's sometimes more than just what pixelmatch detects as an antialiasing difference (for example, sometimes rendered text is shifted a pixel down.) We want these to be acceptable differences.

I'm not sure what options we have here, but if anyone has other libraries they want to test, or updates to our custom diff code, I'd love to hear your ideas!

@Vaivaswat2244
Copy link

Vaivaswat2244 commented Jan 28, 2025

hello @davepagurek , we can use resemble.js. Although snapbox pixelmatch was built being inspired by resemble.js so I'm not 100 percent sure that this will work or not but it can be worth a shot. From what I saw, resemble sure gives us more flexebility.

// ResembleJS approach - more flexible
resemble(image1)
  .compareTo(image2)
  .ignoreAntialiasing()
  .ignoreColors()
  .setTolerance(2.5)
  .scaleToSameSize()
  .onComplete(result => {
    // Detailed analysis available
    console.log(result.misMatchPercentage);
    console.log(result.diffBounds);
    console.log(result.analysisTime);
  });

// Pixelmatch approach - more rigid
const diff = pixelmatch(img1, img2, null, width, height, {
  threshold: 0.1,
  includeAA: false
});

@davepagurek
Copy link
Contributor Author

Thanks @Vaivaswat2244! I actually started trying to use resemblejs in #7251 and then ended up taking it out again in #7266 because I was finding that text rendering was causing text to be shifted by around a pixel or so on CI compared with images generated on my mac. I think this is probably the main hurdle to overcome: a lot of these packages are set up to try to handle differences just on the border of shapes, and not the shape itself moving a tiny bit and being accommodating of that.

I think something like resemble (or pixelmatch) could still work, but we'll have to maybe do something nonstandard to handle the text case. It might only show issues on CI if you happen to be using the same system the images were first generated on (a mac, in my case) so feel free to make a PR with your changes if you want to see what breaks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants