Add missing map2/apply2 functions to `Pixel` trait

image-rs / image

Encoding and decoding images in Rust

Apache License 2.0

4.93k stars 610 forks source link

Add missing map2/apply2 functions to `Pixel` trait #2239

Open jnickg opened 5 months ago

jnickg commented 5 months ago

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

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)

jnickg commented 4 months ago

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 *2_without_alpha functions though (in fact I only added them for parity, and also thought they could just use the closure you mentioned).

Just to clarify, it sounds like you're looking for a code change for the *2_without_alpha functions so that they are defaults on the trait. Is that accurate? Would you like the *2_with_alpha functions implemented that way as well?

Thanks!

HeroicKatora commented 4 months ago

Just to clarify, it sounds like you're looking for a code change for the 2_without_alpha functions so that they are defaults on the trait. Is that accurate? Would you like the 2_with_alpha functions implemented that way as well?

Yes, I meant these functions aren't present on the impl block in color.rs. It looks like they are defaulted in precisely the manner which I had thought the {map,apply}2_without_alpha should be. This is definitely good enough. There's no need to default *2_with_alpha.

jnickg commented 3 months ago

Just to close the loop, I rebased onto the latest main and applied the cargo fmt changes to make CI happy. Hopefully that is enough to get it merge-able

jnickg commented 3 months ago

@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.

HeroicKatora commented 3 months ago

This is still a breaking change so in light of the issue with packaging, a release of 0.25.2 before this is merged to the main branch for 0.26 then. But should be good to go, nothing more to do from your side I think.

jnickg commented 3 months ago

Thank you!!!

ripytide commented 1 month ago

I think we should wait to merge this until the the conflict with #2259 has been discussed.