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

lerpColor in HSL or HSB mode now can loop the color wheel if needed #6708

Merged
merged 3 commits into from
Jan 14, 2024

Conversation

Bumblebee00
Copy link
Contributor

Resolves #6436

Changes:

I modified p5.prototype.lerpColor in src/color/creating_reading.js. Now if the color mode is HSL or HSB the interpolation of the HUE value will proceed along the shortest path on the color wheel. So for example from hue 359 to hue 1 it will increase up to 360 then wrap at 0 and increase up to 1. I tried implementing this as suggested in the issue discussion (converting the color to vector and the using slerp), but I found another simpler solution that works.

Screenshots of the change:
Lerping from 340 to 20 before:
Screenshot 2024-01-07 alle 15 12 57
Lerping from 340 to 20 now:
Screenshot 2024-01-07 alle 15 14 31

PR Checklist

… wheel (so wrap around from hue=360 to hue=0 if needed)
Copy link

welcome bot commented Jan 7, 2024

🎉 Thanks for opening this pull request! Please check out our contributing guidelines if you haven't already. And be sure to add yourself to the list of contributors on the readme page!

@davepagurek
Copy link
Contributor

davepagurek commented Jan 7, 2024

Thanks for implementing this @Bumblebee00! I think this implementation makes sense. Just a heads up, some tests are currently failing. Could you take a look and double check that these are all because the old value being checked for was lerping the wrong way, and update the tests if so? Here's the output from the test runner:

Test output
4 failing
  1) color/CreatingReading
       p5.prototype.lerpColor
         should correctly get lerp colors in HSL:

      AssertionError: expected [ 190, 44, 63, 255 ] to deeply equal [ 66, 190, 44, 255 ]
      + expected - actual

       [
      +  66
         190
         44
      -  63
         255
       ]
      
      at assert.deepEqual (http://localhost:9001/node_modules/chai/chai.js:2367:32)
      at Context.<anonymous> (unit/color/creating_reading.js:162:14)

  2) color/CreatingReading
       p5.prototype.lerpColor
         should correctly get lerp colors in HSB:

      AssertionError: expected [ 192, 47, 66, 255 ] to deeply equal [ 69, 192, 47, 255 ]
      + expected - actual

       [
      +  69
         192
         47
      -  66
         255
       ]
      
      at assert.deepEqual (http://localhost:9001/node_modules/chai/chai.js:2367:32)
      at Context.<anonymous> (unit/color/creating_reading.js:169:14)

  3) color/CreatingReading
       p5.prototype.lerpColor with alpha
         should correctly get lerp colors in HSL with alpha:

      AssertionError: expected [ 190, 44, 63, 99 ] to deeply equal [ 66, 190, 44, 99 ]
      + expected - actual

       [
      +  66
         190
         44
      -  63
         99
       ]
      
      at assert.deepEqual (http://localhost:9001/node_modules/chai/chai.js:2367:32)
      at Context.<anonymous> (unit/color/creating_reading.js:200:14)

  4) color/CreatingReading
       p5.prototype.lerpColor with alpha
         should correctly get lerp colors in HSB with alpha:

      AssertionError: expected [ 192, 47, 66, 99 ] to deeply equal [ 69, 192, 47, 99 ]
      + expected - actual

       [
      +  69
         192
         47
      -  66
         99
       ]
      
      at assert.deepEqual (http://localhost:9001/node_modules/chai/chai.js:2367:32)
      at Context.<anonymous> (unit/color/creating_reading.js:207:14)

I'm going to tag the color stewards to double-check, are any of you able to also take a look? @paulaxisabel, @SoundaryaKoutharapu, @mrbrack, @TJ723, @Zarkv, @SkylerW99, @ramya202000, @hannahvy, @robin-haxx, @hiddenenigma Technically this would change the behaviour of existing sketches. It feels like the previous behaviour would be considered unexpected or a bug, so updating the behaviour makes sense to me, but it'd be great to get your feedback on this make sure we don't need to keep around the old behaviour too.

@Bumblebee00
Copy link
Contributor Author

Sure! sorry I didn't see them before. Yep they seem to be incorrect because before it was lerping the wrong way. In fact the test are failing are only in HSL and HSB mode, not RGB mode. Furthermore the tests with alpha value are failing because of the color, but the alpha values are correct. To double check everything:
Screenshot 2024-01-07 alle 17 57 13
Here is the first test. It starts from the color on the left and goes to the one on the right, checking the color lerped at 0.33 and 0.66. I put some labels to se the color that was in the test (old one) and the one I changed it with (new one). Now the tests should be updated.

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 for updating the tests! I think this looks good to me, so I'm going to merge this one in.

@davepagurek davepagurek merged commit a32f909 into processing:main Jan 14, 2024
2 checks passed
@davepagurek
Copy link
Contributor

@all-contributors please add @Bumblebee00 for code

Copy link
Contributor

@davepagurek

@Bumblebee00 already contributed before to code

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.

colorLerp cycles hues linearly without considering the hue spectrum is a circular wheel
2 participants