jay3332 / ril

Rust Imaging Library: A high-level imaging crate for Rust.
https://crates.io/crates/ril
MIT License
81 stars 10 forks source link

consider undeprecating Image::inverted #24

Open backwardspy opened 1 year ago

backwardspy commented 1 year ago

hi! i really like this crate for how easy it makes image manipulation, and as such i would like to make a suggestion about its API.

implementing Not for inverting an image is a neat trick, and does provide a convenient shortcut, however in longer method chains i believe it hurts readability.

here is an example of a method chain that i think is harmed by the new syntax:

// old
let result = image
    .cropped(300, 300, 800, 800)
    .inverted()
    .brightened(50)
    .hue_rotated(180);

// new
let result = (!image.cropped(300, 300, 800, 800))
    .brightened(50)
    .hue_rotated(180);

in my opinion it's harder to tell at a glance what the second one does, and it also took me a couple of attempts to get the parentheses in the correct place to produce the same output. for example, this slight change (swapped ! and ( by accident) changes where the inversion happens and produces a different output than i wanted:

let result = !(image.cropped(300, 300, 800, 800))
    .brightened(50)
    .hue_rotated(180);

i don't necessarily believe you should remove the option of using !, just that it would be nice if .inverted() was not deprecated at the same time.

please let me know your thoughts, and thanks for the awesome crate!

jay3332 commented 1 year ago

I only deprecated it because it was pretty odd having two different ways of doing the same thing, but I'll probably undeprecate it in 0.11 for the sake of method chains

backwardspy commented 1 year ago

yeah i generally agree, but i do think in this case there's a compelling reason to keep both. thanks for your consideration! :)