Skip to content
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 texture sampling calculations for CPU pipeline #479

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

adroitwhiz
Copy link
Contributor

@adroitwhiz adroitwhiz commented Jul 9, 2019

Resolves

Resolves scratchfoundation/scratch-vm#2076, plus some of #214
Resolves scratchfoundation/scratch-vm#2077

EDIT: may fix scratchfoundation/scratch-gui#2860

Proposed Changes

Well, this is a bit of a weird one. The first change (snapping candidate bounds to ints) fixed the test project given in scratchfoundation/scratch-vm#2076, but when I created a test project, it still failed! The second change (changing silhouette sampling math) fixed the test project I added to the suite. While I'd prefer to separate it out, both changes are necessary to fully resolve the issue.

EDIT 2: I put some more work into this, and now, the CPU renderer is pixel-perfect, barring rounding differences and antialiasing.

This PR:

  1. Changes _candidatesTouching, used for "touching sprite" queries, to expand the bounding boxes of all candidate drawables to integer coordinates, as is already done for the query drawable in _touchingBounds.
  2. Changes Silhouette "color/point at position" functions to scale the point position back up by the silhouette's dimensions, instead of its dimensions - 1. This is because texture space goes from [0, 1] inclusive, not [0, 1) exclusive.
  3. Offsets Drawable.getLocalPosition's input Scratch-space vector by 0.5, and texture-space vectors in Silhouette by -0.5, because OpenGL samples pixel centers at half-integer coordinates.
  4. Changes Silhouette.getPoint and Silhouette.getColor to clamp their coordinates, matching the behavior of the GL_CLAMP_TO_EDGE behavior used in the GL renderer.
  5. Moves the point-in-bounds check from Drawable.getLocalPosition to Silhouette.isTouching[Nearest/Linear] to support the "clamp to edge" sampling.
  6. Updates the "cpu-render.html" test page to match the "index.html" test page, so that it will actually load projects. done in Attach V2 adapters in CPU render playground #484
  7. Adds a black border around the stages in the "cpu-render.html" test page to make it easier to line the stages up in image editing software, especially if the backdrop is white
  8. Re-enables linear silhouette sampling now that the math is correct. This would be better separated into another PR.

EDIT 2019-02-07:

  1. Adds back in the alpha test in sprite.frag, but only in the silhouette draw mode. This was done by me some time in the past, but I can't remember when and the commits were squashed. This was apparently done in Use premultiplied alpha everywhere, take 2 #515.
  2. Includes some commits from Adjust CPU isTouchingColor to match GPU results (again) #419:
  • Adjust the loops in isTouchingColor to avoid testing extra edge pixels
  • Iterate drawables in the same order on CPU & GPU
  • Fix exception on first button click
  • Add touching-color test to verify stencil use
  • Run test projects in each GPU usage mode
  • Make touching-color test more robust against GPU imprecision
  1. Doesn't apply the ghost effect to isTouchingColor's drawable on the CPU path. This was already excluded in the GPU path, which made the existing color-touching-tests.sb2 test pass, but running tests on both the CPU and GPU reveals the issue.
  2. Doesn't include text bubbles in "touching color" tests. An important note on compatibility: doesnt-touch-say-bubble is flaky, and should never have passed. As far as I can tell, ever since the release of Scratch 3.0, if a sprite has been touching a text bubble, it will count as touching white in most cases. As such, you may want to consider marking "say" bubbles' touchability as a known incompatibility between 2.0 and 3.0.

Reason for Changes

Regarding (1), if a Drawable's bounds lie at a half-pixel, that pixel will still render as partially opaque and should be considered "inside" the drawable. If not, you could end up with e.g. subtle subpixel differences between a sprite's "fast" bounds and its convex hull bounds which result in inconsistent collision detection, as in scratchfoundation/scratch-vm#2076.

Regarding the rest, these changes make "touching" tests more closely match Scratch 2.0 [EDIT: and also correct!], and make "touching color" tests match the GPU better [EDIT: and also correct!]. See, for example, this project (rename to .sb2).

Test Coverage

I have added a test for the issue described in scratchfoundation/scratch-vm#2076.

@adroitwhiz adroitwhiz changed the title Push out touching-sprite candidate bounds to integer coords Push out touching-sprite candidate bounds to integer coords, plus fix silhouette math Jul 10, 2019
@thisandagain thisandagain added this to the August 2019 milestone Jul 15, 2019
@thisandagain
Copy link
Contributor

/cc @cwillisf @kchadha @BryceLTaylor

@adroitwhiz
Copy link
Contributor Author

@gnarf since you wrote the CPU renderer code, you may find this of interest.

@adroitwhiz
Copy link
Contributor Author

@fsih b3bcbe4 fixes an issue introduced with #515--it was revealed by running tests in both CPU and GPU mode.

I haven't seen any projects "in the wild" that are broken, but if you want me to git cherry-pick that commit into a new PR, I'd be happy to do so.

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.

This looks good pending compatibility bug hunt, thanks for digging through all this!

@fsih fsih added the needs-qa label Jul 6, 2020
@fsih
Copy link
Contributor

fsih commented Jul 9, 2020

FYI I tried reproing #432 using https://scratch.mit.edu/projects/301945007/editor on various browsers (but not a surface) and didn't get repro with this patched

@fsih
Copy link
Contributor

fsih commented Jul 9, 2020

@cwillisf saw the issue where the cat does not touch the bread on a lower pixel density device, but it was also on prod. We think that the pr that was rolled back was not the cause of this issue, rather it's that the higher density mip map is overwriting the silhouette on higher density devices, and the lower density silhouette doesn't touch the bread. New issue filed: #656

@BryceLTaylor BryceLTaylor modified the milestones: September 2019, Overdue Jul 13, 2020
@adroitwhiz adroitwhiz changed the title Push out touching-sprite candidate bounds to integer coords, plus fix silhouette math Fix texture sampling calculations for CPU pipeline Jul 14, 2020
@adroitwhiz
Copy link
Contributor Author

adroitwhiz commented Mar 15, 2021

@fsih @cwillisf Any chance of this PR going in? Not sure if you still need to figure out QA...

There are other "touching" fixes I want to make that would be kinda pointless without this one going in first.

@adroitwhiz
Copy link
Contributor Author

Rebased this on the latest develop branch; the only merge conflict I had to resolve was the _makeOrthoProjection change in extractDrawable, which I moved to extractDrawableScreenSpace.

@fsih
Copy link
Contributor

fsih commented Jul 27, 2021

Hey @adroitwhiz, really sorry for the lag on this ): unfortunately we don't have the bandwidth right now to put in these changes, so they likely won't be going in soon.

@adroitwhiz
Copy link
Contributor Author

adroitwhiz commented Sep 15, 2023

@cwillisf Is there any path to getting this change put in at any point and mitigating any potential project breakage?

Over the years I've had various thoughts about writing my own Scratch runtime or something similar, and every time, I end up going "ugh, but I'll have to replicate 3.0's busted sprite collision code..."

If Scratch itself ever decides to rewrite the renderer (perhaps when rewriting the rest of the editor?), it might have to replicate these bugs too.

I feel like the longer this PR stays in limbo, the more projects will be created that rely on the broken behavior (and it is broken in many, many ways. The "touching" results don't visually match the sprite, the hitbox changes when you click the sprite and its bounds become tighter, and the magnitude of the error grows the smaller the sprite is).

Most projects should behave similarly if these fixes go in, but I can think of a couple cases where there may be problems:

  • Hairline collisions where a sprite is moving parallel to another sprite's hitbox rather than moving towards or away from it. I don't think (m)any projects do this, but I could be wrong.
  • Fencing bugs--some projects may depend on sprites being able to go a certain distance off-screen. This PR should shrink hitboxes for the most part, allowing sprites to go further off-screen, but there may be some cases where it doesn't?
    I forgot that fencing uses sprites' axis-aligned bounds, which aren't affected by this change. "if on edge, bounce" does use convex hull bounds, and its behavior would be slightly affected.

I understand that testing this PR across various projects to see the breakage requires a lot of QA work, but not putting this change in also has a cost: you're essentially committing to replicating the broken behavior in all future versions of Scratch.

Christopher Willis-Ford and others added 10 commits March 3, 2024 14:29
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.
Fix direction for Y iteration on CPU path

For some reason the JavaScript engine insists on running the code
instead of doing what the comment says. I guess they should match.

Fix (x,y) => point[] conversion comments
This should ensure that CPU/GPU diffs are actually useful,
and not improperly shifted.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants