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

applyMask() fix in Gd/Image #708

Merged
merged 3 commits into from
Jun 13, 2021
Merged

applyMask() fix in Gd/Image #708

merged 3 commits into from
Jun 13, 2021

Conversation

ninze
Copy link
Contributor

@ninze ninze commented Apr 16, 2019

Created applyMask() fix for #460

@ausi
Copy link
Collaborator

ausi commented Jul 6, 2019

I’m not sure if this fix is correct. I think applyMask() is meant to be used with a grayscale image.

Did you try to use ->applyMask($mask->mask()); as described in #460 (comment) ?

@ninze
Copy link
Contributor Author

ninze commented Jul 10, 2019

@ausi yes I did without success. The fix makes imagine work consistently with both GD and Imagick drivers. We are using this fix in production since April without any issues.

@mlocati
Copy link
Collaborator

mlocati commented Nov 3, 2020

Is it possible to have a test case that covers what this PR is fixing?

@ninze
Copy link
Contributor Author

ninze commented Nov 6, 2020

It probably is, will look into it.

@ninze
Copy link
Contributor Author

ninze commented Dec 26, 2020

Done. I don't have gmagick installed, but GD and Imagick produce identical results after the fix.

@jakubjo
Copy link

jakubjo commented May 15, 2021

@ausi yes I did without success. The fix makes imagine work consistently with both GD and Imagick drivers. We are using this fix in production since April without any issues.

Maybe it's about time someone merges this?

@ninze
Copy link
Contributor Author

ninze commented Jun 10, 2021

After more than 2 years since the pull request was created, I also think someone should either merge or close this.

Copy link
Collaborator

@ausi ausi left a comment

Choose a reason for hiding this comment

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

Tested this pull request locally and all worked correctly.

In the old code ->dissolve(…) could have never ended up with a negative value and therefore could never have masked anything.

@mlocati
Copy link
Collaborator

mlocati commented Jun 13, 2021

Thanks for reviewing this PR, @ausi !

@mlocati mlocati merged commit 5da0c38 into php-imagine:develop Jun 13, 2021
@chekalsky
Copy link

chekalsky commented Jun 13, 2021

@mlocati thank you! is it possible to release a new version of Imagine wit this fix any time soon?

@mlocati
Copy link
Collaborator

mlocati commented Jun 14, 2021

@chekalsky I'd publish a new version after tests have been fixed (I'd move them to GitHub Actions)

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

Successfully merging this pull request may close these issues.

5 participants