image-rs / imageproc

Image processing operations
MIT License
736 stars 144 forks source link

Remove non-`_mut` function variants. #622

Closed ripytide closed 3 months ago

ripytide commented 3 months ago

A lot of functions at the moment have two variants: a _mut version and a non-_mut version. For example, morphology::dilate() and morphology::dilate_mut(). The non-_mut version is simply a 3-line wrapper of the mut version.

This makes maintaining the library a lot more difficult as it doubles the API surface for those functions. Documenting both functions becomes more difficult as you either have to duplicate the docs but just change from &mut image to passing in a whole image and storing the result, or, link one of the docs to the others using a doc-link and hope the user can figure out the differences in ownership semantics.

This seems like the kind of job a user should be taking care of and not this library. If the user is doing &mut image.clone() a whole lot then they can easily abstract that themselves but I don't feel we should be taking on that burden for them.

My suggestion would be to either deprecate all the non-mut function variants and then delete them in the second-next release or simply delete them outright.

ripytide commented 3 months ago

This came up when working on #602 where the non-mut functions double the amount of work I would have to do.

theotherphil commented 3 months ago

I expect the non-mut versions will be far more widely used, with the _mut versions existing solely to allow users to control allocations if this matter for their use cases. I'm not a fan of removing the more ergonomic and more used (I assume) versions of functions.

We should standardise on having detailed documentation on the non-mut versions and linking to this from the _mut versions.

In theory the generation of the _mut versions could be automated via a proc macro but this feels unlikely to be worth the extra complexity or compile time.

ripytide commented 3 months ago

Judging by some grep.app searches it varies quite a bit.

dilate shows only the mut version is used. map_pixels shows both the mut version and the non-mut version.

ripytide commented 3 months ago

In theory the generation of the _mut versions could be automated via a proc macro but this feels unlikely to be worth the extra complexity or compile time.

I think a proc macro would solve my issues with maintainability and help with standardization, I don't think the added compile time would be noticeable for such a simple proc-macro, the complexity would be mostly a one-time cost on the implementation of the proc-macro and should be very easy to use after its created perhaps?

ripytide commented 3 months ago

Nevermind, I tried implementing the proc-macro and I needed to use a lot of heuristics when generating the non-mut function signature such as choosing a return type based on the input image type which could get very complicated very quickly when generics are involved.

What about switching the documentation to the mut version rather than the non-mut function seeing as that is the "real" function whereas the non-mut variation is simply a useful helper function? Then we could standardize the documentation of the non-mut functions as:

/// An immutable version of [`erode_mut`].

This function clones the given image, then runs the operation on the cloned image and then returns the mutated image.

This documentation could be generated from a regular macro_rules!(erode_mut) as well which reduces the amount of duplication.

theotherphil commented 3 months ago

It’s more convenient to write doctests for the non-mutating variants, so I’d flip your suggestion. But the idea of generating the _mut docs via a macro is a good one.

ripytide commented 3 months ago

But is it a good idea to be teaching new users to use the less-efficient functions? That may cause users to use the non-mut variations even if they don't need the original image anymore since those are the functions with examples. That doesn't really fit with rusts fast-by-default design imo.

theotherphil commented 3 months ago

The two versions of each function will be next to each other in the documentation so anyone who wants to re-use an allocation can quickly spot that this is possible.

ripytide commented 3 months ago

Fair point, I'll start a PR standardizing all the mut/non-mut functions using a macro then :+1: After that, I'll be able to rebase and continue #602 which might become easier.

theotherphil commented 3 months ago

Sounds good, thanks.