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

Put back in "Merge pull request #489 from adroitwhiz/touching-white-fixes" #676

Merged
merged 10 commits into from
Aug 26, 2020

Conversation

fsih
Copy link
Contributor

@fsih fsih commented Aug 11, 2020

Reverts #674

Fixes #680

// If the color we're checking for is the background color, don't confine the check to
// candidate drawables' bounds--since the background spans the entire stage, we must check
// everything that lies inside the drawable.
bounds = this._touchingBounds(drawableID);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue is here, touchingBounds can return null: https://github.com/LLK/scratch-render/blob/954cfff02b08069a082cbedd415c1fecd9b1e4fb/src/RenderWebGL.js#L1385

Then we reference bounds.width on line 792

@fsih
Copy link
Contributor Author

fsih commented Aug 12, 2020

I am trying to figure out why row 1 and column 1 of the background silhouette are full of transparent pixels (which are getting detected as white) and it's the rabbit hole

@fsih
Copy link
Contributor Author

fsih commented Aug 12, 2020

Rabbit holes are actually not very deep or complicated? https://i.pinimg.com/originals/d9/cc/2a/d9cc2ab7fff45888ef3fef5fc5e92ed1.jpg

@fsih
Copy link
Contributor Author

fsih commented Aug 13, 2020

I think the first row and column being detected as transparent is coming from floating point calculation issues. Here in sampleColor4b the localPosition is coming out to barely < 0, so it's returning transparent.
Screen Shot 2020-08-12 at 20 39 26
Screen Shot 2020-08-13 at 00 29 05

Here's what's in the matrix.
Screen Shot 2020-08-13 at 00 31 13

Edit: I was having a lot of frustration testing using Blue Sky 2, but I realized it actually is offset from the left. It has transparent padding all around and a viewbox that starts at x < 0, y < 0

Edit: filed #688 for this

@fsih fsih force-pushed the revert-674-revert-673-revert-660-revert-489 branch from ab8907a to 72b320f Compare August 13, 2020 18:05
@fsih fsih force-pushed the revert-674-revert-673-revert-660-revert-489 branch from 72b320f to 5fa9c76 Compare August 13, 2020 18:07
@fsih
Copy link
Contributor Author

fsih commented Aug 13, 2020

"Touches the colorfully bent semi-transparent crab" is failing

@cwillisf
Copy link
Contributor

I was comparing some of this code to #479 and I think the best approach might be to first merge that PR then look at this again. I think the way that #479 does the texture clamping in the Silhouette.js get* functions is probably better than the approach I suggested here, where you're clamping in Drawable#sampleColor4b. In fact, I now feel like all the if (localPosition... code in sampleColor4b -- even the old code from before this PR -- should just be removed in favor of the stuff in Silhouette.js. There might be some subtlety that I'm missing though... hopefully the colorfully bent semi-transparent crab will know for sure.

@cwillisf
Copy link
Contributor

Hmm, another idea that might be worth trying: what happens if you just remove the clamping in Drawable#sampleColor4b without #479's changes? We won't have all the other fixes but that might be sufficient to unblock this PR...

@fsih
Copy link
Contributor Author

fsih commented Aug 13, 2020

The colorfully bent semi-transparent crab has spoken and it is Not Ok

src/Drawable.js Outdated
Comment on lines 723 to 730
if (localPosition[0] < 0 || localPosition[1] < 0 ||
localPosition[0] > 1 || localPosition[1] > 1) {
dst[0] = 0;
dst[1] = 0;
dst[2] = 0;
dst[3] = 0;
return dst;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

So... it turns out I steered you wrong when I recommended removing this. I was thinking about texture coordinates being out of range (which should be and is handled at the Silhouette layer) when I should have been thinking about geometric coordinates being out of range (which is what this if handles). I think bringing this back in will fix the crab test.

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.

Looks good! I'm glad the crab is pleased 🦀

@@ -34,7 +34,7 @@
"eslint": "^4.6.1",
"eslint-config-scratch": "^5.0.0",
"gh-pages": "^1.0.0",
"jsdoc": "^3.5.5",
"jsdoc": "^3.6.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Might want to add "resolves #680" to the PR description... at least I think that's what this is...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I broke this out into #683

@fsih fsih merged commit 5902c4a into develop Aug 26, 2020
@fsih fsih deleted the revert-674-revert-673-revert-660-revert-489 branch August 26, 2020 00:08
@fsih fsih restored the revert-674-revert-673-revert-660-revert-489 branch August 27, 2020 20:50
@fsih fsih deleted the revert-674-revert-673-revert-660-revert-489 branch August 27, 2020 21:40
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.

scratch-render tests fail locally on node 12
3 participants