-
Notifications
You must be signed in to change notification settings - Fork 622
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 missing map2/apply2 functions to Pixel
trait
#2239
base: main
Are you sure you want to change the base?
Conversation
1fe6db1
to
b7b4d9f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The {apply,map}2_with_alpha
are well-defined extensions and were sketched out already. Overall whether or not they can be accomplished with the existing functions or not wouldn't matter too much here, it's a pretty small change whose minor breakage can be easily part of the next major version. (In theory there could be an approximate default impl based on as_channels
and color model but I don't think the risk of modelling error is worth it to mitigate that inconvenience).
I think the opposite applies to *_without_alpha
. Those can be written by using a specific parameter choice for g
from the with_alpha
version: |lhs, _| lhs
. We don't have the equivalent method for the single pixel function either. Here it might be best to provide them as defaults on the trait instead.
Hi @HeroicKatora , thanks for the review. I might be misunderstanding something though, can you please clarify? I see these functions which look like the equivalent single-pixel functions, which is why I added them. I'm happy to elide Just to clarify, it sounds like you're looking for a code change for the Thanks! |
Yes, I meant these functions aren't present on the |
- This makes their implementations parallel with {map, apply}_without_alpha implementations - Per PR feedback in image-rs#2239
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, it fits the design. Should be considered for the next breaking version change (0.26).
* Account for alpha channel to simplify some image math operations where naively operating on all pixel components won't work (such as summing images, where usually picking one alpha value from `self` or `other` makes more sense)
- This makes their implementations parallel with {map, apply}_without_alpha implementations - Per PR feedback in image-rs#2239
42b9fe0
to
220f39f
Compare
Just to close the loop, I rebased onto the latest |
@HeroicKatora looks like everything is green, but I don't have a "merge" button and can't push a merge commit. Please let me know if there's any further action I should take. |
This is still a breaking change so in light of the issue with packaging, a release of |
Thank you!!! |
I think we should wait to merge this until the the conflict with #2259 has been discussed. |
New contributor license agreement: I license past and future contributions under the dual MIT/Apache-2.0 license,
allowing licensees to choose either at their option.
This PR merges quick changes to add a few missing functions to
Pixel
so that basic image math operations are easier when dealing with alpha-channel images.Commit Messages
self
orother
makes more sense)