Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add list support for lerpColor like other color functions #6954

Merged
merged 3 commits into from
Sep 17, 2024

Conversation

RandomGamingDev
Copy link
Contributor

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

@davepagurek
Copy link
Contributor

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.

@RandomGamingDev
Copy link
Contributor Author

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.

@ashish1729
Copy link
Contributor

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?

@RandomGamingDev
Copy link
Contributor Author

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.

@davepagurek
Copy link
Contributor

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

@RandomGamingDev
Copy link
Contributor Author

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
Copy link
Contributor Author

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).

@davepagurek
Copy link
Contributor

That works, thanks!

src/color/creating_reading.js Outdated Show resolved Hide resolved
Copy link
Contributor

@davepagurek davepagurek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@davepagurek davepagurek merged commit 17fe7b0 into processing:main Sep 17, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lerpColor doesn't support list based colors despite other functions supporting it
3 participants