Closed ripytide closed 3 months ago
The only remaining failing CI check is the deny(public_private_dependencies)
lints failing for all the pixeli
trait bounds which seems fine to me since its a public dependency? Can we add an exception for it somehow?
Thanks for your interest in this crate. This change is far too large and has way too many breaking changes to review/iterate on. Breaking changes and big API additions need to be discussed and agreed on individually before they can be added. And in any case, we've generally been aiming to go 1-2 years between breaking releases
Why are we so scared of breaking changes that we can't make improvements? That seems very counter-productive and unhealthy for the long-term sustainability of the project. Technical debt will only build the longer we postpone important breaking changes.
And most of these changes have been discussed in #1523, with the rest just from a waterfall effect.
Just to be clear here, this PR would solve:
But if you think not making breaking changes is more important to users, then that is your decision to make I suppose.
I'd be interested to your opinion @HeroicKatora on this topic if you have time.
I don't think this addresses enough of major points raised by #1523 to warrant the breakage either. This PR changes the representation which the least problem here. Declaring a new pre-0.*
crate as a compatible solution is a non-starter, there already is rgb
with several advantages such as its bytemuck integration allowing many opaque (not mentioned in type-bounds) uses.
Designing around the easy goals won't work, as it is the harder goals that break the current design, too. The two key points missing are awareness of color spaces and single responsibility. Instead, this design encodes even more information into the type system and it will run into the same troubles. The Pixel
, if indeed one even supposes such a trait is needed, needs foremost a clear design boundary, but this offers even less of one.
There are a few interesting pieces here and there:
*Luma*
to *Gray*
might be good for consistency.Component
is a better wording than Subpixel
.ContiguousPixel
suggests a NonContiguousPixel
which is a pretty obvious non-goal for the simple kinds of buffers that DynamicImage
tries to offer as variants. There's definitely a conflict in design here. You're not going to be able to semantically encode all existing texel layouts in types (at least not in Rust).FromPixelCommon
raises an interesting point. If we explicitly close the DynamicImage
design against new variants, we do have a static 'top' element on our pixel conversion lattice and it's not Rgba<u8>
but rather Rgba<f64>
. Then DynamicImage
should probably use that one. But also, if we close the conversion trait anyways we could do type-id dispatch instead of relying on associated methods in traits, which would probably be extremely more performant.If you take one key point, then let it be the one summary also raised during gfx
's lessons learned to wgpu
: It's not user-friendly to encode all of this in the type system, and algorithms built onto the representation can be (much) more optimal if they get to decide the mode of access (such as: bringing lookup-tables for color conversion instead of From<_>
traits not having any such parameters). image
's own design still pre-dates this by quite a bit and if there be breakage then for the sake of fixing this. (But also, DynamicImage
tries to represent a simplified view that shouldn't be so affected by the underlying reasons, so no breakage-for-breakage sake).
BTW we've had a similar conversation about the rgb
crate. I'm interested in adding such improved Pixel
trait to it, but also want to preserve backwards compatibility with the current structs.
gfx's lessons learned to wgpu: It's not user-friendly to encode all of this in the type system, and algorithms built onto the representation can be (much) more optimal if they get to decide the mode of access
Do you have more info on this?
See this whole talk, but particularly starting from here: https://youtu.be/m0JgF5Wb-dA?t=395&si=XoK-TroLp4idLt2g
Repeating some feedback given offline:
The breaking changes aspect of the PR was a concern, but there were actually a few bigger factors...
Multi-thousand line PRs are too big: The larger a PR is the more rounds of feedback it takes to review and the longer each of those reviews takes. There's a good chance the reviews needed for a PR of this scale would need 10+ hours of maintainer time. And even if some parts are ready after a couple rounds of back-and-forth, there's still a very high chance of all the work being wasted because the PR stalled out before everyone reached agreement on the remaining details.
API changes need to be discussed individually: A single GitHub PR is a bad place to try to discuss a lot of different changes. Every API change has tradeoffs and questions to address, even when it isn't a breaking change.
The two key points missing are awareness of color spaces and single responsibility. Instead, this design encodes even more information into the type system and it will run into the same troubles.
Good point, I didn't tackle the color space issue at the same time, in that regard the four choices I can think of for attaching color-space to pixels are:
enum ColorSpace {
Unknown,
Linear,
...
}
struct Rgba<T> {
r: T,
g: T,
b: T,
a: T,
color_space: ColorSpace,
}
struct ImageBuffer<P> {
pixels: Vec<P>,
color_space: ColorSpace,
}
PhantomData<U>
with a generic tag-type like eudlid
Point
types.PhantomData
tags as well.As you say the type-system heavy approaches in 3. and 4. are harder to reason with and learn, (is that similar to how gfx
did things?). 1. Seems terrible for storage space which leaves options 2. as probably the best option given all the tradeoffs I think but I don't know if you had something else in mind?
With option 2. I think FromPixelCommon
would change to something like:
trait FromPixelCommon<P> {
fn from_pixel_common(pixel: P, source_color_space: ColorSpace, destination_color_space: ColorSpace) -> Self;
}
Am I understanding correctly?
On the single responsibility aspect, which bit of this design goes did you see going against that principle? Are you suggesting moving the helper functions such as component_array()
color_component_array()
and the map_*()
functions to an extension trait? I think I'd probably be fine with that but I'm not sure as to the advantages/disadvantages of separating them either.
This is an overhaul of the crate to extract out the
Pixel
types and traits and improve them simultaneously into thepixeli
crate. This would fix #1523 and possibly many of its linked issues.Summary of Changes:
pixeli
crate which becomes a dependencyRgb([T; 3])
to#[repr(C)] struct Rgb<T> {r: T, g: T, b: T}
which is nicer to work with (pixel.g
vspixel.0[1]
)pixeli
dependecy since users will often need to use those types/traits for doing pixel related stuffLuma
is renamed toGray
as that is more understandablePixel::Subpixel
is renamed toPixel::Component
since subpixels are used with physical monitors for the rgb components and so the alpha component doesn't fitDynamicImage
variants needed to be added to pass the testsGray32F
andGrayAlpha32F
which removes the edge condition for thegrayscale()
function not actually convertingRgb32F
image variants to grayscale.channels4()
legacy methods.convert_generic_image()
I understand this is huge PR, but for this to be an atomic PR there wasn't really a way of splitting it any smaller. I also understand that this probably is a breaking change in many different areas, but I think that's acceptable given the scale of improvement this PR offers.
Btw the
pixeli
crate is pretty cool in my biased opinion, the newPixel
trait is wayyy more powerful than the previous one (credit to @johannesvollmer for some of the inspiration (#1523) and @kornelski for the related discussions in thergb
crate) and solves lots of previous issues, (like nowmap_components()
can map to a different component type!).