change invert() and add invert_in_place()
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()
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.
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
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
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.
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.
In the meantime, we do have some evidence that in-place transformations (or rather in-allocation) are generally desirable: https://github.com/image-rs/image/discussions/2650. I think more people stumbled upon it but simply did not ask and accepted the allocation. That said there is a bit of design space on how to achieve them since it's correctly observed that we do need two separate APIs for aliasing reasons when we take &mut _ at least. It's also not entirely clear if in-place refers to an output argument or modifying the input itself—which makes for actually three incompatible call interfaces.
If we spin this thread of thought further, we could possibly have Rc/Arc like images (at least, I found that relevant in my own designs) and that would be even more different interfaces. But that is a bit besides the point here, I would explicitly avoid basing an argument around that.
So:
DynamicImage::invert(&self) -> DynamicImageDynamicImage::invert_in_place(&mut self)DynamicImage::invert_into(&self, _: &mut DynamicImage): opportunistically reuse the buffer or error, that is discussion for another day
Something as simple as invert seems like a comfortable way to at start with that variety without too much of a maintenance problem.