Adjust CPU isTouchingColor to match GPU results #407
Conversation
For some reason the JavaScript engine insists on running the code instead of doing what the comment says. I guess they should match.
| Math.floor(vec[1] * (this._height - 1)), | ||
| dst | ||
| ); | ||
| const x = Math.round(vec[0] * this._width); |
There was a problem hiding this comment.
I went back and forth a long time on if -1 was needed here or not -- do you have some ninja unit/integration testing that proves it isn't?
There was a problem hiding this comment.
I was using #406 extensively while working on this, and skipping the -1 here is one of the main changes that makes the results exactly the same as the GPU version. In fact, this work is why I did #406 in the first place :)
I also drew it out on paper to convince myself that it's correct: take a look at which texel you "want" to sample in a 4-texel-wide image when your texture coordinate is 0.01, 0.24, 0.26, 0.49, 0.51, 0.74, 0.76, and 0.99.
src/RenderWebGL.js
Outdated
| ) { | ||
| return true; | ||
| for (let x = 0; x < bounds.width; ++x) { | ||
| point[1] = bounds.top - y; // bounds.top <= y < bounds.bottom ("flipped") |
There was a problem hiding this comment.
I really don't like the fact that this ends up with y !== point[1] -- can we maybe rename the outer loop var to yFromTop or something
There was a problem hiding this comment.
Hmm, I kind of agree but on the other hand if you combine this with #406 then you end up with debugCanvasContext.fillRect(x, y, 1, 1) being the correct way to draw to the debug canvas to match the GPU path's results. I'm not saying that clean debug code should be the priority (ha!) but rather I feel like that's a sign that these are the "natural" values for x and y. (Side note: it's very satisfying to see the CPU path early-out when a matching color is found, leaving the rest of the canvas blank!)
Consider also that 0 <= y < bounds.height and 0 <= x < bounds.width are both correct here, as is bounds.left <= point[0] < bounds.right, but bounds.bottom <= point[1] < bounds.top is wrong.
Actually, that makes me realize that this comment refers to the wrong variable. I'll fix that :/
Resolves
Fixes some causes of #214
Proposed Changes
Functionality changes:
isTouchingColorto avoid testing extra edge pixelscolorAtNearestto match hardware texture sampling resultssampleColor4bno longer contains an early-out bounds test sincecolorAtNearestdoes 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:
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.Test Coverage
See #406