From f4385b2de9bf08ac3dae514bc809dba8ba98f669 Mon Sep 17 00:00:00 2001 From: Andrea Bogazzi Date: Mon, 10 Nov 2014 11:53:30 +0100 Subject: [PATCH] Fix line accuray issue, remove some duplicate code, do not force width = 1 on 0. Fix object render method to render 0-dimensions lines Check how this version could behave in general rendering. --- src/mixins/object_geometry.mixin.js | 11 +++-- src/mixins/object_interactivity.mixin.js | 23 ++++++---- src/shapes/line.class.js | 39 ++++++---------- src/shapes/object.class.js | 2 +- test/unit/group.js | 11 +++-- test/unit/line.js | 6 +-- test/unit/object.js | 58 +++++++++++++++++++----- 7 files changed, 88 insertions(+), 62 deletions(-) diff --git a/src/mixins/object_geometry.mixin.js b/src/mixins/object_geometry.mixin.js index 86022738..71a2e9a5 100644 --- a/src/mixins/object_geometry.mixin.js +++ b/src/mixins/object_geometry.mixin.js @@ -302,7 +302,7 @@ * @chainable */ setCoords: function() { - var strokeWidth = this.strokeWidth > 1 ? this.strokeWidth : 0, + var strokeWidth = this.strokeWidth, theta = degreesToRadians(this.angle), vpt = this.getViewportTransform(), f = function (p) { @@ -311,10 +311,11 @@ w = this.width, h = this.height, capped = this.strokeLineCap === 'round' || this.strokeLineCap === 'square', - vLine = this.type === 'line' && this.width === 1, - hLine = this.type === 'line' && this.height === 1, - strokeW = (capped && hLine) || this.type !== 'line', - strokeH = (capped && vLine) || this.type !== 'line'; + vLine = this.type === 'line' && this.width === 0, + hLine = this.type === 'line' && this.height === 0, + sLine = vLine || hLine, + strokeW = (capped && hLine) || !sLine, + strokeH = (capped && vLine) || !sLine; if (vLine) { w = strokeWidth; diff --git a/src/mixins/object_interactivity.mixin.js b/src/mixins/object_interactivity.mixin.js index 074716d7..94ab631b 100644 --- a/src/mixins/object_interactivity.mixin.js +++ b/src/mixins/object_interactivity.mixin.js @@ -284,12 +284,14 @@ var w = this.getWidth(), h = this.getHeight(), - strokeWidth = this.strokeWidth > 1 ? this.strokeWidth : 0, + strokeWidth = this.strokeWidth, capped = this.strokeLineCap === 'round' || this.strokeLineCap === 'square', - vLine = this.type === 'line' && this.width === 1, - hLine = this.type === 'line' && this.height === 1, - strokeW = (capped && hLine) || this.type !== 'line', - strokeH = (capped && vLine) || this.type !== 'line'; + vLine = this.type === 'line' && this.width === 0, + hLine = this.type === 'line' && this.height === 0, + sLine = vLine || hLine, + strokeW = (capped && hLine) || !sLine, + strokeH = (capped && vLine) || !sLine; + if (vLine) { w = strokeWidth / scaleX; } @@ -348,14 +350,15 @@ var size = this.cornerSize, size2 = size / 2, vpt = this.getViewportTransform(), - strokeWidth = this.strokeWidth > 1 ? this.strokeWidth : 0, + strokeWidth = this.strokeWidth, w = this.width, h = this.height, capped = this.strokeLineCap === 'round' || this.strokeLineCap === 'square', - vLine = this.type === 'line' && this.width === 1, - hLine = this.type === 'line' && this.height === 1, - strokeW = (capped && hLine) || this.type !== 'line', - strokeH = (capped && vLine) || this.type !== 'line'; + vLine = this.type === 'line' && this.width === 0, + hLine = this.type === 'line' && this.height === 0, + sLine = vLine || hLine, + strokeW = (capped && hLine) || !sLine, + strokeH = (capped && vLine) || !sLine; if (vLine) { w = strokeWidth; diff --git a/src/shapes/line.class.js b/src/shapes/line.class.js index 4901bf13..d0982731 100644 --- a/src/shapes/line.class.js +++ b/src/shapes/line.class.js @@ -85,8 +85,8 @@ _setWidthHeight: function(options) { options || (options = { }); - this.width = Math.abs(this.x2 - this.x1) || 1; - this.height = Math.abs(this.y2 - this.y1) || 1; + this.width = Math.abs(this.x2 - this.x1); + this.height = Math.abs(this.y2 - this.y1); this.left = 'left' in options ? options.left @@ -155,30 +155,21 @@ if (noTransform) { // Line coords are distances from left-top of canvas to origin of line. - // // To render line in a path-group, we need to translate them to // distances from center of path-group to center of line. var cp = this.getCenterPoint(); ctx.translate( - cp.x, - cp.y + cp.x - this.strokeWidth / 2, + cp.y - this.strokeWidth / 2 ); } if (!this.strokeDashArray || this.strokeDashArray && supportsLineDash) { - // move from center (of virtual box) to its left/top corner // we can't assume x1, y1 is top left and x2, y2 is bottom right - var xMult = this.x1 <= this.x2 ? -1 : 1, - yMult = this.y1 <= this.y2 ? -1 : 1; - - ctx.moveTo( - this.width === 1 ? 0 : (xMult * this.width / 2), - this.height === 1 ? 0 : (yMult * this.height / 2)); - - ctx.lineTo( - this.width === 1 ? 0 : (xMult * -1 * this.width / 2), - this.height === 1 ? 0 : (yMult * -1 * this.height / 2)); + var p = this.calcLinePoints(); + ctx.moveTo(p.x1, p.y1); + ctx.lineTo(p.x2, p.y2); } ctx.lineWidth = this.strokeWidth; @@ -197,14 +188,10 @@ * @param {CanvasRenderingContext2D} ctx Context to render on */ _renderDashedStroke: function(ctx) { - var - xMult = this.x1 <= this.x2 ? -1 : 1, - yMult = this.y1 <= this.y2 ? -1 : 1, - x = this.width === 1 ? 0 : xMult * this.width / 2, - y = this.height === 1 ? 0 : yMult * this.height / 2; + var p = this.calcLinePoints(); ctx.beginPath(); - fabric.util.drawDashedLine(ctx, x, y, -x, -y, this.strokeDashArray); + fabric.util.drawDashedLine(ctx, p.x1, p.y1, p.x2, p.y2, this.strokeDashArray); ctx.closePath(); }, @@ -225,10 +212,10 @@ calcLinePoints: function() { var xMult = this.x1 <= this.x2 ? -1 : 1, yMult = this.y1 <= this.y2 ? -1 : 1, - x1 = (xMult * this.width / 2), - y1 = (yMult * this.height / 2), - x2 = (xMult * -1 * this.width / 2), - y2 = (yMult * -1 * this.height / 2); + x1 = (xMult * this.width * 0.5), + y1 = (yMult * this.height * 0.5), + x2 = (xMult * this.width * -0.5), + y2 = (yMult * this.height * -0.5); return { x1: x1, diff --git a/src/shapes/object.class.js b/src/shapes/object.class.js index 81234cf0..3fd640df 100644 --- a/src/shapes/object.class.js +++ b/src/shapes/object.class.js @@ -963,7 +963,7 @@ */ render: function(ctx, noTransform) { // do not render if width/height are zeros or object is not visible - if (this.width === 0 || this.height === 0 || !this.visible) { + if ((this.width === 0 && this.height === 0) || !this.visible) { return; } diff --git a/test/unit/group.js b/test/unit/group.js index 4d689f5a..4daa022a 100644 --- a/test/unit/group.js +++ b/test/unit/group.js @@ -7,10 +7,10 @@ } function makeGroupWith2Objects() { - var rect1 = new fabric.Rect({ top: 100, left: 100, width: 30, height: 10 }), - rect2 = new fabric.Rect({ top: 120, left: 50, width: 10, height: 40 }); + var rect1 = new fabric.Rect({ top: 100, left: 100, width: 30, height: 10, strokeWidth: 0 }), + rect2 = new fabric.Rect({ top: 120, left: 50, width: 10, height: 40, strokeWidth: 0 }); - return new fabric.Group([ rect1, rect2 ]); + return new fabric.Group([ rect1, rect2 ], {strokeWidth: 0}); } function makeGroupWith4Objects() { @@ -163,7 +163,7 @@ 'height': 60, 'fill': 'rgb(0,0,0)', 'stroke': null, - 'strokeWidth': 1, + 'strokeWidth': 0, 'strokeDashArray': null, 'strokeLineCap': 'butt', 'strokeLineJoin': 'miter', @@ -201,6 +201,7 @@ test('toObject without default values', function() { 'top': 100, 'width': 80, 'height': 60, + 'strokeWidth': 0, 'objects': clone.objects }; @@ -386,7 +387,7 @@ test('toObject without default values', function() { var group = makeGroupWith2Objects(); ok(typeof group.toSVG == 'function'); - var expectedSVG = '\n\n\n\n'; + var expectedSVG = '\n\n\n\n'; equal(group.toSVG(), expectedSVG); }); diff --git a/test/unit/line.js b/test/unit/line.js index 6243d8c0..d470a2d3 100644 --- a/test/unit/line.js +++ b/test/unit/line.js @@ -125,12 +125,12 @@ equal(oLine.get('y2'), 0, 'missing attributes count as 0 values'); }); - test('straight lines should be displayed', function() { + test('straight lines may have 0 width or heigth', function() { var line1 = new fabric.Line([10,10,100,10]), line2 = new fabric.Line([10,10,10,100]); - equal(line1.get('height'), 1); - equal(line2.get('width'), 1); + equal(line1.get('height'), 0); + equal(line2.get('width'), 0); }); test('changing x/y coords should update width/height', function() { diff --git a/test/unit/object.js b/test/unit/object.js index d54acb90..e804f454 100644 --- a/test/unit/object.js +++ b/test/unit/object.js @@ -328,7 +328,7 @@ }); test('getBoundingRect', function() { - var cObj = new fabric.Object(), + var cObj = new fabric.Object({ strokeWidth: 0 }), boundingRect; ok(typeof cObj.getBoundingRect == 'function'); @@ -361,8 +361,42 @@ equal(boundingRect.height, 334); }); +test('getBoundingRectWithStroke', function() { + var cObj = new fabric.Object(), + boundingRect; + ok(typeof cObj.getBoundingRect == 'function'); + + cObj.setCoords(); + boundingRect = cObj.getBoundingRect(); + equal(boundingRect.left.toFixed(2), -0.5); + equal(boundingRect.top.toFixed(2), -0.5); + equal(boundingRect.width.toFixed(2), 1); + equal(boundingRect.height.toFixed(2), 1); + + cObj.set('width', 123).setCoords(); + boundingRect = cObj.getBoundingRect(); + equal(boundingRect.left.toFixed(2), -0.5); + equal(boundingRect.top.toFixed(2), -0.5); + equal(boundingRect.width.toFixed(2), 124); + equal(boundingRect.height.toFixed(2), 1); + + cObj.set('height', 167).setCoords(); + boundingRect = cObj.getBoundingRect(); + equal(boundingRect.left.toFixed(2), -0.5); + equal(boundingRect.top.toFixed(2), -0.5); + equal(boundingRect.width.toFixed(2), 124); + equal(boundingRect.height.toFixed(2), 168); + + cObj.scale(2).setCoords(); + boundingRect = cObj.getBoundingRect(); + equal(boundingRect.left.toFixed(2), -1); + equal(boundingRect.top.toFixed(2), -1); + equal(boundingRect.width.toFixed(2), 248); + equal(boundingRect.height.toFixed(2), 336); + }); + test('getWidth', function() { - var cObj = new fabric.Object(); + var cObj = new fabric.Object({ strokeWidth: 0 }); ok(typeof cObj.getWidth == 'function'); equal(cObj.getWidth(), 0); cObj.set('width', 123); @@ -401,7 +435,7 @@ }); test('scaleToWidth', function() { - var cObj = new fabric.Object({ width: 560 }); + var cObj = new fabric.Object({ width: 560, strokeWidth: 0 }); ok(typeof cObj.scaleToWidth == 'function'); equal(cObj.scaleToWidth(100), cObj, 'chainable'); equal(cObj.getWidth(), 100); @@ -409,7 +443,7 @@ }); test('scaleToHeight', function() { - var cObj = new fabric.Object({ height: 560 }); + var cObj = new fabric.Object({ height: 560, strokeWidth: 0 }); ok(typeof cObj.scaleToHeight == 'function'); equal(cObj.scaleToHeight(100), cObj, 'chainable'); equal(cObj.getHeight(), 100); @@ -417,14 +451,14 @@ }); test('scaleToWidth on rotated object', function() { - var obj = new fabric.Object({ height: 100, width: 100 }); + var obj = new fabric.Object({ height: 100, width: 100, strokeWidth: 0 }); obj.rotate(45); obj.scaleToWidth(200); equal(Math.round(obj.getBoundingRect().width), 200); }); test('scaleToHeight on rotated object', function() { - var obj = new fabric.Object({ height: 100, width: 100 }); + var obj = new fabric.Object({ height: 100, width: 100, strokeWidth: 0 }); obj.rotate(45); obj.scaleToHeight(300); equal(Math.round(obj.getBoundingRect().height), 300); @@ -456,7 +490,7 @@ }); test('setCoords', function() { - var cObj = new fabric.Object({ left: 150, top: 150, width: 100, height: 100 }); + var cObj = new fabric.Object({ left: 150, top: 150, width: 100, height: 100, strokeWidth: 0}); ok(typeof cObj.setCoords == 'function'); equal(cObj.setCoords(), cObj, 'chainable'); @@ -1134,10 +1168,10 @@ test('toDataURL & reference to canvas', function() { }); test('intersectsWithObject', function() { - var object = new fabric.Object({ left: 20, top: 30, width: 40, height: 50, angle: 230 }), - object1 = new fabric.Object({ left: 20, top: 30, width: 60, height: 30, angle: 10 }), - object2 = new fabric.Object({ left: 25, top: 35, width: 20, height: 20, angle: 50 }), - object3 = new fabric.Object({ left: 50, top: 50, width: 20, height: 20, angle: 0 }); + var object = new fabric.Object({ left: 20, top: 30, width: 40, height: 50, angle: 230, strokeWidth: 0 }), + object1 = new fabric.Object({ left: 20, top: 30, width: 60, height: 30, angle: 10, strokeWidth: 0 }), + object2 = new fabric.Object({ left: 25, top: 35, width: 20, height: 20, angle: 50, strokeWidth: 0 }), + object3 = new fabric.Object({ left: 50, top: 50, width: 20, height: 20, angle: 0, strokeWidth: 0 }); object.set({ originX: 'center', originY: 'center' }).setCoords(); object1.set({ originX: 'center', originY: 'center' }).setCoords(); @@ -1210,7 +1244,7 @@ test('toDataURL & reference to canvas', function() { }); test('containsPoint', function() { - var object = new fabric.Object({ left: 40, top: 40, width: 40, height: 50, angle: 160 }), + var object = new fabric.Object({ left: 40, top: 40, width: 40, height: 50, angle: 160, strokeWidth: 0 }), point1 = new fabric.Point(30, 30), point2 = new fabric.Point(60, 30), point3 = new fabric.Point(45, 65),