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

Fix comparison when layout differs #111

Merged

Conversation

serpent7776
Copy link
Contributor

In case layoutDifference is true compColor is assigned incorrect pixel value read from base instead of comp.

In case `layoutDifference` is `true` `compColor` is assigned incorrect
pixel value read from `base` instead of `comp`.
@dmtrKovalenko
Copy link
Owner

It would be amazing if you can add a test case that would fail with the previous implementation

@serpent7776
Copy link
Contributor Author

I have no idea how to add new image files, because they are not being copied to the _build dir by dune.

@dmtrKovalenko
Copy link
Owner

intersting we have one of the existing tests failing could you please elaborate more on why do you think the existing test is wrong

@serpent7776
Copy link
Contributor Author

I can understand that current value of 399 comes from difference in the image sizes, but now there's 18 more pixels. I don't know how antialiased comparison works, so I don't know why.

@serpent7776
Copy link
Contributor Author

The following pixels are considered different (where previously weren't) after this fix:

x,y
90,0
93,0
94,0
95,0
96,0
103,0
104,0
105,0
106,0
109,0
0,90
0,93
0,95
0,96
0,103
0,104
0,106
0,109

@serpent7776
Copy link
Contributor Author

I added a new test. Locally I had to copy the new images manually to _build directory for whatever reason.

@serpent7776
Copy link
Contributor Author

I updated expects for the failing test 🤷

@dmtrKovalenko dmtrKovalenko merged commit 99e5e2a into dmtrKovalenko:main Oct 2, 2024
5 checks passed
@serpent7776 serpent7776 deleted the fix-different-layout-compare branch October 2, 2024 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants