-
Notifications
You must be signed in to change notification settings - Fork 352
Fix "touching color" for background color #489
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
Fix "touching color" for background color #489
Conversation
src/RenderWebGL.js
Outdated
| }; | ||
| } | ||
|
|
||
| // TODO: figure out why this is all in a try block? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC this try-finally dates back to cwillisf's initial implementation of the code. What error is it supposed to catch?
4582935 to
faf90b8
Compare
|
This is actually failing the test I wrote on the GPU mode. At least we know the test works... |
When we construct bounds inside which we should loop over pixels, which is done in _candidatesBounds, we expect their dimensions+position to be integers when looping over them. If they aren't, weird things will occur (e.g. "phantom" touching-color results depending on the precise subpixel bounds position) that are not very fun to debug.
faf90b8 to
f09131e
Compare
|
Fixed it! I'm surprised we didn't see issues earlier-- in some places we were initializing loop counters to floating-point values! |
| /** @type {Uint8ClampedArray} | ||
| * @readonly */ | ||
| this._backgroundColor3b = new Uint8ClampedArray(3); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't remove this._backgroundColor because it never existed here-- it's first defined when setBackgroundColor is called
src/RenderWebGL.js
Outdated
| */ | ||
| setBackgroundColor (red, green, blue) { | ||
| this._backgroundColor = [red, green, blue, 1]; | ||
| this._backgroundColor4f = [red, green, blue, 1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this might generate less GC work:
| this._backgroundColor4f = [red, green, blue, 1]; | |
| this._backgroundColor4f[0] = red; | |
| this._backgroundColor4f[1] = green; | |
| this._backgroundColor4f[2] = blue; | |
| this._backgroundColor4f[3] = 1; |
| * @param {number} green The green component for the background. | ||
| * @param {number} blue The blue component for the background. | ||
| */ | ||
| setBackgroundColor (red, green, blue) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's consider removing this function entirely in a future PR
cwillisf
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 thanks!
Revert "Merge pull request #489 from adroitwhiz/touching-white-fixes"
Revert "Revert "Merge pull request #489 from adroitwhiz/touching-white-fixes""
Revert "Revert "Revert "Merge pull request #489 from adroitwhiz/touching-white-fixes"""
…ert-489 Put back in "Merge pull request #489 from adroitwhiz/touching-white-fixes"
…ert-660-revert-489 Revert "Put back in "Merge pull request #489 from adroitwhiz/touching-white-fixes""
…ert-673-revert-660-revert-489 Put back in "Merge pull request #489 from adroitwhiz/touching-white-fixes", fix touching color issue
Resolves
Resolves #488
Proposed Changes
This PR makes several changes that are all necessary to properly fix "touching color" for background colors:
RenderWebGL._backgroundColorhas been replaced withRenderWebGL._backgroundColor4fandRenderWebGL._backgroundColor3b. This makes it easier to callcolorMatcheson the background color.RenderWebGL.isTouchingColornow checks whether the color to be checkedcolorMatchesthe background color, and if it does, will not limit the check to "candidate" drawables' bounds, as the background spans the entire stage and should always be checked.backgrounddraw mode and draw region has been added, which draws the background color as a quad. This is necessary to support the next change.#ifdeflogic in the shader code a bit.isTouchingColornow clears to transparent black, and only draws in the opaque background for pixels masked in by the stencil buffer (which can only be done if the background is drawn as a quad--gl.clearwill fill the entire buffer). The loop in_isTouchingColorGpuFinthat checks for a pixel match will no longer check transparent pixels. This is the correct behavior, as only masked-in pixels should be checked._candidatesTouching, used for "touching sprite" queries, expands the bounding boxes of all candidate drawables to integer coordinates, as is already done for the query drawable in_touchingBounds._candidatesBounds, and_candidatesBoundsuses the bounding boxes returned by_candidatesTouching, it's extra important that the bounds be integers-- other code expects it to be so. For instance, what do you expect this code to do ifbounds.widthandbounds.heightare floating-point values?Reason for Changes
These changes are all necessary to make "touching color" behave properly when checking if the background color is being touched.
Test Coverage
An "is touching background color" test has been added.