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 missing map2/apply2 functions to Pixel trait #2239

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jnickg
Copy link

@jnickg jnickg commented May 20, 2024

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)

Copy link
Member

@HeroicKatora HeroicKatora left a 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.

@jnickg
Copy link
Author

jnickg commented Jun 10, 2024

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
Copy link
Member

HeroicKatora commented Jun 10, 2024

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 pushed a commit to jnickg/image that referenced this pull request Jun 11, 2024
- This makes their implementations parallel with {map,
  apply}_without_alpha implementations
- Per PR feedback in image-rs#2239
@HeroicKatora HeroicKatora mentioned this pull request Jun 12, 2024
7 tasks
Copy link
Member

@HeroicKatora HeroicKatora left a 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).

jnickg added 2 commits July 5, 2024 11:45
* 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
@jnickg jnickg force-pushed the feature/apply2_alpha branch from 42b9fe0 to 220f39f Compare July 5, 2024 18:46
@jnickg
Copy link
Author

jnickg commented Jul 5, 2024

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 jnickg requested a review from HeroicKatora July 6, 2024 05:11
@jnickg
Copy link
Author

jnickg commented Jul 6, 2024

@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
Copy link
Member

HeroicKatora commented Jul 10, 2024

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
Copy link
Author

jnickg commented Jul 11, 2024

Thank you!!!

@ripytide
Copy link
Member

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

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

Successfully merging this pull request may close these issues.

3 participants