p5.js icon indicating copy to clipboard operation
p5.js copied to clipboard

Add list support for `lerpColor` like other color functions

Open RandomGamingDev opened this issue 1 year ago • 7 comments

Resolves #6953

Changes:

Adds list support to the lerpColor like other color functions. (I understand why this function doesn't support string based colors since you should know the actual value of the color when lerping between them, but adding list support is still definitely something that would be useful as seen in the referenced issue. However, if anyone thinks that adding string based color support to lerpColor as well would be preferred I'll add that to the PR.)

Screenshots of the change:

const red = [255, 0, 0];
const blue = [0, 255, 0];

function setup() {
  // These always worked
  fill(red);
  fill(blue);
  // This works now!
  lerpColor(red, blue, 0.5);
}

PR Checklist

RandomGamingDev avatar Apr 09 '24 13:04 RandomGamingDev

Thanks, looks good!

I understand why this function doesn't support string based colors

Do you mean this as in, we don't want to support this, or the color() function doesn't handle it? Just looking at your code, it seems like the code may actually already support string based colors. I haven't tested this, but if it does work, I'm ok adding |String to the types (and maybe mentioning in the docs that any argument to color() also works as an input) if you agree.

davepagurek avatar Sep 05 '24 12:09 davepagurek

Thanks, looks good!

I understand why this function doesn't support string based colors

Do you mean this as in, we don't want to support this, or the color() function doesn't handle it? Just looking at your code, it seems like the code may actually already support string based colors. I haven't tested this, but if it does work, I'm ok adding |String to the types (and maybe mentioning in the docs that any argument to color() also works as an input) if you agree.

I mean this as in I understand why you wouldn't support it, but I personally would prefer if it were supported. I'd be fine with adding the String type as well and yes, the function already does support String types.

RandomGamingDev avatar Sep 05 '24 15:09 RandomGamingDev

although I can see that heavy lifting is being done by the Color constructor underneath, will it still be valuable to add examples and unit tests for this update in behavior?

ashish1729 avatar Sep 06 '24 13:09 ashish1729

although I can see that heavy lifting is being done by the Color constructor underneath, will it still be valuable to add examples and unit tests for this update in behavior?

Probably not. It's pretty expected behavior.

RandomGamingDev avatar Sep 06 '24 16:09 RandomGamingDev

Agreed that it's probably OK to leave it as is. Maybe we can just add |String to the types and then merge?

davepagurek avatar Sep 09 '24 20:09 davepagurek

Agreed that it's probably OK to leave it as is. Maybe we can just add |String to the types and then merge?

Should we add lists as well and all other types that can be converted into p5.Color? Because for background() for instance, it says "p5.Color: any value created by the color() function" and doesn't list out all the types that can be converted into p5.Color, and including all those types could actually lead to more confusion. I think we should convert it to be the same as the rest of the documentation that involves colors like this and copy it from background().

RandomGamingDev avatar Sep 09 '24 22:09 RandomGamingDev

I've added that instead for now to correspond better to the rest of p5.js's documentation instead of include all types (that would create inconsistencies although it could be nicer for some stuff).

RandomGamingDev avatar Sep 09 '24 22:09 RandomGamingDev

That works, thanks!

davepagurek avatar Sep 17 '24 20:09 davepagurek