Skip to content

Fuzzy pixel color comparison#507

Merged
tkaik merged 3 commits intomasterfrom
feature/fuzz
May 27, 2019
Merged

Fuzzy pixel color comparison#507
tkaik merged 3 commits intomasterfrom
feature/fuzz

Conversation

@kaczymuczy
Copy link
Copy Markdown

@kaczymuczy kaczymuczy commented Apr 29, 2019

Description

Introduce an option to match colors that are close to the target color in a grayscale space when comparing pixels with the LayoutComparator.
The comparison can now run in two modes:

  • the standard mode (nothing changed),
  • "fuzz mode" - the baseline pattern and new screenshot images are converted to grayscale and the comparison takes into account a "fuzz" - a value that describes how different (how big is the distance) the pattern and screenshot pixels can be in order to consider them equal.
    In other words - a naive implementation of https://imagemagick.org/script/command-line-options.php#fuzz

Motivation and Context

This option allows to filter out image rendering issues like AEM image rendition differences not visible to the human eye.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • I have reviewed (and updated if needed) the documentation regarding this change

I hereby agree to the terms of the AET Contributor License Agreement.

Add unit tests for fuzz option
Update existing code to fit the new fuzz option
@kaczymuczy
Copy link
Copy Markdown
Author

Testing on a working AET instance not done yet.

@kaczymuczy
Copy link
Copy Markdown
Author

Documentation not done yet.

@kaczymuczy
Copy link
Copy Markdown
Author

Testing on a working AET instance - done, works fine.

@kaczymuczy
Copy link
Copy Markdown
Author

Documentation done

@wiiitek
Copy link
Copy Markdown
Contributor

wiiitek commented Apr 30, 2019

We could add some slightly different images (lie those generated by AEM renditions engines) to our integration tests with example of how to use fuzz parameter in test suite.
What do you think about such idea?

@kaczymuczy
Copy link
Copy Markdown
Author

#507 (comment)
The unit tests pretty much cover everything there is to test for this feature and the documentation covers the usage example. I don't think extra integration tests would bring more value - you believe this is necessary?

@wiiitek
Copy link
Copy Markdown
Contributor

wiiitek commented May 7, 2019

#507 (comment)
Thank you for your response!

I don't think they are really necessary. And also I don't know what is the role of integration tests currently.

I have looked into sample files for integration tests - they are taken from the real-life use cases, right?

@plutasnyy
Copy link
Copy Markdown
Contributor

I like this feature very much! Problems with images were really really annoying. I think a bit about users - if a test is marked as a fail user doesn't know if it was because of threshold or fuzzy. I also think that using it together is not the best way so I wouldn't change anything.
The second thing that I wouldn't know is how big is the difference between some values of fuzzy, maybe some comparison of noised images that pass a test with the fuzzy value equal to 5% 10% 15% etc could be helpful?
Please update changelog :D
Good job!

@kaczymuczy
Copy link
Copy Markdown
Author

#507 (comment)
Yes, the samples are built from a number of real life cases.

Changelog updated

So, what exactly am I required to do to have this merged? I would love to use this as soon as possible. :D

Copy link
Copy Markdown
Contributor

@plutasnyy plutasnyy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my side you have approval. Good luck ;)

@tkaik tkaik merged commit 4c9fd08 into master May 27, 2019
@tkaik tkaik deleted the feature/fuzz branch May 27, 2019 06:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants