Open KyleMaas opened 1 year ago
Thanks for the issue, this is a bit of a gap in the library currently and I understand not wanting to use another dependency.
The kmeans calculation is done on a floating point component type. When I started this crate, image
didn't support f32
images so I never thought about supporting their color struct before. The palette
crate is convenient for me because of being to easily convert to Lab
and Rgb<f32>
. I also helped to optimize the component conversion between u8
and f32
.
I was originally a little hesitant to support image::Rgb
because
but I see the value in adding it for users. The current implementation is written in a less clear way than it used to be so it's harder to implement for your own types. I'll work on adding documentation for implementation and annotating the current implementation with comments. This was the previous version of the Srgb
implementation which was a bit easier to copy/paste and replace for your own type
https://docs.rs/kmeans_colors/0.5.0/src/kmeans_colors/colors/kmeans.rs.html#88-162
After this is implemented, you'll need to use image::Rgb<f32>
for kmeans. If you have an RgbImage
you'll need to wrap it in a DynamicImage
variant and then use into_rgb32f
because RgbImage
defaults to Rgb<u8>
. Otherwise, image::open
into an image::Rgb32FImage
.
Another thought I had was implementing it for &[[T; 3]]
so you wouldn't need the image::Rgb
feature but I'll probably get around to that later.
I'll try to add an implementation for image::Rgb<f32>
or image::Rgb<T: num_traits::Float>
(in case f64
is supported some day in image
).
Awesome! That would save a bunch of the conversion I'm having to do right now. Thanks!
By the way, this is what I'm using it for:
https://github.com/KyleMaas/realworldarchive
Specifically, the palette detection:
The code quality is horrible on this project - it's very much experimental at this stage - but it does work pretty well.
I mean, heck, if you could cluster directly on HSL pixels, that'd be pretty awesome, too. But I'd think that would be of far less utility to most people.
I mean, heck, if you could cluster directly on HSL pixels, that'd be pretty awesome, too. But I'd think that would be of far less utility to most people.
That's why I'll probably implement the traits for something like [T; 3]
. It'd have the benefit of being able to use the same impl for any arbitrary 3 component float color type (RGB, HSL, HSV, etc.) in a [f32; 3]
array format. You could use bytemuck
to cast from a &[f32]
(that you'd get from calling ImageBuffer::as_raw
on an Rgb32FImage
) to a slice of arrays, or a buffer you made of HSL colors in the image. playground link
use bytemuck; // 1.13.1
use image::{ImageBuffer, Rgb32FImage}; // 0.24.6
fn main() {
let data = vec![0_f32, 1., 2., 3., 4., 5.];
let x: Rgb32FImage = ImageBuffer::from_raw(2, 1, data).unwrap();
let y: &[[f32; 3]] = bytemuck::try_cast_slice(x.as_raw()).unwrap();
dbg!(y); // [[0.0, 1.0, 2.0], [3.0, 4.0, 5.0],]
}
Thanks for sharing your code, that gives me a better idea of what you're using it for.
For the convergence factors, I wish I had a better resource to point to but I ended up at those numbers from experimentation.
I know you're interested in removing dependency on palette
but I had a few quick things to say on that. The errors can be slightly hard to understand at first but the maintainer is really helpful about answering questions if you have any in discussions. palette
supports converting to/from Hsl
and to/from u8
/f32
easily so I find it convenient, but if what you're using already covers what you need then there's no need to use it too. I typically do a lot of things with Lab
, so my workflow with image
is to cast the decoded image file from Rgb
to the palette
types right away and save images by passing a Vec<u8>
to the encoders so I never have to touch image::Rgb
.
Two little things that probably won't matter anyway if you swap out palette
but figured I'd share.
into_format
should convert from u8
to f32
for you
https://github.com/KyleMaas/realworldarchive/blob/89655e1a91eaea5600829a888ff95100c42c8947/src/color_multiplexer.rs#L131
.map(|x| Srgb::new(x[0], x[1], x[2]).into_format()) // might need ::<_, f32>() annotation
This part could probably be rewritten to something like the following. In general with image::Rgb
, I find it easier to construct my own [u8; 3]
elsewhere and then pass it to the tuple as a single variable.
https://github.com/KyleMaas/realworldarchive/blob/89655e1a91eaea5600829a888ff95100c42c8947/src/color_multiplexer.rs#L164-L168
let color: [u8; 3] = c.into_format().into();
colors_rgb.push(Rgb(color));
colors_hsl.push(HSL::from_rgb(&color));
I've done a first pass at #56. With this, you'll be able to use it for Hsl
but you'll have to normalize the hue from [0.0, 1.0]
if it's [0, 360]
. (This is a shortcoming I noticed in the current design of the library while adding this, but it's a bigger change I want to address in a breaking version in the future)
I have a separate enhancement I want to investigate, but I should be able to push a new update so you don't need palette
within the next few days.
Cool. Thank you for the pointers. It'll be a little bit before I can test this, but I'll let you know. That'd be great if I could cluster on HSL without normalization - is it possible you could file an issue on that refactor so I can subscribe to it and know when you have it working?
In trying to use
get_kmeans()
, I attempted to run it on a list ofimage::Rgb
pixels, but it seems to only takepalette::lab::Lab
orpalette::rgb::rgb::Rgb
. Is it possible this could be implemented forimage::Rgb
so I can skip using thepalette
library?