Skip to content

change invert() and add invert_in_place() #2438

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

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

Conversation

Its-Just-Nans
Copy link
Contributor

@Its-Just-Nans Its-Just-Nans commented Mar 17, 2025

This PR copy the design of flip_horizontal() for invert()

Invert is the only inplace function

With this PR there are now invert() and invert_in_place()

@fintelia
Copy link
Contributor

fintelia commented Mar 18, 2025

This is a breaking change, so at a minimum it would have to wait for our next major release.

However, I'm also not sure whether this change is worthwhile. Switching to the *_in_place version would be consistent with the other methods in the colorops module. But I'm concerned that anyone currently using the invert method would have their code continue to compile, but now give different results. Adding #[must_use] would at least generate a warning, but fixing it would still be manual effort

I'd probably hold off on the _in version given that none of the other colorops methods have it.

@Its-Just-Nans
Copy link
Contributor Author

I changed the design for no breaking change

Still I think that not having the same API for method is not great.

Maybe deprecated invert() could do the job

@Its-Just-Nans
Copy link
Contributor Author

Its-Just-Nans commented Mar 18, 2025

I also think that a breaking change is a good option with the version bumped

Note great for current users of image crates but needed to have the same API style

@awxkee
Copy link
Contributor

awxkee commented Mar 18, 2025

I believe before adding any new processing methods iterators at the very least need to get sorted out.
As for now we're technically encouraging users to use a highly inefficient implementation, even though we already know it is not efficient.
I think it is reasonable to discourage as much as possible using plainly inefficient implementations.

@fintelia
Copy link
Contributor

We've been intermittently discussing a possible overhaul of the GenericImage trait for at least five years, so that alone doesn't need to block other progress.

What is this PR trying to achieve? My interpretation is that it is about adding consistency to the colorops module. Currently, the brighten, contrast, and huerotate methods all have both cloning and _in_place versions. While the dither and invert methods operate in-place but don't have the name suffix, and none of the methods in that module have an _to version.

But stepping back, is having two versions of every method really the approach we'd pick starting from scratch? We'd probably just have the in-place versions, right? Which we could do in the next major release by dropping all the _in_place suffixes and making those the default. That is much less likely to be silent breakage: the caller would have to both be passing a mutable image and never actually use the return value anywhere.

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