kornelski / rust-rgb

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

Non-Breaking Way to replace `Gray` and `GrayAlpha` #85

Closed ripytide closed 1 month ago

ripytide commented 2 months ago

How about replacing Gray and GrayAlpha as suggested in #83 but in a non-breaking compatible way by doing what rustlang is doing with the legacy Range types by making new types and keeping the old types around but moving them to a legacy module and deprecating them?

ripytide commented 2 months ago

Additionally, the deprecation notice could be conditionally enabled via a feature which is enabled by v0.8 but off by default in v0.9 so the deprecation is only active when all dependencies in the chain have moved to v0.9 due to feature unification.

ripytide commented 2 months ago

Another alternative method could be to simply make the legacy types be conditionally compiled and enable them from v0.8 but off by default in v0.9 similar to the way you suggested removing the new() methods from BGR.

kornelski commented 2 months ago

Are you really so bothered by the field name to cause all this trouble?

Having two Gray structs could be confusing and give poor diagnostics.

However, making Gray doc-hidden and adding a new Luma type could work. Type name change will make it clear it's incompatible.

ripytide commented 2 months ago

I'm bothered by the inconsistency between the pixel types, Rgba, Rgb, Bgr and all the other pixel types use structs with named fields, and yet Gray and especially GrayAlpha do not which is inconsistent. GrayAlpha is particularly egregious to me since it would actually significantly benefit the readability with using named fields: pixel.a = 0 vs pixel.1 = 0.

I'm hesitant to suggest renaming Gray to Luma since Gray is more understandable in everyday usage to me (goes well with functions like grayscale()). Rust has special error messages when two types have the same name but are different which I think should be enough here, especially if the error messages quote the full paths: "rgb::GrayAlpha and rgb::legacy::GrayAlpha may have the same name but are actually distinct types.". We could also write about this in a migration guide which would help anyone doing the upgrade to let them know about the change in types.

If this crate was already nearly at a "finished" state I would probably not care so much about Gray and GrayAlpha but considering we are already agreed on changing quite a bit for 0.9 with the new Pixel trait it would seem a waste not to take advantage of the quasi-breaking release to also fix this smaller issue if possible.

ripytide commented 2 months ago

I don't think this change would be too impactful considering that the two types are already in the alt module and don't have particularly widespread usage: https://grep.app/search?q=rgb%3A%3Aalt%3A%3AGRAY shows only 6 publicly visable projects using either type.

kornelski commented 2 months ago

.a for GrayAlpha would be great, but the other field seems like shifting one inconsistency into another inconsistency:

rgba.r = r;
rgba.g = g;
rgba.b = b;
rgba.a = a;

aligns in expressions, but

gray.gray = gray;
gray.a = gray.a;

is not aligned.

ripytide commented 2 months ago

Excellent points. I think I'm swayed.

Your points did make me think of another possible solution that might be maximally consistent with all the other pixels, how about this?

struct L<T> {
    l: T,
}

struct La<T> {
    l: T,
    a, T,
}

Where the l stands for luminance which I think is a fairly common assignment in colour theory and can be explained as such in the docs.

kornelski commented 2 months ago

L as a type name is weird tho.

Non-linear luminance in color spaces like L*a*b* use a capital L. Possibly because .l is confusable with .1.

ripytide commented 2 months ago

L as a type name is definitely weird and may look like a generic type parameter which is a disadvantage.

The l Vs 1 confusion isn't a big issue I think since any programming font worth its salt makes them visually distinct which is how they'd normally be viewed.

ripytide commented 2 months ago

Another possibility I can think of:

struct GrayA {
    gray: T,
    a: T,
}

Which I think is mildly more consistent than being called GrayAlpha.

ripytide commented 2 months ago

There is also the option of:

struct GrayA<T, A>(T, A)

Which could be done completely non-breakingly.and would be consistent with the type synonyms GrayA8.

ripytide commented 2 months ago

Thanks for discussing this issue with me, now I'm much happier knowing that the current option is not sub-optimal but rather that there are many tradeoffs involved and so different options are better in different ways.

ripytide commented 2 months ago

For posterity I'd also like to add a hybrid of the previous options, possibly my favorite so far:

struct GrayA<T> {
    l: T,
    a: T,
}
struct Gray<T> {
    l: T,
}
kornelski commented 2 months ago

GrayA(0,1) is a good idea, and can be backwards compatible.

.l would be better in Luma. In Gray that's inconsistent that struct is named after one thing, and the field after another.

ripytide commented 2 months ago

Another idea: Since GrayA and LumaA are both alternatives that I can imagine people could reasonably prefer either variant LumaA for consistency with the l field and GrayA for consistency with the well-known Grayscale terminology, how about we provide both via re-exporting a type alias?

pub struct LumaA<T, A> {
    l: T,
    a: A,
}
pub type GrayA<T, A> = LumaA<T, A>
// and the same for Luma and Gray

This way each user can choose for themselves what name they want to use in their code that is most readable to them while also having 100% interoperability since they are both the same type under the hood?

kornelski commented 2 months ago

There are two separate cases here:

  1. what to do about upgrade from v0.8
  2. what the ideal API should be like (if compatibility wasn't an issue)

Back compat

For 0.8 renaming GrayAlpha to GrayA or LumaA via type aliases is easy, but it needs to keep .0 and .1. Sadly there are no field aliases in Rust. At best there can be .l() and .a() getters.

Alternatively, v0.9 could add a brand new Luma type with .l, while keeping Gray.0 as a separate type for back-compat, and users would be asked to migrate. Addition of Luma as an option would not force anybody to upgrade immediately. Simply changing Gray to have .l in v0.9 would be annoying to upgrade - in mixed codebases there would be two incompatible types with the same name, and no option to avoid making changes immediately.

Ideal 1.0 API

To me both Gray/GrayA and Luma/LumaA are okay as type names.

So for the 1.0 goal I'd opt to either keep Gray.0 or go for Luma.l. There's doc(alias) feature that can make gray/grey work in docs search. I think the crate should just pick one name. People can add their own pub type Gray<T> = Luma<T> if they're really bothered.


I've realized that if Rust supported field name aliases, the whole problem would go away, and these structs could be both back-compatible, and have whatever .l or .gray or .grey however one wants:

https://internals.rust-lang.org/t/name-aliases-for-struct-fields-and-enum-variants/21041

ripytide commented 2 months ago

Yep, I'd planned on adding another type for v0.9 and deprecating the old one like what std is doing with the range types.

Another possible reason for migrating away from a tuple struct that I don't think has been mentioned yet: the theoretical ALuma/AGray type which would be an absolute nightmare to work with as changing from GrayA to AGray would also swap .0 with .1 which would be incredibly bug-prone unlike .l and .a which would still work after changing the type similar to switching Rgb to Bgr and others.

Regarding Gray vs Grey I consider Gray to be the "correct" spelling since that is the American spelling and most programming languages tend to use American English, probably since there are more American programmers than British and consistency is valuable.

I'm happy with only offering Luma versions and not offering a Gray type alias but on the other hand I don't see much harm in it either since if users don't want it then they won't use it but if they do want it then it saves them having to manage their own alias.

kornelski commented 2 months ago

Great suggestion in the forum thread:

You can hack partial syntax compatibility together by having one type Deref to the other via pointer cast.

impl Deref for GrayAlpha { type Target = LumaA } would make .l and .a work on it!

ripytide commented 2 months ago

https://en.wikipedia.org/wiki/Luma_(video)#Luma_versus_relative_luminance

Luma vs Luminosity: oh no, not more jargon discrepancy :frowning:

kornelski commented 2 months ago

Yup.

kornelski commented 1 month ago

Gray.v it is, with re-export and deref tricks for back compat