Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert Pixel types into corresponding types from rgb crate. #1383

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

msmorgan
Copy link

This allows their components to be accessed by name. For example,
if c is Rgb<u8>, one can write c.r instead of c.channels()[0].

I license past and future contributions under the dual MIT/Apache-2.0 license,
allowing licensees to chose either at their option.

@HeroicKatora
Copy link
Member

HeroicKatora commented Dec 15, 2020

Interesting idea. Maybe we should deref into the types of the rgb crate instead of defining our own? It's possibly one of the most stable crates for the domain and would improve interop, avoiding the situation of the xkcd comic in its Readme :) The biggest difference being that its gray scale types for some reason don't seem to use the same structure but are a tuple …

@fintelia
Copy link
Contributor

Worth pointing out that you can do c.0[0] instead of c.channels()[0], but obviously c.r is nicer.

Not thrilled that this would add more unsafe to the crate. I don't see anything unsound though as far as I can tell.

@msmorgan msmorgan force-pushed the image_component_deref branch 2 times, most recently from 8a9bef9 to 413e118 Compare December 15, 2020 16:16
@msmorgan msmorgan changed the title Add types for Pixel structs to auto-deref into. Convert Pixel types into corresponding types from rgb crate. Dec 15, 2020
@msmorgan
Copy link
Author

@HeroicKatora Done.

Implements `Deref` targeting corresponding types from `rgb` crate.
This allows their components to be accessed by name. For example,
if `c` is `Rgb<u8>`, one can write `c.r` instead of `c.channels()[0]`.

Implements `From<rgb::Ty> for image::Ty` bidirectionally.
src/color.rs Outdated Show resolved Hide resolved
Copy link
Member

@HeroicKatora HeroicKatora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From an implementation perspective this PR has my approval. It's just that my experience with the technique of using Deref like that is limited, so I do wonder if there is any stability concern that we're overlooking? A run of rustwide against downstream crates might give more certain results, I've wanted to write some integration for this for a while now.

@msmorgan
Copy link
Author

I was inspired by nalgebra crate, which uses Deref to expose convenience properties for Matrix (and vectors), and for Point.

@fintelia
Copy link
Contributor

I started looking at the code this generates and I'm starting to get worried this is too messy/confusing. After this change we'd have structs Rgb and RGB, crate rgb, enum variants Rgb8 and Rgb16, and type aliases RGB8 and RGB16. Yes, our pixel struct Rgb would transparently deref to RGB, but the reverse would require an into() call. To make matters worse, the change adds a couple methods to our pixel structs which return types from the rgb crate.

The core issue is that Rust doesn't really support the ability to have multiple structs that can be used interchangeably. If we want to support nice field accesses, I think the right way to do it would be to commit to a breaking change. It would take more discussion but I do think a strong case could be made for adopting the standard rgb crate types outright (though perhaps wait for them to reach 1.0 first?).

@HeroicKatora
Copy link
Member

It would take more discussion but I do think a strong case could be made for adopting the standard rgb crate types outright (though perhaps wait for them to reach 1.0 first?).

They have been on 0.8 for about three years now, iirc. It's slightly unfortunate but as a 'compatibility crate' they are more or less bound to that version for now but could re-export those types as 1.0 at some point by a dependency on the old version.

@msmorgan
Copy link
Author

@fintelia

After this change we'd have structs Rgb and RGB, crate rgb, enum variants Rgb8 and Rgb16, and type aliases RGB8 and RGB16.

rgb::RGB<T> and rgb::RGB8 are not publicly exported by image, so they shouldn't add too much confusion for users. In the docs, the associated type <Rgb<T> as Deref>::Target will show up as belonging to a different crate.

Yes, our pixel struct Rgb would transparently deref to RGB, but the reverse would require an into() call.

This is true if you have a value of rgb::RGB<T>, but Deref produces a reference, &rgb::RGB<T>, with the same lifetime as the borrow by the deref function. There would be no need to convert back, since the owned value is still an image::Rgb.

@fintelia
Copy link
Contributor

Here's a screenshot that shows what this looks like in the generated documentation. I don't see anything that indicates that Rgba and RGB / RGBA are from different crates:

image

@msmorgan
Copy link
Author

Hmm, that is confusing. You'd be directed to the rgb crate's type if you click on "RGBA". I wonder if there's a way to hide all those methods though. The types from nalgebra that vector and point deref into have very few methods on them so they don't cause so much documentation noise.

@msmorgan
Copy link
Author

I added a commit that leaves the From impls intact for conversion to/from the rgb crate types, but adds simple proxy structs as Deref targets for component access. Let me know what you think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants