Image: "Constructors" for Rgb<T>?

[LYP951018]

It seems that we could only construct a Rgb<T> object by

Rgb<f32> { data: [1.0, 1.0, 1.0] }

This is truly leaky abstraction, and annoying. Also, access R, G, B by index is ... annoying, too.

[Veykril]

I was taking a look at this and noticed that the define_colors! macro in the color module actually defines a function with the same name as the struct with no documentation: https://github.com/PistonDevelopers/image/blob/d000240fc263fde294236ace79e1873eaa7a20ac/src/color.rs#L65-L69
Nevertheless if the need for this is still there I'd love to make a constructor implementation for these things. I suppose they are meant to be of the signature(rgba as the example) fn new(r: T, g: T:, b: T, a: T) -> Self, what I'd like to ask is if it would be alright to change the define_colors! macro a bit to make it possible to implement them via the macro.

[mandeep]

I agree this is annoying. Also, in order to avoid having to index into each r, g, b component I'm using this let [r, g, b] = self.im.get_pixel(i as u32, j as u32).data; which is still annoying due to having to access the data attribute.

[fintelia]

This interface has actually changed on master since the last release. The constructors are gone, and .data is replaced by a tuple struct access: .0. That change also makes it possible to write your code slightly differently: let Rgb([r,g,b]) = self.im.get_pixel(...);

If you have thoughts on further tweaks we could make before releasing, this would be a good time to consider them.

[mandeep]

You can use Rgb([r, g, b]) but then you have to dereference each component if you need to do something like cast to f64. Tuple struct access would be a good compromise, however it doesn't seem to be available for me in this scenario:

let color = self.im.get_pixel(i, j);
let r = color.0;

/// this also doesn't work in case it's what you mean
let (r, g, b) = self.im.get_pixel(i, j).0;

I think my preferred way of access would be something like let (r, g, b) = self.im.get_pixel(...)

[HeroicKatora]

Not sure why you are under the impression that you would need to dereference the components. get_pixel returns an owned Rgb<u8> that is/will be ordinarly destructured. Into an array and not a tuple but I don't see the difference.

Edit: I'll actually go through the example and update them to that destructuring because it is nicer See below.

[HeroicKatora]

Ah, I see. Reworking everything to that nice destructuring is blocked by Rust 1.24.1. That may be the source of confusion, slice pattern syntax was still experimental back then. However, that will likely change pretty soon as well as this only compounds to the list of reasons to upgrade the minimum required compiler version.

[fintelia]

To clarify, this does work (edit: on current/recent versions of Rust):

let [r, g, b] = self.im.get_pixel(i, j).0;

[HeroicKatora]

@fintelia No, same problem with Rust 1.24.1

error: slice pattern syntax is experimental (see issue #23121)
  --> examples/fractal.rs:38:17
   |
38 |             let [r, _, b] = pixel.0;
   |                 ^^^^^^^^^

[mandeep]

I see. I was using let image::Rgb { data: [r, g, b] } = self.im.get_pixel(i as u32, j as u32); because when using let image::Rgb([r, g, b]) = self.im.get_pixel(i as u32, j as u32); I receive the error: expected tuple struct/variant, found function image::Rgb I'm sure I'm doing something wrong here, but not sure what.

When using let [r, g, b] = self.im.get_pixel(i as u32, j as u32).0; I receive the error: no field 0 on type &image::Rgb<u8>

[HeroicKatora]

@mandeep We're both talking about what is currently on master and what is going to be released (as 0.22) next, not about what is released in 0.21.1. You're probably utilizing the version from crates.io.

If you want to try it, you can use

image = { git = "https://github.com/image-rs/image" }

[mandeep]

@HeroicKatora gotcha, thanks!

Also in that case, maybe we can revisit the tuple struct access?

[HeroicKatora]

You mean something like

impl<T: Copy> Rgb<T> {
    pub fn r(self) -> T {
        self.0[0]
    }
}

[mandeep]

Yep, or even providing a method that returns (r, g, b).

[fintelia]

I'm not sure I'm following why a tuple of (r,g,b) is preferable to an array. All the components are the same type, and with an array you can iterate over them if you want. Providing ways to access both would just clutter the interface

[mandeep]

@fintelia I agree that it doesn't matter what is returned, but I don't like how I have to use .0 to do so.

[HeroicKatora]

We're not going to turn the type into a [T; 3] itself due to obvious trait troubles with reusing a type from the standard library. So how is fn rgb(self) -> [T; 3] and rgb.rgb() preferrable over rgb.0 to get to the representation?

Edit: I would see the point of having better From/Into/Deref/AsRef/AsMut support though!

[mandeep]

I think it's really just an aesthetic concern. I'm not sure rgb.0 is as clear as something like rgb.components(). If I were someone not familiar with the code, I'd probably be wondering what .0 actually is. Even .data is more explicit.

[mandeep]

I gave the master branch a try. This is what I mean by having to dereference:

        let image::Rgb([r, g, b]) = self.im.get_pixel(i as u32, j as u32);
        Vector3::new(*r as f64 / 255.0, *g as f64 / 255.0, *b as f64 / 255.0)

[HeroicKatora]

Can also be a single deref. And that's an unfortunate difference between ImageBuffer and GenericImageView, huh.

        let image::Rgb([r, g, b]) = *self.im.get_pixel(i as u32, j as u32);
        Vector3::new(r as f64 / 255.0, g as f64 / 255.0, b as f64 / 255.0)

[mandeep]

Yep.

Thanks to you both for your help on this btw.