Skip to content

Update EffectTransform.transformColor to match GPU "color" + "brightness" effects #542

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

Merged

Conversation

adroitwhiz
Copy link
Contributor

@adroitwhiz adroitwhiz commented Jan 9, 2020

Resolves

Resolves #578

Proposed Changes

This PR changes EffectTransform.transformColor to match what the sprite.frag shader does with regards to the brightness and color effects.

In particular, it implements the "color" effect as a hue shift in HSV space, and the "brightness" effect as an addition in RGB space.

Reason for Changes

EffectTransform is used in the CPU "touching color" pipeline, and so it should give the exact same results as the GPU.

Test Coverage

CPU/GPU parity tests are blocked on #537

@adroitwhiz adroitwhiz force-pushed the effect-transform-parity branch from 794cd26 to 8b643b6 Compare January 29, 2020 02:00
@adroitwhiz adroitwhiz requested review from cwillisf and fsih February 5, 2020 01:37
@adroitwhiz adroitwhiz force-pushed the effect-transform-parity branch from 8b643b6 to d513b83 Compare March 26, 2020 09:03
@adroitwhiz adroitwhiz force-pushed the effect-transform-parity branch from d513b83 to 21ee6ba Compare March 26, 2020 09:25
@adroitwhiz adroitwhiz changed the title Update EffectTransform.transformColor to match GPU Update EffectTransform.transformColor to match GPU "color" + "brightness" effects Jun 25, 2020
Copy link
Contributor

@cwillisf cwillisf left a comment

Choose a reason for hiding this comment

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

Good ol' HSV vs. HSL confusion! A long tradition in Scratch ;)
Thanks for the fix! I had one set of minor suggestions but otherwise it looks good to me!

Co-authored-by: Chris Willis-Ford <[email protected]>
Copy link
Contributor

@cwillisf cwillisf 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! One minor suggestion for hsvToRgb -- I'll go ahead and apply it myself if GitHub lets me...

this makes it safer to call hsvToRgb without worrying about h=1, which previously caused incorrect results
@cwillisf cwillisf merged commit 50ff576 into scratchfoundation:develop Aug 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Touching color" blocks on CPU path don't apply brightness or color effects properly
2 participants