Closed (fixed)
Project:
Image Effects
Version:
8.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
30 Jan 2016 at 22:40 UTC
Updated:
17 Feb 2016 at 18:14 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
hctomAnd here is the patch... cross your fingers that the color values in the test are valid ;)
Comment #4
mondrakeLet's keep alphabetical sort here. Then it should be one line below.
$this->t()... actually this is wrong in the brightness effect too.
'0x' - Maybe this is expecting an hex level?
Comment #5
hctomHere is a new patch with the following additional changes:
* Alphabetically sorted list of effects in
README.md* Changed
t()to$this->t()(also for Brightness effect - I know this is out of scope of this issue, but let's get that fixed fast, hehe)@mondrake: The '0x' you mentioned is required for the correct value of the ImageMagick command, as it represents the brightness change (0 = none). You can find more information at: http://www.imagemagick.org/script/command-line-options.php#brightness-co...
Unfortunately the test will fail again, because of the wrong test color results and I am working on that. But unfortunately I can't get my test to work on my local machine, bacause of strange errors thrown in the
ImageEffectsTestBaseclass. So perhaps there will be another ticket fixing this before I can examine my tests locally ;)Comment #6
hctomWhoops, I hid the wrong file!
Comment #7
mondrake1.
is there a reason for the blank space before $this-t ?
2.
Thank you, clear.
3. Just in case you find issues with color results being different in GD and ImageMagick, also consider reviewing the patch in #2651960: GD watermark operation loses watermark image alpha , that one has a method to check colors 'closeness', not just absolute equality
EDIT - sorry, xpost, it looks like the patch files should be uploaded again :(
Comment #8
hctomFixed typo in issue description!
Comment #9
hctomHere are the updated patches (unfortunately still with failing test, but I will have a look at that now) for reference.
@mondrake: I really don't know why there were these whitespaces ;) I removed them in both contrast and image effect now.
Comment #11
hctomJust came back from the new Star Wars movie and I hope the force is strong with me right now ;) So let's give it another try!!!
Here is a patch with adjusted test color values. And for anybody interested in the local test problems mentioned in #5: It is not possible to test a module installed in an install profile folder ;) So it has to reside in
docroot/modulesotherwise it won't be found, becausesimpletestuses thetestingprofile during test runs.Comment #12
hctomSo... and finally here is another new patch with adjusted test color values for ImageMagick toolkit.
Comment #13
mondrakeLooks good to me. Tested manually and automatic tests for ImageMagick pass locally. The color differences between GD and ImageMagick may be addressed by using the 'colorsAreClose' method being introduced in #2651960: GD watermark operation loses watermark image alpha , but that could be a follow-up.
RTBC
Comment #14
hctomCool, thanx! ;)
I guess the different color values need to be addressed as is, because both toolkits handle contrast changes completely different. ImageMagick even has another contrast method
-sigmoidal-contrast(http://www.imagemagick.org/script/command-line-options.php#sigmoidal-con...), which increases the contrast without saturating highlights or shadows.Comment #15
mondrakeYep, we are warning that different toolkits may give different results on the same effect. Here it would be rather a possibility to 'lax' the color equality check in the tests, and avoid hardcoding results by toolkit. Let's discuss in a follow-up if that makes sense.
Comment #16
slashrsm commentedCommitted. Thanks!