Skip to content

Change brightness effect to match Scratch 2.0 in 2D mode #399

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
merged 1 commit into from
Jan 16, 2019

Conversation

cwillisf
Copy link
Contributor

@cwillisf cwillisf commented Jan 16, 2019

I wasn't sure who would want to review a GLSL change so I just assigned a bunch of people. Feel free to unassign yourself if you're not interested :)

If you're unfamiliar with GLSL or just need a refresher, take a look at pages 3 & 4 of this PDF:
https://www.khronos.org/files/webgl/webgl-reference-card-1_0.pdf

Resolves

Resolves #142

Proposed Changes

Previously this repository's brightness effect matched the behavior of Scratch 2.0's 3D renderer by adjusting the value component of the HSV representation of a color. Now, it matches the behavior of Scratch 2.0's 2D renderer, which simply offsets (and clamps/saturates) the RGB values.

Note that to fully match Scratch 2.0's 2D renderer the brightness effect must happen after the color effect, not before.

Reason for Changes

It appears that most uses of the brightness effect in Scratch 2.0 are not paired with other graphical effects which cause the 3D renderer to activate, so matching the 2D renderer will likely cause fewer projects to display incorrectly in Scratch 3.0.

Test Coverage

Testing is easiest by comparing the Scratch 2.0 renderer against the scratch-render playground. See also #142 for example projects to test.

@cwillisf
Copy link
Contributor Author

Note that adding ?w=1 to the URL of the diff view will tell GitHub's diff to ignore whitespace differences and make the diff much more readable for this change :)

Copy link
Contributor

@paulkaplan paulkaplan left a comment

Choose a reason for hiding this comment

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

LGTM. It seems like we are bound to make someone upset with this change, I just can't tell which group is bigger (this change will re-break the original issue it is linked to #142)

But code LG!

@cwillisf
Copy link
Contributor Author

After talking with both @paulkaplan and @rschamp I think the right move is to merge this for now and then consider more complicated solutions if we receive a lot of complaint.

@cwillisf cwillisf merged commit e54b590 into scratchfoundation:develop Jan 16, 2019
@cwillisf cwillisf deleted the fix-brightness-effect branch January 16, 2019 20:22
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.

6 participants