image-rs / image

Encoding and decoding images in Rust
Apache License 2.0
4.97k stars 618 forks source link

What to do in 0.24 #1433

Closed HeroicKatora closed 2 years ago

HeroicKatora commented 3 years ago

We have collected some amount of cruft that could be fixed in a new major version. There is no single issue that is a pressing concern, some have workaround, but I'd want to arrive at somewhat of a consensus or plan nevertheless as in total it is awkward. In particular the Rust compiler has moved forward quite a bit. I'd make a next branch like last time, which I believe worked out quite well.

The main issues that could be addressed:

Pure renames and restructing

Design issues

Implementation is breaking

Unanswered questions

fintelia commented 3 years ago

I think it is a good time to start thinking about a 0.24 and I agree with your list of items to consider. Under the first point we should probably also include removal of the deprecated submodules under image::math::* (#1434)

My sense is that the biggest items for this release would/should be iterating the APIs for Pixel, GenericImage and DynamicImage. The async and no_std points could also be quite impactful, but I'm not sure how far we'll be able to get on those in the near term.

HeroicKatora commented 3 years ago

The next branch has been created, and CI enabled. The MSRV limit has also been removed.

okaneco commented 3 years ago

I'd like to see more use of Option and Result return types where feasible to avoid panicking and make interfaces more ergonomic for users. There are several public facing APIs with recoverable states such as get_pixel and get_pixel_mut, ImageBuffer functions that handle Option with expect (but from_vec returns an Option), and functions that assert_[eq]!, some of which do return ImageResult and some don't. More imageops functions should probably return Result or Option. I understand some of these choices may have been made to maintain backwards compatibility.

I know efforts were made for GenericImage::opt_pixel but that was reverted in #1245 and I don't see any issues for it other than #1243 discussing the name. I'd like to see access methods that don't panic in the next version of image. Should I make a new issue to discuss that or use an existing issue like #1243 or #1340?

Another thing I've seen in a TODO in buffer.rs was the 'static bound on Pixel and if that is still necessary there and in imageops.

HeroicKatora commented 3 years ago

Feel free to use #1243 for it. And Ive edited your points into the initial comment.

HeroicKatora commented 3 years ago

Regarding the MSRV policy, how will we handle it and which version should we choose. The situation changed a bit compared to our last choice:

okaneco commented 3 years ago

Under Implementation is breaking, as a sub-point to

[ ] optional get_pixel methods (and more clarity on assert vs. Result)

1500 added functions that returned Option on the ImageBuffer struct in a non-breaking manner but it hasn't been in a patch release for 0.23. Breaking changes would occur if this function was added to traits like GenericImage/GenericImageView or the corresponding functions had their signatures changed.

5225225 commented 3 years ago

Should DynamicImage be made NonExhaustive?

Apparently it's not at the moment (Mentioned in #1552), but it looks like the kind of enum we'd want to extend without it being a breaking change.

jplatte commented 2 years ago

Are there plans to do another 0.23.x release? I'm mostly interested because of #1538.

antonok-edm commented 2 years ago

+1 on another 0.23.x release. Looks like there's been a bunch of additions since the last release; in particular I'd love to see https://github.com/image-rs/image/pull/1624 supported in a version on crates.io (https://github.com/jaredforth/webp/issues/7#issuecomment-987693204 for context).