search

kornelski / rust-rgb

struct RGB for sharing pixels between crates
https://lib.rs/rgb
MIT License
97 stars 19 forks source link

Should `Gray` and `GrayAlpha` be regular structs? #83

Closed ripytide closed 2 months ago

ripytide commented 2 months ago

From #82.

Reasoning: Being a tuple struct provides less of a ergonomic benefit to using the structs in calculations that a named struct. For example:

// regular struct
let total_blended = pixel1.gray * pixel1.a + pixel2.gray * pixel2.a
// tuple struct
let total_blended = pixel1.0 * pixel1.1 + pixel2.0 * pixel2.1

This has less of an effect with Gray since it only has one member and so the change would be from pixel.0 to pixel.gray.

For me the readability is much easier with named fields. gray seems the best name since g could be confused with green from Rgb, but a can be kept short to be consistent with the a from other alpha structs like Rgba.

kornelski commented 2 months ago

The structs can't change due to backwards compatibility.

For this crate it's necessary to get all 0.8 users on board, which requires backwards compatibility. Types from 0.8 and 0.9 have to be interoperable, to allow users upgrade without waiting for others. Otherwise upgrading will require semver-major bumps for users, or block some from upgrading when their dependencies haven't upgraded, and can create a delay like Python 2 vs 3.

ripytide commented 2 months ago

That's a shame, I wonder how long an ecosystem transition from 0.8 to 0.9 would take if we did make the upgrade a breaking change.

Most libraries I've seen upgrade their dependencies fairly regularly so it might only take a year for all of the well-maintained libraries to update, and conversions could be implemented as a workaround for interop.

kornelski commented 2 months ago

The problem with incompatible upgrades is that the slowest participants hold back everyone else. You can't upgrade rgb in your crate until all your dependencies upgrade, and they can't until their dependencies upgrade, etc.

Your dependencies will take forever to upgrade, because they need to bump their own semver-major when switching to the incompatible rgb, and that's disruptive for them too, and now all their users will have to upgrade too, causing another wave of churn and delays.

So I think the answer to when everyone upgrades could be "never".

ripytide commented 2 months ago

Playing devil's advocate In this scenario: The drawback with never making breaking changes though is it risks an rgb2 crate being created that offers a (marginally) better value proposition by making those breaking changes the original refused to make. Now you have an ecosystem split anyway just with a different crate name. At least the crates that can use it freely (like new libraries) get a (marginally) better experience. Cue the xkcd comic.

kornelski commented 2 months ago

It's easy to add features, and difficult to achieve interoperability. Interoperability wins over features (just look at the unkillable GIF format).

ripytide commented 2 months ago

I think that would depend on the use-case, if you're exporting types from a crate and you don't want to make any breaking changes I'd want a rock solid dependency rgb1. If I'm just using the types because they're useful and am not re-exporting them (or don't care about breaking changes) and because they have all the helper methods I would have written anyway then I'd want the second crate rgb2.

I would think these are two of the largest but distinct use-cases of this crate.

ripytide commented 2 months ago

Isn't moving the inherent methods to traits backwards incompatible as well? As even with the semver-trick unless the original inherent methods are also kept but deprecated they couldn't be implemented "only" in the 0.8 branch due to the orphan rules. But if we do keep the inherent method (but deprecate them) then there will be naming clashes with the trait functions which would make usability much harder unless we also gave them different names like map(), map_alpha(), though map_c() is different from map_color(). Same with removing the deprecated Bgr::new() in #67.

kornelski commented 2 months ago

Maybe inherent methods could be behind a feature flag? I know that's not 100% backwards compatible, but it only needs to be compatible enough for the current users, and to offer a gradual migration path 0.8 -> 0.9 and then 0.9 -> 1.0 could drop hacks for 0.8.

ripytide commented 2 months ago

I think that could work but might cause very confusing errors due to crate feature unification and naming disambiguation clashes. A hard breaking change 0.8 -> 0.9 release may be simpler overall. But again, this would totally vary on a case-by-case basis between crates depending on their usages and how much they re-export the types and their breaking change release schedules. We might need to do some case-studies on the most popular crates using rgb.