Merge pull request #419 from cwillisf/coordinates-fixups-2

Adjust CPU `isTouchingColor` to match GPU results (again)

Author: cwillisf

src/Drawable.js

@@ -24,12 +24,11 @@ const __isTouchingPosition = twgl.v3.create();
  * @return {twgl.v3} [x,y] texture space float vector - transformed by effects and matrix
  */
 const getLocalPosition = (drawable, vec) => {
-    // Transfrom from world coordinates to Drawable coordinates.
+    // Transform from world coordinates to Drawable coordinates.
     const localPosition = __isTouchingPosition;
     const v0 = vec[0];
     const v1 = vec[1];
     const m = drawable._inverseMatrix;
-    // var v2 = v[2];
     const d = (v0 * m[3]) + (v1 * m[7]) + m[15];
     // The RenderWebGL quad flips the texture's X axis. So rendered bottom
     // left is 1, 0 and the top right is 0, 1. Flip the X axis so
@@ -342,7 +341,7 @@ class Drawable {
         // Drawable configures a 3D matrix for drawing in WebGL, but most values
         // will never be set because the inputs are on the X and Y position axis
         // and the Z rotation axis. Drawable can bring the work inside
-        // _calculateTransform and greatly reduce the ammount of math and array
+        // _calculateTransform and greatly reduce the amount of math and array
         // assignments needed.
 
         const scale0 = this._skinScale[0];
@@ -625,11 +624,6 @@ class Drawable {
      */
     static sampleColor4b (vec, drawable, dst) {
         const localPosition = getLocalPosition(drawable, vec);
-        if (localPosition[0] < 0 || localPosition[1] < 0 ||
-            localPosition[0] > 1 || localPosition[1] > 1) {
-            dst[3] = 0;
-            return dst;
-        }
         const textColor =
         // commenting out to only use nearest for now
         // drawable.useNearest ?

src/RenderWebGL.js

@@ -267,8 +267,7 @@ class RenderWebGL extends EventEmitter {
         this._yBottom = yBottom;
         this._yTop = yTop;
 
-        // swap yBottom & yTop to fit Scratch convention of +y=up
-        this._projection = twgl.m4.ortho(xLeft, xRight, yBottom, yTop, -1, 1);
+        this._projection = this._makeOrthoProjection(xLeft, xRight, yBottom, yTop);
 
         this._setNativeSize(Math.abs(xRight - xLeft), Math.abs(yBottom - yTop));
     }
@@ -292,6 +291,20 @@ class RenderWebGL extends EventEmitter {
         this.emit(RenderConstants.Events.NativeSizeChanged, {newSize: this._nativeSize});
     }
 
+    /**
+     * Build a projection matrix for Scratch coordinates. For example, `_makeOrthoProjection(-240,240,-180,180)` will
+     * mean the lower-left pixel is at (-240,-179) and the upper right pixel is at (239,180), matching Scratch 2.0.
+     * @param {number} xLeft - the left edge of the projection volume (-240)
+     * @param {number} xRight - the right edge of the projection volume (240)
+     * @param {number} yBottom - the bottom edge of the projection volume (-180)
+     * @param {number} yTop - the top edge of the projection volume (180)
+     * @returns {module:twgl/m4.Mat4} - a projection matrix containing [xLeft,xRight) and (yBottom,yTop]
+     */
+    _makeOrthoProjection (xLeft, xRight, yBottom, yTop) {
+        // swap yBottom & yTop to fit Scratch convention of +y=up
+        return twgl.m4.ortho(xLeft, xRight, yBottom, yTop, -1, 1);
+    }
+
     /**
      * Create a new bitmap skin from a snapshot of the provided bitmap data.
      * @param {ImageData|HTMLImageElement|HTMLCanvasElement|HTMLVideoElement} bitmapData - new contents for this skin.
@@ -529,7 +542,7 @@ class RenderWebGL extends EventEmitter {
      * Returns the position of the given drawableID in the draw list. This is
      * the absolute position irrespective of layer group.
      * @param {number} drawableID The drawable ID to find.
-     * @return {number} The postion of the given drawable ID.
+     * @return {number} The position of the given drawable ID.
      */
     getDrawableOrder (drawableID) {
         return this._drawList.indexOf(drawableID);
@@ -543,7 +556,7 @@ class RenderWebGL extends EventEmitter {
      * "go to back": setDrawableOrder(id, 1); (assuming stage at 0).
      * "go to front": setDrawableOrder(id, Infinity);
      * @param {int} drawableID ID of Drawable to reorder.
-     * @param {number} order New absolute order or relative order adjusment.
+     * @param {number} order New absolute order or relative order adjustment.
      * @param {string=} group Name of layer group drawable belongs to.
      * Reordering will not take place if drawable cannot be found within the bounds
      * of the layer group.
@@ -714,7 +727,7 @@ class RenderWebGL extends EventEmitter {
 
     /**
      * Check if a particular Drawable is touching a particular color.
-     * Unlike touching drawable, if the "tester" is invisble, we will still test.
+     * Unlike touching drawable, if the "tester" is invisible, we will still test.
      * @param {int} drawableID The ID of the Drawable to check.
      * @param {Array<int>} color3b Test if the Drawable is touching this color.
      * @param {Array<int>} [mask3b] Optionally mask the check to this part of Drawable.
@@ -738,7 +751,7 @@ class RenderWebGL extends EventEmitter {
 
         // if there are just too many pixels to CPU render efficiently, we need to let readPixels happen
         if (bounds.width * bounds.height * (candidates.length + 1) >= maxPixelsForCPU) {
-            this._isTouchingColorGpuStart(drawableID, candidates.map(({id}) => id).reverse(), bounds, color3b, mask3b);
+            this._isTouchingColorGpuStart(drawableID, candidates.map(({id}) => id), bounds, color3b, mask3b);
         }
 
         const drawable = this._allDrawables[drawableID];
@@ -747,24 +760,24 @@ class RenderWebGL extends EventEmitter {
         const hasMask = Boolean(mask3b);
 
         // Scratch Space - +y is top
-        for (let y = bounds.bottom; y <= bounds.top; y++) {
-            if (bounds.width * (y - bounds.bottom) * (candidates.length + 1) >= maxPixelsForCPU) {
-                return this._isTouchingColorGpuFin(bounds, color3b, y - bounds.bottom);
+        for (let y = 0; y < bounds.height; ++y) {
+            if (bounds.width * y * (candidates.length + 1) >= maxPixelsForCPU) {
+                return this._isTouchingColorGpuFin(bounds, color3b, y);
             }
-            for (let x = bounds.left; x <= bounds.right; x++) {
-                point[1] = y;
-                point[0] = x;
+            for (let x = 0; x < bounds.width; ++x) {
+                point[0] = bounds.left + x; // bounds.left <= point[0] < bounds.right
+                point[1] = bounds.top - y; // bounds.bottom < point[1] <= bounds.top ("flipped")
                 // if we use a mask, check our sample color...
                 if (hasMask ?
                     maskMatches(Drawable.sampleColor4b(point, drawable, color), mask3b) :
                     drawable.isTouching(point)) {
                     RenderWebGL.sampleColor3b(point, candidates, color);
                     if (debugCanvasContext) {
                         debugCanvasContext.fillStyle = `rgb(${color[0]},${color[1]},${color[2]})`;
-                        debugCanvasContext.fillRect(x - bounds.left, bounds.bottom - y, 1, 1);
+                        debugCanvasContext.fillRect(x, y, 1, 1);
                     }
                     // ...and the target color is drawn at this pixel
-                    if (colorMatches(color, color3b, 0)) {
+                    if (colorMatches(color3b, color, 0)) {
                         return true;
                     }
                 }
@@ -794,7 +807,7 @@ class RenderWebGL extends EventEmitter {
         // Limit size of viewport to the bounds around the target Drawable,
         // and create the projection matrix for the draw.
         gl.viewport(0, 0, bounds.width, bounds.height);
-        const projection = twgl.m4.ortho(bounds.left, bounds.right, bounds.top, bounds.bottom, -1, 1);
+        const projection = this._makeOrthoProjection(bounds.left, bounds.right, bounds.top, bounds.bottom);
 
         let fillBackgroundColor = this._backgroundColor;
 
@@ -877,7 +890,7 @@ class RenderWebGL extends EventEmitter {
         const candidates = this._candidatesTouching(drawableID,
             // even if passed an invisible drawable, we will NEVER touch it!
             candidateIDs.filter(id => this._allDrawables[id]._visible));
-        // if we are invisble we don't touch anything.
+        // if we are invisible we don't touch anything.
         if (candidates.length === 0 || !this._allDrawables[drawableID]._visible) {
             return false;
         }
@@ -910,7 +923,7 @@ class RenderWebGL extends EventEmitter {
 
     /**
      * Convert a client based x/y position on the canvas to a Scratch 3 world space
-     * Rectangle.  This creates recangles with a radius to cover selecting multiple
+     * Rectangle. This creates rectangles with a radius to cover selecting multiple
      * scratch pixels with touch / small render areas.
      *
      * @param {int} centerX The client x coordinate of the picking location.
@@ -1027,7 +1040,7 @@ class RenderWebGL extends EventEmitter {
             for (worldPos[0] = bounds.left; worldPos[0] <= bounds.right; worldPos[0]++) {
 
                 // Check candidates in the reverse order they would have been
-                // drawn. This will determine what candiate's silhouette pixel
+                // drawn. This will determine what candidate's silhouette pixel
                 // would have been drawn at the point.
                 for (let d = candidateIDs.length - 1; d >= 0; d--) {
                     const id = candidateIDs[d];
@@ -1111,7 +1124,7 @@ class RenderWebGL extends EventEmitter {
         // Limit size of viewport to the bounds around the target Drawable,
         // and create the projection matrix for the draw.
         gl.viewport(0, 0, bounds.width, bounds.height);
-        const projection = twgl.m4.ortho(bounds.left, bounds.right, bounds.top, bounds.bottom, -1, 1);
+        const projection = this._makeOrthoProjection(bounds.left, bounds.right, bounds.top, bounds.bottom);
 
         gl.clearColor(0, 0, 0, 0);
         gl.clear(gl.COLOR_BUFFER_BIT);
@@ -1186,7 +1199,7 @@ class RenderWebGL extends EventEmitter {
         const pickY = bounds.top - scratchY;
 
         gl.viewport(0, 0, bounds.width, bounds.height);
-        const projection = twgl.m4.ortho(bounds.left, bounds.right, bounds.top, bounds.bottom, -1, 1);
+        const projection = this._makeOrthoProjection(bounds.left, bounds.right, bounds.top, bounds.bottom);
 
         gl.clearColor.apply(gl, this._backgroundColor);
         gl.clear(gl.COLOR_BUFFER_BIT);
@@ -1255,8 +1268,7 @@ class RenderWebGL extends EventEmitter {
     }
 
     /**
-     * Filter a list of candidates for a touching query into only those that
-     * could possibly intersect the given bounds.
+     * Filter a list of candidates for a touching query into only those that could possibly intersect the given bounds.
      * @param {int} drawableID - ID for drawable of query.
      * @param {Array<int>} candidateIDs - Candidates for touching query.
      * @return {?Array< {id, drawable, intersection} >} Filtered candidates with useful data.
@@ -1267,8 +1279,7 @@ class RenderWebGL extends EventEmitter {
         if (bounds === null) {
             return result;
         }
-        // iterate through the drawables list BACKWARDS - we want the top most item to be the first we check
-        for (let index = candidateIDs.length - 1; index >= 0; index--) {
+        for (let index = 0; index < candidateIDs.length; ++index) {
             const id = candidateIDs[index];
             if (id !== drawableID) {
                 const drawable = this._allDrawables[id];
@@ -1428,7 +1439,7 @@ class RenderWebGL extends EventEmitter {
 
         // Limit size of viewport to the bounds around the stamp Drawable and create the projection matrix for the draw.
         gl.viewport(0, 0, bounds.width, bounds.height);
-        const projection = twgl.m4.ortho(bounds.left, bounds.right, bounds.top, bounds.bottom, -1, 1);
+        const projection = this._makeOrthoProjection(bounds.left, bounds.right, bounds.top, bounds.bottom);
 
         gl.clearColor(0, 0, 0, 0);
         gl.clear(gl.COLOR_BUFFER_BIT);
@@ -1517,7 +1528,7 @@ class RenderWebGL extends EventEmitter {
      * can skip superfluous extra state calls when it is already in that
      * region. Since one region may be entered from within another a exit
      * handle can also be registered that is called when a new region is about
-     * to be entered to restore a common inbetween state.
+     * to be entered to restore a common in-between state.
      *
      * @param {any} regionId - id of the region to enter
      * @param {function} enter - handle to call when first entering a region
@@ -1649,7 +1660,7 @@ class RenderWebGL extends EventEmitter {
          *
          * The determinant is useful in this case to know if AC is counter
          * clockwise from AB. A positive value means the AC is counter
-         * clockwise from AC. A negative value menas AC is clockwise from AB.
+         * clockwise from AC. A negative value means AC is clockwise from AB.
          *
          * @param {Float32Array} A A 2d vector in space.
          * @param {Float32Array} B A 2d vector in space.
@@ -1754,16 +1765,15 @@ class RenderWebGL extends EventEmitter {
      * Sample a "final" color from an array of drawables at a given scratch space.
      * Will blend any alpha values with the drawables "below" it.
      * @param {twgl.v3} vec Scratch Vector Space to sample
-     * @param {Array<Drawables>} drawables A list of drawables with the "top most"
-     *              drawable at index 0
+     * @param {Array<Drawables>} drawables A list of drawables with the "bottom most" drawable at index 0
      * @param {Uint8ClampedArray} dst The color3b space to store the answer in.
      * @return {Uint8ClampedArray} The dst vector with everything blended down.
      */
     static sampleColor3b (vec, drawables, dst) {
         dst = dst || new Uint8ClampedArray(3);
         dst.fill(0);
         let blendAlpha = 1;
-        for (let index = 0; blendAlpha !== 0 && index < drawables.length; index++) {
+        for (let index = drawables.length - 1; blendAlpha !== 0 && index >= 0; --index) {
             /*
             if (left > vec[0] || right < vec[0] ||
                 bottom > vec[1] || top < vec[0]) {

src/Silhouette.js

@@ -11,16 +11,16 @@
 let __SilhouetteUpdateCanvas;
 
 /**
- * Internal helper function (in hopes that compiler can inline).  Get a pixel
- * from silhouette data, or 0 if outside it's bounds.
+ * Internal helper function (in hopes that compiler can inline). Get the alpha value for a texel in the silhouette
+ * data, or 0 if outside it's bounds.
  * @private
- * @param {Silhouette} silhouette - has data width and height
- * @param {number} x - x
- * @param {number} y - y
+ * @param {Silhouette} $0 - has data, width, and height
+ * @param {number} x - X position in texels (0..width).
+ * @param {number} y - Y position in texels (0..height).
  * @return {number} Alpha value for x/y position
  */
 const getPoint = ({_width: width, _height: height, _colorData: data}, x, y) => {
-    // 0 if outside bouds, otherwise read from data.
+    // 0 if outside bounds, otherwise read from data.
     if (x >= width || y >= height || x < 0 || y < 0) {
         return 0;
     }
@@ -39,14 +39,14 @@ const __cornerWork = [
 
 /**
  * Get the color from a given silhouette at an x/y local texture position.
- * @param {Silhouette} The silhouette to sample.
- * @param {number} x X position of texture (0-1).
- * @param {number} y Y position of texture (0-1).
- * @param {Uint8ClampedArray} dst A color 4b space.
- * @return {Uint8ClampedArray} The dst vector.
+ * @param {Silhouette} $0 - The silhouette to sample.
+ * @param {number} x - X position in texels (0..width).
+ * @param {number} y - Y position in texels (0..height).
+ * @param {Uint8ClampedArray} dst - A color 4b space.
+ * @return {Uint8ClampedArray} - The dst vector.
  */
 const getColor4b = ({_width: width, _height: height, _colorData: data}, x, y, dst) => {
-    // 0 if outside bouds, otherwise read from data.
+    // 0 if outside bounds, otherwise read from data.
     if (x >= width || y >= height || x < 0 || y < 0) {
         return dst.fill(0);
     }
@@ -110,7 +110,7 @@ class Silhouette {
         }
 
         this._colorData = imageData.data;
-        // delete our custom overriden "uninitalized" color functions
+        // delete our custom overridden "uninitialized" color functions
         // let the prototype work for itself
         delete this.colorAtNearest;
         delete this.colorAtLinear;
@@ -124,12 +124,10 @@ class Silhouette {
      * @returns {Uint8ClampedArray} dst
      */
     colorAtNearest (vec, dst) {
-        return getColor4b(
-            this,
-            Math.floor(vec[0] * (this._width - 1)),
-            Math.floor(vec[1] * (this._height - 1)),
-            dst
-        );
+        const x = Math.round(vec[0] * this._width);
+        const y = Math.round(vec[1] * this._height);
+        const color = getColor4b(this, x, y, dst);
+        return color;
     }
 
     /**

src/shaders/sprite.frag

@@ -45,6 +45,8 @@ uniform sampler2D u_skin;
 
 varying vec2 v_texCoord;
 
+const float minAlpha = 1.0 / 255.0;
+
 #if !defined(DRAW_MODE_silhouette) && (defined(ENABLE_color))
 // Branchless color conversions based on code from:
 // http://www.chilliant.com/rgb2hsv.html by Ian Taylor
@@ -155,9 +157,14 @@ void main()
 
 	gl_FragColor = texture2D(u_skin, texcoord0);
 
-    #ifdef ENABLE_ghost
-    gl_FragColor.a *= u_ghost;
-    #endif // ENABLE_ghost
+	if (gl_FragColor.a < minAlpha)
+	{
+		discard;
+	}
+
+	#ifdef ENABLE_ghost
+	gl_FragColor.a *= u_ghost;
+	#endif // ENABLE_ghost
 
 	#ifdef DRAW_MODE_silhouette
 	// switch to u_silhouetteColor only AFTER the alpha test

test/integration/index.html

@@ -29,7 +29,10 @@
         vm.attachV2BitmapAdapter(new ScratchSVGRenderer.BitmapAdapter());
 
         document.getElementById('file').addEventListener('click', e => {
-            document.body.removeChild(document.getElementById('loaded'));
+            const loaded = document.getElementById('loaded');
+            if (loaded) {
+                document.body.removeChild(loaded);
+            }
         });
 
         document.getElementById('file').addEventListener('change', e => {