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

Add support for clipping/cutting out (#435) #448

Merged
merged 1 commit into from
Jun 25, 2016

Conversation

kleisauke
Copy link
Contributor

Usage: overlayWith('overlayimage.png', { cutout: true } )

Only the API Docs and changelog needs to be updated. Test cases are included.

Thanks to @jcupitt for the help provided here.

using sharp::MaximumImageAlpha;

bool maskHasAlpha = HasAlpha(mask);
bool dstHasAlpha = HasAlpha(dst);
Copy link
Owner

Choose a reason for hiding this comment

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

I think dstHasAlpha will always be true as there's logic elsewhere to ensure the presence of an alpha channel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lovell
Copy link
Owner

lovell commented May 30, 2016

Fantastic, thank you for this Kleis. It's always a pleasure to see tests and I'm happy to update the docs etc. I've added a few comments inline.

USAGE: overlayWith('overlayimage.png', { cutout: true } )
@coveralls
Copy link

coveralls commented May 31, 2016

Coverage Status

Coverage decreased (-0.2%) to 98.462% when pulling 6130f24 on kleisauke:feature-cutout into 4f3262c on lovell:master.

@coveralls
Copy link

coveralls commented May 31, 2016

Coverage Status

Coverage decreased (-0.2%) to 98.462% when pulling 6130f24 on kleisauke:feature-cutout into 4f3262c on lovell:master.

@lovell
Copy link
Owner

lovell commented May 31, 2016

Updates look great, thank you.

"What will be the best option? Will there be any difference in the image if we don't pre-multiply it?"

I think the best approach is to avoid premultiplying the image to be overlaid, assuming there are no other reasons for shouldPremultiplyAlpha being set to true, when using cutout.

This will avoid (a) a performance hit, (b) a source of rounding/clipping errors, and (c) potential light/dark artefacts at the "edges" of what was the discarded alpha channel.

@jcupitt
Copy link
Contributor

jcupitt commented May 31, 2016

A general Porter-Duff compose operator could solve this problem too. vips will need PD at some point.

@lovell
Copy link
Owner

lovell commented Jun 24, 2016

@kleisauke Would you like to remove the premultiply logic here or would you prefer I take a look?

@kleisauke
Copy link
Contributor Author

@lovell I already did that. See: pipeline.cc#L463-L464 and pipeline.cc#L698-L700.

@lovell
Copy link
Owner

lovell commented Jun 25, 2016

@kleisauke You are absolutely correct - sorry I completely missed that. Thank you again for adding this very useful feature!

@lovell lovell merged commit 2e9cd83 into lovell:master Jun 25, 2016
@kleisauke kleisauke deleted the feature-cutout branch June 26, 2016 13:12
@kleisauke
Copy link
Contributor Author

@lovell No problem! Thanks for merging the pull request! 👍

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