Skip to content

Don't apply ghost effect to "color is touching color" drawable #576

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

Resolves

Resolves #575

Proposed Changes

This PR re-adds the effectMask parameter to sampleColor4b and transformColor, and uses it to mask out the ghost effect for the target drawable of isTouchingColor.

Reason for Changes

#515 made it so that the ghost effect multiplies all color channels by the ghost effect, not just the alpha value. Because of this, if the ghost effect is applied to a drawable, its colors will change, making the color mask invalid.

This was already fixed on the GPU path (which is why the tests pass), but I overlooked it for the CPU path.

Test Coverage

Waiting on #537 for CPU/GPU parity tests

@adroitwhiz adroitwhiz changed the title Don't apply ghost to "touching color" drawables Don't apply ghost effect to "color is touching color" drawable Mar 22, 2020
@fsih fsih self-assigned this May 7, 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.

Code looks good! If we can get #537 in soonish and add a regression test here then I'd like to do that before merging this PR, but if #537 ends up being blocked or delayed then I think we should go ahead and merge this. We can always add regression tests later if that happens.

Copy link
Contributor

@fsih fsih left a comment

Choose a reason for hiding this comment

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

LG! We are taking a look at https://github.com/LLK/scratch-render/pull/537/files now and might want to wait on tests

@fsih
Copy link
Contributor

fsih commented May 7, 2020

oops beaten to it

@cwillisf
Copy link
Contributor

For future reference: we ended up replacing #537 with #604 but CPU/GPU testing is no longer blocked

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.

👍

@cwillisf cwillisf assigned adroitwhiz and unassigned fsih May 11, 2020
@adroitwhiz adroitwhiz merged commit d8c9f33 into scratchfoundation:develop May 11, 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.

"color () is touching ()" block does not work properly on ghosted sprites
3 participants