Address review feedback

Author: adroitwhiz

src/PenSkin.js

@@ -25,6 +25,12 @@ const DefaultPenAttributes = {
     diameter: 1
 };
 
+/**
+ * Reused memory location for storing a premultiplied pen color.
+ * @type {FloatArray}
+ */
+const __premultipliedColor = [0, 0, 0, 0];
+
 
 /**
  * Reused memory location for projection matrices.
@@ -79,9 +85,6 @@ class PenSkin extends Skin {
     constructor (id, renderer) {
         super(id);
 
-        // This silhouette will be updated with data from `gl.readPixels`, which is premultiplied.
-        this._silhouette.premultiplied = true;
-
         /**
          * @private
          * @type {RenderWebGL}
@@ -154,13 +157,6 @@ class PenSkin extends Skin {
         return true;
     }
 
-    /**
-     * @returns {boolean} true if alpha is premultiplied, false otherwise
-     */
-    get hasPremultipliedAlpha () {
-        return true;
-    }
-
     /**
      * @return {Array<number>} the "native" size, in texels, of this skin. [width, height]
      */
@@ -188,7 +184,7 @@ class PenSkin extends Skin {
     clear () {
         const gl = this._renderer.gl;
         twgl.bindFramebufferInfo(gl, this._framebuffer);
-        
+
         /* Reset framebuffer to transparent black */
         gl.clearColor(0, 0, 0, 0);
         gl.clear(gl.COLOR_BUFFER_BIT);
@@ -375,6 +371,13 @@ class PenSkin extends Skin {
         const radius = diameter / 2;
         const yScalar = (0.50001 - (radius / (length + diameter)));
 
+        // Premultiply pen color by pen transparency
+        const penColor = penAttributes.color4f || DefaultPenAttributes.color4f;
+        __premultipliedColor[0] = penColor[0] * penColor[3];
+        __premultipliedColor[1] = penColor[1] * penColor[3];
+        __premultipliedColor[2] = penColor[2] * penColor[3];
+        __premultipliedColor[3] = penColor[3];
+
         const uniforms = {
             u_positionScalar: yScalar,
             u_capScale: diameter,
@@ -388,7 +391,7 @@ class PenSkin extends Skin {
                 twgl.m4.scaling(scalingVector, __modelScalingMatrix),
                 __modelMatrix
             ),
-            u_lineColor: penAttributes.color4f || DefaultPenAttributes.color4f
+            u_lineColor: __premultipliedColor
         };
 
         twgl.setUniforms(currentShader, uniforms);
@@ -648,7 +651,7 @@ class PenSkin extends Skin {
             skinImageData.data.set(skinPixels);
             skinContext.putImageData(skinImageData, 0, 0);
 
-            this._silhouette.update(this._canvas);
+            this._silhouette.update(this._canvas, true /* isPremultiplied */);
 
             this._silhouetteDirty = false;
         }

src/RenderWebGL.js

@@ -1895,7 +1895,7 @@ class RenderWebGL extends EventEmitter {
             }
             */
             Drawable.sampleColor4b(vec, drawables[index].drawable, __blendColor);
-            // if we are fully transparent, go to the next one "down"
+            // Equivalent to gl.blendFunc(gl.ONE, gl.ONE_MINUS_SRC_ALPHA)
             dst[0] += __blendColor[0] * blendAlpha;
             dst[1] += __blendColor[1] * blendAlpha;
             dst[2] += __blendColor[2] * blendAlpha;

src/Silhouette.js

@@ -20,7 +20,7 @@ let __SilhouetteUpdateCanvas;
  * @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;
     }
@@ -47,7 +47,7 @@ const __cornerWork = [
  * @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);
     }
@@ -71,7 +71,7 @@ const getColor4b = ({_width: width, _height: height, _colorData: data}, x, y, ds
  * @return {Uint8ClampedArray} The dst vector.
  */
 const getPremultipliedColor4b = ({_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);
     }
@@ -103,13 +103,6 @@ class Silhouette {
          */
         this._colorData = null;
 
-        /**
-         * Whether or not the color data is premultiplied with its alpha channel.
-         * If it isn't, it will be multiplied here.
-         * @type {boolean}
-         */
-        this._isPremultiplied = false;
-
         // By default, silhouettes are assumed not to contain premultiplied image data,
         // so when we get a color, we want to multiply it by its alpha channel.
         // Point `_getColor` to the version of the function that multiplies.
@@ -118,36 +111,13 @@ class Silhouette {
         this.colorAtNearest = this.colorAtLinear = (_, dst) => dst.fill(0);
     }
 
-    /**
-     * @returns {boolean} true if the silhouette color data is premultiplied, false if not.
-     */
-    get premultiplied () {
-        return this._isPremultiplied;
-    }
-
-    /**
-     * Set the alpha premultiplication state of this silhouette, to ensure proper color values are returned.
-     * If set to true, the silhouette will assume it is being set with premultiplied color data,
-     * and will not multiply color values by alpha.
-     * If set to false, it will multiply color values by alpha.
-     * @param {boolean} isPremultiplied Whether this silhouette will be populated with premultiplied color data.
-     */
-    set premultiplied (isPremultiplied) {
-        this._isPremultiplied = isPremultiplied;
-
-        if (isPremultiplied) {
-            this._getColor = getPremultipliedColor4b;
-        } else {
-            this._getColor = getColor4b;
-        }
-    }
-
     /**
      * Update this silhouette with the bitmapData for a skin.
-     * @param {*} bitmapData An image, canvas or other element that the skin
+     * @param {ImageData|HTMLCanvasElement|HTMLImageElement} bitmapData An image, canvas or other element that the skin
+     * @param {boolean} isPremultiplied True if the source bitmap data comes premultiplied (e.g. from readPixels).
      * rendering can be queried from.
      */
-    update (bitmapData) {
+    update (bitmapData, isPremultiplied = false) {
         let imageData;
         if (bitmapData instanceof ImageData) {
             // If handed ImageData directly, use it directly.
@@ -170,6 +140,12 @@ class Silhouette {
             imageData = ctx.getImageData(0, 0, width, height);
         }
 
+        if (isPremultiplied) {
+            this._getColor = getPremultipliedColor4b;
+        } else {
+            this._getColor = getColor4b;
+        }
+
         this._colorData = imageData.data;
         // delete our custom overriden "uninitalized" color functions
         // let the prototype work for itself

src/Skin.js

@@ -79,13 +79,6 @@ class Skin extends EventEmitter {
         return false;
     }
 
-    /**
-     * @returns {boolean} true if alpha is premultiplied, false otherwise
-     */
-    get hasPremultipliedAlpha () {
-        return false;
-    }
-
     /**
      * @return {int} the unique ID for this Skin.
      */

src/shaders/sprite.frag

@@ -43,14 +43,16 @@ uniform sampler2D u_skin;
 
 varying vec2 v_texCoord;
 
+// Add this to divisors to prevent division by 0, which results in NaNs propagating through calculations.
+// Smaller values can cause problems on some mobile devices.
+const float epsilon = 1e-3;
+
 #if !defined(DRAW_MODE_silhouette) && (defined(ENABLE_color))
 // Branchless color conversions based on code from:
 // http://www.chilliant.com/rgb2hsv.html by Ian Taylor
 // Based in part on work by Sam Hocevar and Emil Persson
 // See also: https://en.wikipedia.org/wiki/HSL_and_HSV#Formal_derivation
 
-// Smaller values can cause problems on some mobile devices
-const float epsilon = 1e-3;
 
 // Convert an RGB color to Hue, Saturation, and Value.
 // All components of input and output are expected to be in the [0,1] range.
@@ -156,7 +158,7 @@ void main()
 	#if defined(ENABLE_color) || defined(ENABLE_brightness)
 	// Divide premultiplied alpha values for proper color processing
 	// Add epsilon to avoid dividing by 0 for fully transparent pixels
-	gl_FragColor.rgb /= gl_FragColor.a + epsilon;
+	gl_FragColor.rgb = clamp(gl_FragColor.rgb / (gl_FragColor.a + epsilon), 0.0, 1.0);
 
 	#ifdef ENABLE_color
 	{
@@ -209,9 +211,7 @@ void main()
 	#endif // DRAW_MODE_silhouette
 
 	#else // DRAW_MODE_lineSample
-	// Pen skins use premultiplied alpha, but u_lineColor is not premultiplied, so multiply it here
-	vec4 premulColor = vec4(u_lineColor.rgb * u_lineColor.a, u_lineColor.a);
-	gl_FragColor = premulColor * clamp(
+	gl_FragColor = u_lineColor * clamp(
 		// Scale the capScale a little to have an aliased region.
 		(u_capScale + u_aliasAmount -
 			u_capScale * 2.0 * distance(v_texCoord, vec2(0.5, 0.5))