Color implementation

[etfrogers]

This is a (currently partial) implementation of some of the color types. More are planned to follow.

The main reason for starting a partial PR is to ask if the changes to the internal API are reasonable: the removal of the new_plot_n() functions and effective replacement with a generate_data macro.

(Also this has snowballed a bit and led me to try implementing several new plot types as I started trying to reproduce some of the gnuplot demos as my examples - those should probably come as separate PRs at some point)

Addresses #105 #86

[etfrogers]

Come to think of it, this macro would allow removal of the box_xxx_set_width() functions (simplifying the external API) in favour of a BoxWidth(Vec<f64>) PlotOption

[SiegeLord]

Sorry I didn't get to this this weekend. I'll try to look at it soon.

[etfrogers]

Think this is ready for discussion now (though no rush).

Happy to take feedback: I want to make it work for what you want and need for the project.

[etfrogers]

Note that a couple of types are unfinished: these need finishing (if I can work out what they do) or removing.

[SiegeLord]

Alright, I have many little comments but I wanted to start with a high level API one. I really don't like how the ColorOpt/Color came out. I don't want to have weirdly capitalized functions masquerading as enum variants. I'd rather bite the bullet, and force the user to type .into() for everything. I.e. I'm suggesting:

enum ColorType<T>
{
  RGBString(T),
}

enum PlotOption
{
  Color(ColorType<T>)
}

...

[Color("red".into())]

While it requires more characters, I think it's easier to understand what is happening. Calling .into(). on arguments is a common sight in Rust code.

[etfrogers]

That makes sense. I think I tried too hard to keep it to the same interface as before, but happy to make the change you suggest.

[etfrogers]

Done (assuming tests pass). I think perhaps I got a bit carried away finding a way I could create the Color() function, and forgot to think about whether I should do it. Clever code is all very well, but often (as in this case) it just makes things opaque for the user that shouldn't be.

Also, I hadn't quite clicked that Color("red".into()) was all that was needed (in spite of that being what my Color function did!). I was thinking the user would need Color(RGBString("red")) which is just a bit less easy.

[SiegeLord]

Here's some more comments. Overall this looks pretty good. The main remaining issue I have is what to do with the alpha channel (see one of the comments).

[SiegeLord]

It's very upsetting that gnuplot defines alpha backwards here (everywhere else, alpha = 0 -> fully transparent, alpha = 255 -> fully opaque). I wonder if the Rust API should invert it. I'll need to think about it.

[etfrogers]

Yes, this is a tricky one. Both options could lead to "unexpected behaviour". The fix of inverting it could be done cleanly in the data() function, if that's the route you decide to go down.

[SiegeLord]

One thing I forgot: the double string conversion is a bit excessive. I think you can make it somewhat more efficient (and imo nicer to read) by making the match patterns return the string directly, i.e.

pub fn command(&self) -> String
{
	match self
	{
		VariableRGBInteger(_) => "rgb variable".into(),
		PaletteFracColor(v) => format!("palette frac {v}"),
	}
}

[etfrogers]

Happy to make the change, and this is the way I wrote it first. I thought it was clearer with the selection logic separate from the conversion logic, but that's personal preference (and it's your project 😄 ). Fair point about efficiency though.

Strike that: having made the change it's far better your way!

[SiegeLord]

Alright, this is merged as ffd11c0 with a few fixes (and the alpha inversion) in ab2c419. I verified that everything looks the same as before this change using the new output tests I added in a7dd0af.

Thanks a lot for this, this was a lot of work and I appreciate you sticking through with it and addressing my comments.

[etfrogers]

Alright, this is merged as ffd11c0 with a few fixes (and the alpha inversion) in ab2c419. I verified that everything looks the same as before this change using the new output tests I added in a7dd0af.

Thanks a lot for this, this was a lot of work and I appreciate you sticking through with it and addressing my comments.

Thanks for engaging with it: It made a fair bit of work in reviewing as well! I like the new output tests, too.