Skip to content

Adjust CPU isTouchingColor to match GPU results (again) #419

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 9 commits into from
Apr 10, 2019

Conversation

cwillisf
Copy link
Contributor

@cwillisf cwillisf commented Feb 28, 2019

Resolves

Fixes some causes of #214 as well as some of the problems encountered in #407, the initial attempt at these changes.

Proposed Changes

Unfortunately there are several intertwined changes here, all of which impact the correctness of touching color.

These functionality changes from #407 are still included here:

  • Adjust the loops in isTouchingColor to avoid testing extra edge pixels
  • Adjust the math in colorAtNearest to match hardware texture sampling results
  • sampleColor4b no longer contains an early-out bounds test since colorAtNearest does the same test more accurately. For example, we were previously getting incorrect results when "zero" turned into a very small negative number due to floating point error.

Other changes included from #407:

  • Move projection matrix calculation into a separate function
    • An earlier version of this code contained some adjustments to the projection matrix. It turns out those adjustments weren't needed, but I decided to keep the centralized function anyway.
  • Spelling corrections in comments

These functionality changes are new since #407:

  • The fragment shader once again discards fully-transparent fragments. This was removed several months ago either accidentally or for the sake of performance, but it's needed to ensure that touching color checks only non-transparent parts of the sprite running the block.
  • Instead of iterating drawables backwards in _candidatesTouching then reversing that list if/when _isTouchingColorGpuStart is called, sampleColor3b iterates the list backwards since it's the only consumer that needs the list in that order.
  • Improvements to the integration tests (see "Test Coverage" section below)

This link will show you only the changes made since #407:
https://github.com/LLK/scratch-render/pull/419/files/10fc498d87d39558839e4cad7c5f303a948483dd..HEAD

Reason for Changes

The CPU version of isTouchingColor, sampleColor4b, and a few related functions were not returning the same results as their GPU equivalents. This could cause unexpected behavior, especially if a project uses both paths. Note that both the CPU and GPU results were slightly incorrect at times; I generally used Scratch 2.0 as a guide when determining what the "correct" answer is in a given situation.

Test Coverage

This PR adds a new test project, stencil-touching-circle.sb2, which specifically tests for the false-positive touching color results that show up when alpha test stenciling isn't working.

I also significantly refactored the test framework in test/integration/scratch-tests.js such that it now runs each test three times: once in "force GPU" mode, once in "force CPU" mode, and once in the default "automatic" mode. This should make it easy to catch cases where only one path exhibits incorrect behavior. As an example, in #407 only the GPU path was returning incorrect results for stencil-touching-circle.sb2, but the sprites in that project are small enough that the GPU path would never be used if we didn't force it.

Testing all three GPU usage modes is overkill for tests which don't use any of the "touching color" logic, but in my opinion the extra time is acceptable. This is mainly due to the fact that the testing framework loads each project only once before running the project 3 times in quick succession, changing the renderer's GPU usage mode each time. This does mean that our test projects need to be authored such that vm.greenFlag() truly resets all relevant state but we should be doing that anyway.

@cwillisf
Copy link
Contributor Author

cwillisf commented Feb 28, 2019

I'm not yet sure why the tests are failing on Travis CI, especially since they're passing for me locally. I'll try to figure it out tomorrow.

Note that the test is failing in "ForceGPU" mode -- most likely it was only testing the CPU path prior to the changes in this PR.

This is the output from the failing test:

  File: color-touching-tests.sb2, GPU Mode: ForceGPU
  not ok touches a color that doesn't actually exist right now
    at:
      line: 26
      column: 15
      file: test/integration/scratch-tests.js
      function: fail
    stack: |
      Object.fail (test/integration/scratch-tests.js:26:15)
      says.forEach.text (test/integration/scratch-tests.js:43:38)
      Array.forEach (<anonymous>)
      checkOneGpuMode (test/integration/scratch-tests.js:39:10)
      Test.t (test/integration/scratch-tests.js:111:58)
      testFile (test/integration/scratch-tests.js:111:9)
      <anonymous>
    source: |
      t.fail(reason);

@cwillisf cwillisf added this to the March 2019 milestone Mar 1, 2019
rschamp
rschamp previously requested changes Mar 6, 2019
Copy link
Contributor

@rschamp rschamp left a comment

Choose a reason for hiding this comment

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

Marking as needs work until the tests are figured out

Christopher Willis-Ford added 4 commits March 20, 2019 11:21
For some reason the JavaScript engine insists on running the code
instead of doing what the comment says. I guess they should match.
@cwillisf cwillisf force-pushed the coordinates-fixups-2 branch from 0768ad1 to f9428ee Compare March 20, 2019 18:21
Previously, the `color-touching-tests.sb2` test "touches a color that
doesn't actually exist right now" would use a sprite with ghost 50,
blended against another sprite, to create the color that "doesn't
actually exist" when the query sprite is skipped. Unfortunately the
blend result was near a bit-boundary and, depending on the specific
hardware used, that test could fail on the GPU. When the renderer uses
the CPU path this test works fine, though, so the existing problem went
unnoticed.

To fix the problem I changed the project to use ghost 30 instead, which
results in a color that is less near a bit boundary and is therefore
less likely to fail on specific hardware.

As an example of what was happening: the `touching color` block was
checking for `RGB(127,101,216)` with a mask of `RGB(0xF8,0xF8,0xF0)`. On
the CPU it would find `RGB(120,99,215)`, which is in range, but on some
GPUs the closest color it could find was `RGB(119,98,215)` which
mismatches on all four of the least significant bits -- one of which is
enabled in the mask.
@cwillisf
Copy link
Contributor Author

cwillisf commented Mar 21, 2019

I (finally) fixed the failing test by adjusting the test project; no changes were necessary in the rendering code. There's a detailed explanation in the commit message for b304ea8, but the short version is that the color chosen for one of the the touching-color tests was unlucky and probably has always failed on certain GPUs, but prior to the changes in this PR the test project would have always run on the CPU path and succeeded.

These numbers, especially the binary values for the red components, might help in understanding the commit message for b304ea8:
Screen Shot 2019-03-20 at 11 28 21 PM

@cwillisf cwillisf dismissed rschamp’s stale review March 29, 2019 05:18

Tests are figured out

Copy link
Contributor

@rschamp rschamp left a comment

Choose a reason for hiding this comment

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

This all looks sane to me but I can't say that I feel super 100% confident that I know what I'm talking about :) I trust the tests though.

The only thing that stood out to me is that this also is changing the shader code to get the CPU renderer in line with it — what was the reason for that?

@cwillisf
Copy link
Contributor Author

cwillisf commented Apr 3, 2019

The shader code change is this bit:

The fragment shader once again discards fully-transparent fragments. This was removed several months ago either accidentally or for the sake of performance, but it's needed to ensure that touching color checks only non-transparent parts of the sprite running the block.

It's an old bug (#212) that comes back every once in a while when somebody forgets that the fragment shader needs to output fully transparent fragments if and only if they're fully transparent due to the ghost effect.

EDIT: Oops, the explanation above is a bit backwards. The bug that this shader change fixes is sort of the inverse of #212, where fragments would be available for "touching sprite" even if they were fully transparent in the costume. Basically, without this change the GPU path can give false positives for "touching sprite" checks.

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