search

linebender / piet

An abstraction for 2D graphics.
Apache License 2.0
1.25k stars 94 forks source link

Derive `Copy` for `Color` #524

Closed fwcd closed 2 years ago

fwcd commented 2 years ago

A small patch that derives the Copy trait for Color making it more convenient to use as a value and also permits embedding it in other Copy structures.

cmyr commented 2 years ago

We declined to do this because it turns what is currently an implementation detail (that a color is just an int) into part of the actual API, which may not be desirable. For instance, in the future a color might hold a reference to some color space object, which might not implement Copy.

It now seems unlikely that piet is going to evolve in that particular direction, but this was the original motivation, and I still sort of like this way of thinking. Is the absence of Copy causing you a real headache?

fwcd commented 2 years ago

Not terribly much, but it prevents me from using it in a few otherwise Copy structs where it would look nicer than cloning/referencing all over the place. So I would prefer having Copy here, but if you would rather not expose this implementation detail, I would be fine with that too. Intuitively, a Color feels like it should be Copy since it's such a small/simple construct.

raphlinus commented 2 years ago

I'm of a bit of a mixed mind. The original motivation for not doing Copy is to possibly support HDR colors, which might actually be complex datatypes, though it would also be possible to design a Color type that is copy and also supports HDR.

All that said, Piet is going into maintenance mode, so that thinking is YAGNI and I can appreciate a small quality of life improvement here. So I would be ok with taking this.

xStrom commented 2 years ago

YAGNI seems prudent. I think this is a nice change for Color as it stands now. If complex color is added in later, a major version increase can help deal with the API breakage.

To continue with YAGNI let's fix the clippy errors by changing the method signatures to pass Color by value.

fwcd commented 2 years ago

I've updated the methods to pass Color by value where possible, note that this is a breaking change of the API, however.

xStrom commented 2 years ago

We've got v0.6.0 coming up soon so this is the perfect time for an API change. The leaner API makes sense in light of Color likely remaining lightweight for now.

Also, there seem to be a few remaining compilation issues.