Fix #242, onion skin rendered not cleared if 0 frames

src/js/controller/DrawingController.js

@@ -33,7 +33,7 @@
 
     this.overlayRenderer = new pskl.rendering.frame.CachedFrameRenderer(this.container, renderingOptions, ["canvas-overlay"]);
     this.renderer = new pskl.rendering.frame.CachedFrameRenderer(this.container, renderingOptions, ["drawing-canvas"]);
-    this.onionSkinRenderer = new pskl.rendering.OnionSkinRenderer(this.container, renderingOptions, piskelController);
+    this.onionSkinRenderer = pskl.rendering.OnionSkinRenderer.createInContainer(this.container, renderingOptions, piskelController);
     this.layersRenderer = new pskl.rendering.layer.LayersRenderer(this.container, renderingOptions, piskelController);
 
     this.compositeRenderer = new pskl.rendering.CompositeRenderer();

src/js/rendering/OnionSkinRenderer.js

@@ -1,32 +1,63 @@
 (function () {
   var ns = $.namespace('pskl.rendering');
 
-  ns.OnionSkinRenderer = function (container, renderingOptions, piskelController) {
+  ns.OnionSkinRenderer = function (renderer, piskelController) {
     pskl.rendering.CompositeRenderer.call(this);
 
     this.piskelController = piskelController;
-
-    // Do not use CachedFrameRenderers here, since the caching will be performed in the render method of LayersRenderer
-    this.renderer = new pskl.rendering.frame.FrameRenderer(container, renderingOptions, ["onion-skin-canvas"]);
-
+    this.renderer = renderer;
     this.add(this.renderer);
 
-    this.serializedRendering = '';
+    this.hash = '';
+  };
+
+  ns.OnionSkinRenderer.createInContainer = function (container, renderingOptions, piskelController) {
+    // Do not use CachedFrameRenderers here, caching is performed in the render method
+    var renderer = new pskl.rendering.frame.FrameRenderer(container, renderingOptions, ['onion-skin-canvas']);
+    return new ns.OnionSkinRenderer(renderer, piskelController);
   };
 
   pskl.utils.inherit(pskl.rendering.OnionSkinRenderer, pskl.rendering.CompositeRenderer);
 
   ns.OnionSkinRenderer.prototype.render = function () {
+    var frames = this.getOnionFrames_();
+    var hash = this.computeHash_(frames);
+    if (this.hash != hash) {
+      this.hash = hash;
+      this.clear();
+      if (frames.length > 0) {
+        var mergedFrame = pskl.utils.FrameUtils.merge(frames);
+        this.renderer.render(mergedFrame);
+      }
+    }
+  };
+
+  ns.OnionSkinRenderer.prototype.getOnionFrames_ = function () {
+    var frames = [];
+    
+    var currentFrameIndex = this.piskelController.getCurrentFrameIndex();
+    var layer = this.piskelController.getCurrentLayer();
+
+    var previousIndex = currentFrameIndex - 1;
+    var previousFrame = layer.getFrameAt(previousIndex);
+    if (previousFrame) {
+      frames.push(previousFrame);
+    }
+
+    var nextIndex = currentFrameIndex + 1;
+    var nextFrame = layer.getFrameAt(nextIndex);
+    if (nextFrame) {
+      frames.push(nextFrame);
+    }
+
+    return frames;
+  };
+
+  ns.OnionSkinRenderer.prototype.computeHash_ = function (frames) {
     var offset = this.getOffset();
     var size = this.getDisplaySize();
     var layers = this.piskelController.getLayers();
-    var currentFrameIndex = this.piskelController.getCurrentFrameIndex();
-
-    var frames = [];
-    this.addFrameAtIndexToArray_(currentFrameIndex - 1, frames);
-    this.addFrameAtIndexToArray_(currentFrameIndex + 1, frames);
-
-    var serializedRendering = [
+    return [
       this.getZoom(),
       this.getGridWidth(),
       offset.x,
@@ -37,25 +68,7 @@
         return f.getHash();
       }).join('-'),
       layers.length
-    ].join("-");
-
-
-    if (this.serializedRendering != serializedRendering) {
-      this.serializedRendering = serializedRendering;
-
-      if (frames.length > 0) {
-        this.clear();
-        var mergedFrame = pskl.utils.FrameUtils.merge(frames);
-        this.renderer.render(mergedFrame);
-      }
-    }
-  };
-
-  ns.OnionSkinRenderer.prototype.addFrameAtIndexToArray_ = function (frameIndex, frames) {
-    var layer = this.piskelController.getCurrentLayer();
-    if (this.piskelController.hasFrameAt(frameIndex)) {
-      frames.push(layer.getFrameAt(frameIndex));
-    }
+    ].join('-');
   };
 
   /**
@@ -72,6 +85,6 @@
   };
 
   ns.OnionSkinRenderer.prototype.flush = function () {
-    this.serializedRendering = '';
+    this.hash = '';
   };
 })();

src/js/rendering/frame/CachedFrameRenderer.js

@@ -5,10 +5,10 @@
    * FrameRenderer implementation that prevents unnecessary redraws.
    * @param {HtmlElement} container HtmlElement to use as parentNode of the Frame
    * @param {Object} renderingOptions
-   * @param {Array} classes array of strings to use for css classes
+   * @param {Array} classList array of strings to use for css classes
    */
-  ns.CachedFrameRenderer = function (container, renderingOptions, classes) {
-    pskl.rendering.frame.FrameRenderer.call(this, container, renderingOptions, classes);
+  ns.CachedFrameRenderer = function (container, renderingOptions, classList) {
+    pskl.rendering.frame.FrameRenderer.call(this, container, renderingOptions, classList);
     this.serializedFrame = '';
   };
 

src/js/rendering/frame/FrameRenderer.js

@@ -5,9 +5,9 @@
    * FrameRenderer will display a given frame inside a canvas element.
    * @param {HtmlElement} container HtmlElement to use as parentNode of the Frame
    * @param {Object} renderingOptions
-   * @param {Array} classes array of strings to use for css classes
+   * @param {Array} classList array of strings to use for css classList
    */
-  ns.FrameRenderer = function (container, renderingOptions, classes) {
+  ns.FrameRenderer = function (container, renderingOptions, classList) {
     this.defaultRenderingOptions = {
       'supportGridRendering' : false,
       'zoom' : 1
@@ -39,8 +39,8 @@
 
     this.supportGridRendering = renderingOptions.supportGridRendering;
 
-    this.classes = classes || [];
-    this.classes.push('canvas');
+    this.classList = classList || [];
+    this.classList.push('canvas');
 
     /**
      * Off dom canvas, will be used to draw the frame at 1:1 ratio
@@ -153,7 +153,7 @@
     var height = this.displayHeight;
     var width = this.displayWidth;
 
-    this.displayCanvas = pskl.utils.CanvasUtils.createCanvas(width, height, this.classes);
+    this.displayCanvas = pskl.utils.CanvasUtils.createCanvas(width, height, this.classList);
     pskl.utils.CanvasUtils.disableImageSmoothing(this.displayCanvas);
     this.container.append(this.displayCanvas);
   };

test/js/rendering/CanvasRendererTest.js

@@ -3,25 +3,12 @@ describe("Canvas Renderer test", function() {
   var WHITE = '#ffffff';
   var TRANS = Constants.TRANSPARENT_COLOR;
 
-  var toFrameGrid = function (normalGrid) {
-    var frameGrid = [];
-    var w = normalGrid[0].length;
-    var h = normalGrid.length;
-    for (var x = 0 ; x < w ; x++) {
-      frameGrid[x] = [];
-      for (var y = 0 ; y < h ; y++) {
-        frameGrid[x][y] = normalGrid[y][x];
-      }
-    }
-    return frameGrid;
-  };
-
   beforeEach(function() {});
   afterEach(function() {});
 
   it("draws transparent as white by default", function() {
     // create frame
-    var frame = pskl.model.Frame.fromPixelGrid(toFrameGrid([
+    var frame = pskl.model.Frame.fromPixelGrid(test.testutils.toFrameGrid([
       [BLACK, TRANS],
       [TRANS, BLACK]
     ]));

test/js/rendering/OnionSkinRendererTest.js

@@ -0,0 +1,120 @@
+describe("Onion Skin Renderer test", function() {
+  var BLACK = '#000000';
+  var WHITE = '#ffffff';
+  var TRANS = Constants.TRANSPARENT_COLOR;
+
+  beforeEach(function() {});
+  afterEach(function() {});
+
+  it("renders correctly :)", function() {
+    var fakeRenderer = createMockRenderer();
+    var layer = createMockLayer();
+    var piskelController = createMockPiskelController();
+    piskelController.currentLayer_ = layer;
+
+    var onionSkinRenderer = new pskl.rendering.OnionSkinRenderer(fakeRenderer, piskelController);
+    // create frame
+    var previousFrame = pskl.model.Frame.fromPixelGrid(test.testutils.toFrameGrid([
+      [BLACK, TRANS],
+      [TRANS, TRANS]
+    ]));
+
+    var nextFrame = pskl.model.Frame.fromPixelGrid(test.testutils.toFrameGrid([
+      [TRANS, TRANS],
+      [TRANS, BLACK]
+    ]));
+
+    piskelController.currentFrameIndex_ = 1;
+    layer.frames = [previousFrame, {}, nextFrame];
+
+    // initial state, all counters at 0
+    expect(fakeRenderer.clearCounter_).toBe(0);
+    expect(fakeRenderer.renderCounter_).toBe(0);
+
+    // First rendering, should call clear + render
+    onionSkinRenderer.render();
+    expect(fakeRenderer.clearCounter_).toBe(1);
+    expect(fakeRenderer.renderCounter_).toBe(1);
+    test.testutils.frameEqualsGrid(fakeRenderer.renderedFrame_, [
+      [BLACK, TRANS],
+      [TRANS, BLACK]
+    ]);
+
+    // Second rendering, nothing changed, should not clear or render
+    onionSkinRenderer.render();
+    expect(fakeRenderer.clearCounter_).toBe(1);
+    expect(fakeRenderer.renderCounter_).toBe(1);
+
+    // remove one frame
+    layer.frames = [previousFrame, {}];
+    onionSkinRenderer.render();
+    expect(fakeRenderer.clearCounter_).toBe(2);
+    expect(fakeRenderer.renderCounter_).toBe(2);
+    test.testutils.frameEqualsGrid(fakeRenderer.renderedFrame_, [
+      [BLACK, TRANS],
+      [TRANS, TRANS]
+    ]);
+
+    // remove the other frame
+    layer.frames = [{}];
+    piskelController.currentFrameIndex_ = 0;
+    onionSkinRenderer.render();
+    // nothing to render, but the underlying renderer should still be cleared
+    expect(fakeRenderer.clearCounter_).toBe(3);
+    expect(fakeRenderer.renderCounter_).toBe(2);
+  });
+
+  var createMockLayer = function () {
+    return {
+      frames : [],
+      getFrameAt : function (index) {
+        return this.frames[index];
+      }
+    };
+  };
+
+  var createMockPiskelController = function () {
+    return {
+      currentFrameIndex_ : 1,
+      currentLayer_ : null,
+      getLayers : function () {
+        return [this.currentLayer_];
+      },
+
+      getCurrentFrameIndex : function () {
+        return this.currentFrameIndex_;
+      },
+
+      getCurrentLayer : function () {
+        return this.currentLayer_;
+      }
+    };
+  };
+
+  var createMockRenderer = function () {
+    return {
+      clearCounter_ : 0,
+      clear : function () {
+        this.clearCounter_++;
+      },
+      renderedFrame_ : null,
+      renderCounter_ : 0,
+      render : function (frame) {
+        this.renderCounter_++;
+        this.renderedFrame_ = frame;
+      },
+      getZoom : function () {
+        return 1;
+      },
+      getGridWidth : function () {
+        return 0;
+      },
+      getOffset : function () {
+        return {x:0,y:0};
+      },
+      getDisplaySize : function () {
+        return {width:10,height:10};
+      }
+    };
+  };
+});

test/js/testutils/TestUtils.js

@@ -0,0 +1,22 @@
+(function () {
+  var ns = $.namespace('test.testutils');
+
+  ns.toFrameGrid = function (normalGrid) {
+    var frameGrid = [];
+    var w = normalGrid[0].length;
+    var h = normalGrid.length;
+    for (var x = 0 ; x < w ; x++) {
+      frameGrid[x] = [];
+      for (var y = 0 ; y < h ; y++) {
+        frameGrid[x][y] = normalGrid[y][x];
+      }
+    }
+    return frameGrid;
+  };
+
+  ns.frameEqualsGrid = function (frame, grid) {
+    frame.forEachPixel(function (color, col, row) {
+      expect(color).toBe(grid[row][col]);
+    });
+  };
+})();

test/js/utils/FrameTransformTest.js

@@ -9,29 +9,9 @@ describe("FrameTransform suite", function() {
   var CLOCKWISE = pskl.utils.FrameTransform.CLOCKWISE;
   var COUNTERCLOCKWISE = pskl.utils.FrameTransform.COUNTERCLOCKWISE;
 
-  /**
-   * Check a frame has the pixels as a given grid
-   * @param  {Frame} frame [description]
-   * @param  {Array<Array<String|Color>>} grid
-   */
-  var frameEqualsGrid = function (frame, grid) {
-    frame.forEachPixel(function (color, col, row) {
-      expect(color).toBe(grid[row][col]);
-    });
-  };
-
-  var toFrameGrid = function (normalGrid) {
-    var frameGrid = [];
-    var w = normalGrid[0].length;
-    var h = normalGrid.length;
-    for (var x = 0 ; x < w ; x++) {
-      frameGrid[x] = [];
-      for (var y = 0 ; y < h ; y++) {
-        frameGrid[x][y] = normalGrid[y][x];
-      }
-    }
-    return frameGrid;
-  };
+  // shortcuts
+  var frameEqualsGrid = test.testutils.frameEqualsGrid;
+  var toFrameGrid = test.testutils.toFrameGrid;
 
   /*******************************/
   /************ FLIP *************/