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

Update Alpha Setting for Color #547

Merged
merged 3 commits into from
Dec 19, 2018
Merged

Update Alpha Setting for Color #547

merged 3 commits into from
Dec 19, 2018

Conversation

epeach
Copy link
Contributor

@epeach epeach commented Dec 6, 2018

Issue: #522

To set the alpha of the palette colors in the rainbow, I previously accessed the array of RGBA values in the p5.Color object. This update removes that 'hacky' method and instead creates a new p5.color object with the same RGB values as the old color and a manually set alpha value.

@epeach epeach requested a review from joshlory December 6, 2018 20:30
src/Effects.js Outdated
@@ -58,6 +58,11 @@ module.exports = class Effects {
return p5.lerpColor(p5.color(prev), p5.color(next), remainder);
};

/* Given a p5.Color object, creates a new color object with the given alpha value*/
const setAlphaForColor = (color, alpha) => {
return p5.color(color._getRed(), color._getGreen(), color._getBlue(), alpha);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use p5.red(color) etc. here?

Copy link
Contributor

@joshlory joshlory left a comment

Choose a reason for hiding this comment

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

Looks good! Is there any perf downside of calling p5.color() every frame? I noticed a small slowdown in the Disco Ball effect, but that could've been because we're parsing a hex color (vs. passing in r/g/b/a args).

@codecov-io
Copy link

Codecov Report

Merging #547 into master will increase coverage by 0.03%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #547      +/-   ##
==========================================
+ Coverage   34.45%   34.48%   +0.03%     
==========================================
  Files          23       23              
  Lines        3381     3378       -3     
==========================================
  Hits         1165     1165              
+ Misses       2216     2213       -3
Impacted Files Coverage Δ
src/Effects.js 41.98% <66.66%> (+0.16%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ab7100c...9b9d326. Read the comment docs.

@epeach
Copy link
Contributor Author

epeach commented Dec 18, 2018

@joshlory How bad was the performance degradation? I can definitely understand that this change would cause that and depending on the impact, it might be worth exploring an alternative workaround.

@joshlory
Copy link
Contributor

I think Disco Ball is our slowest effect, but I'm not sure how much of that is p5.color vs. too many draw calls. We should see any regression in the automated Appium tests, so I wouldn't worry about this too much!

@epeach epeach merged commit b3b387c into master Dec 19, 2018
@epeach epeach deleted the remove-hacky-alpha-setting branch December 20, 2018 21:42
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.

3 participants