Skip to content

Commit

Permalink
MAJOR feat(): Reuse fabric.Point logic for scaling and naming consist…
Browse files Browse the repository at this point in the history 
…ency (#7710)


* fix(Object): object scaling inconsisteny

**MAJOR**
 **BREAKING** `Object.getObjectScaling`, `Object.getTotalObjectScaling`
  • Loading branch information
@ShaMan123
ShaMan123 authored Feb 20, 2022
1 parent 7e563c7 commit 88b425c
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 53 deletions.
6 changes: 3 additions & 3 deletions src/shapes/image.class.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -395,8 +395,8 @@
var filter = this.resizeFilter,
minimumScale = this.minimumScaleTrigger,
objectScale = this.getTotalObjectScaling(),
scaleX = objectScale.scaleX,
scaleY = objectScale.scaleY,
scaleX = objectScale.x,
scaleY = objectScale.y,
elementToFilter = this._filteredEl || this._originalElement;
if (this.group) {
this.set('dirty', true);
Expand Down Expand Up @@ -552,7 +552,7 @@
*/
_needsResize: function() {
var scale = this.getTotalObjectScaling();
return (scale.scaleX !== this._lastScaleX || scale.scaleY !== this._lastScaleY);
return (scale.x !== this._lastScaleX || scale.y !== this._lastScaleY);
},

/**
Expand Down
59 changes: 22 additions & 37 deletions src/shapes/object.class.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -728,16 +728,16 @@
var objectScale = this.getTotalObjectScaling(),
// caculate dimensions without skewing
dim = this._getTransformedDimensions(0, 0),
neededX = dim.x * objectScale.scaleX / this.scaleX,
neededY = dim.y * objectScale.scaleY / this.scaleY;
neededX = dim.x * objectScale.x / this.scaleX,
neededY = dim.y * objectScale.y / this.scaleY;
return {
// for sure this ALIASING_LIMIT is slightly creating problem
// in situation in which the cache canvas gets an upper limit
// also objectScale contains already scaleX and scaleY
width: neededX + ALIASING_LIMIT,
height: neededY + ALIASING_LIMIT,
zoomX: objectScale.scaleX,
zoomY: objectScale.scaleY,
zoomX: objectScale.x,
zoomY: objectScale.y,
x: neededX,
y: neededY
};
Expand Down Expand Up @@ -930,37 +930,33 @@

/**
* Return the object scale factor counting also the group scaling
* @return {Object} object with scaleX and scaleY properties
* @return {fabric.Point}
*/
getObjectScaling: function() {
// if the object is a top level one, on the canvas, we go for simple aritmetic
// otherwise the complex method with angles will return approximations and decimals
// and will likely kill the cache when not needed
// https://github.com/fabricjs/fabric.js/issues/7157
if (!this.group) {
return {
scaleX: this.scaleX,
scaleY: this.scaleY,
};
return new fabric.Point(Math.abs(this.scaleX), Math.abs(this.scaleY));
}
// if we are inside a group total zoom calculation is complex, we defer to generic matrices
var options = fabric.util.qrDecompose(this.calcTransformMatrix());
return { scaleX: Math.abs(options.scaleX), scaleY: Math.abs(options.scaleY) };
return new fabric.Point(Math.abs(options.scaleX), Math.abs(options.scaleY));
},

/**
* Return the object scale factor counting also the group scaling, zoom and retina
* @return {Object} object with scaleX and scaleY properties
*/
getTotalObjectScaling: function() {
var scale = this.getObjectScaling(), scaleX = scale.scaleX, scaleY = scale.scaleY;
var scale = this.getObjectScaling();
if (this.canvas) {
var zoom = this.canvas.getZoom();
var retina = this.canvas.getRetinaScaling();
scaleX *= zoom * retina;
scaleY *= zoom * retina;
scale.scalarMultiplyEquals(zoom * retina);
}
return { scaleX: scaleX, scaleY: scaleY };
return scale;
},

/**
Expand Down Expand Up @@ -1427,24 +1423,19 @@
return;
}

var shadow = this.shadow, canvas = this.canvas, scaling,
var shadow = this.shadow, canvas = this.canvas,
multX = (canvas && canvas.viewportTransform[0]) || 1,
multY = (canvas && canvas.viewportTransform[3]) || 1;
if (shadow.nonScaling) {
scaling = { scaleX: 1, scaleY: 1 };
}
else {
scaling = this.getObjectScaling();
}
multY = (canvas && canvas.viewportTransform[3]) || 1,
scaling = shadow.nonScaling ? new fabric.Point(1, 1) : this.getObjectScaling();
if (canvas && canvas._isRetinaScaling()) {
multX *= fabric.devicePixelRatio;
multY *= fabric.devicePixelRatio;
}
ctx.shadowColor = shadow.color;
ctx.shadowBlur = shadow.blur * fabric.browserShadowBlurConstant *
(multX + multY) * (scaling.scaleX + scaling.scaleY) / 4;
ctx.shadowOffsetX = shadow.offsetX * multX * scaling.scaleX;
ctx.shadowOffsetY = shadow.offsetY * multY * scaling.scaleY;
(multX + multY) * (scaling.x + scaling.y) / 4;
ctx.shadowOffsetX = shadow.offsetX * multX * scaling.x;
ctx.shadowOffsetY = shadow.offsetY * multY * scaling.y;
},

/**
Expand Down Expand Up @@ -1549,7 +1540,7 @@
ctx.save();
if (this.strokeUniform && this.group) {
var scaling = this.getObjectScaling();
ctx.scale(1 / scaling.scaleX, 1 / scaling.scaleY);
ctx.scale(1 / scaling.x, 1 / scaling.y);
}
else if (this.strokeUniform) {
ctx.scale(1 / this.scaleX, 1 / this.scaleY);
Expand Down Expand Up @@ -1717,21 +1708,15 @@
var el = fabric.util.createCanvasElement(),
// skip canvas zoom and calculate with setCoords now.
boundingRect = this.getBoundingRect(true, true),
shadow = this.shadow, scaling,
shadowOffset = { x: 0, y: 0 }, shadowBlur,
shadow = this.shadow, shadowOffset = { x: 0, y: 0 },
width, height;

if (shadow) {
shadowBlur = shadow.blur;
if (shadow.nonScaling) {
scaling = { scaleX: 1, scaleY: 1 };
}
else {
scaling = this.getObjectScaling();
}
var shadowBlur = shadow.blur;
var scaling = shadow.nonScaling ? new fabric.Point(1, 1) : this.getObjectScaling();
// consider non scaling shadow.
shadowOffset.x = 2 * Math.round(abs(shadow.offsetX) + shadowBlur) * (abs(scaling.scaleX));
shadowOffset.y = 2 * Math.round(abs(shadow.offsetY) + shadowBlur) * (abs(scaling.scaleY));
shadowOffset.x = 2 * Math.round(abs(shadow.offsetX) + shadowBlur) * (abs(scaling.x));
shadowOffset.y = 2 * Math.round(abs(shadow.offsetY) + shadowBlur) * (abs(scaling.y));
}
width = boundingRect.width + shadowOffset.x;
height = boundingRect.height + shadowOffset.y;
Expand Down
28 changes: 15 additions & 13 deletions test/unit/object.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -806,7 +806,8 @@
canvas.setZoom(3);
canvas.add(object);
var objectScale = object.getTotalObjectScaling();
assert.deepEqual(objectScale, { scaleX: object.scaleX * 3, scaleY: object.scaleY * 3 });
assert.ok(objectScale instanceof fabric.Point);
assert.deepEqual(objectScale, new fabric.Point(object.scaleX * 3, object.scaleY * 3));
});

QUnit.test('getTotalObjectScaling with retina', function(assert) {
Expand All @@ -815,13 +816,15 @@
fabric.devicePixelRatio = 4;
canvas.add(object);
var objectScale = object.getTotalObjectScaling();
assert.deepEqual(objectScale, { scaleX: object.scaleX * 4, scaleY: object.scaleY * 4 });
assert.ok(objectScale instanceof fabric.Point);
assert.deepEqual(objectScale, new fabric.Point(object.scaleX * 4, object.scaleY * 4));
});

QUnit.test('getObjectScaling', function(assert) {
var object = new fabric.Object({ scaleX: 3, scaleY: 2});
var objectScale = object.getObjectScaling();
assert.deepEqual(objectScale, {scaleX: object.scaleX, scaleY: object.scaleY});
assert.ok(objectScale instanceof fabric.Point);
assert.deepEqual(objectScale, new fabric.Point(object.scaleX, object.scaleY));
});

QUnit.test('getObjectScaling in group', function(assert) {
Expand All @@ -831,10 +834,11 @@
group.scaleY = 2;
object.group = group;
var objectScale = object.getObjectScaling();
assert.deepEqual(objectScale, {
scaleX: object.scaleX * group.scaleX,
scaleY: object.scaleY * group.scaleY
});
assert.ok(objectScale instanceof fabric.Point);
assert.deepEqual(objectScale, new fabric.Point(
object.scaleX * group.scaleX,
object.scaleY * group.scaleY
));
});

QUnit.test('getObjectScaling in group with object rotated', function(assert) {
Expand All @@ -844,12 +848,10 @@
group.scaleY = 3;
object.group = group;
var objectScale = object.getObjectScaling();
objectScale.scaleX = objectScale.scaleX.toFixed(3);
objectScale.scaleY = objectScale.scaleY.toFixed(3);
assert.deepEqual(objectScale, {
scaleX: '7.649',
scaleY: '4.707',
});
assert.deepEqual(
new fabric.Point(Math.round(objectScale.x * 1000) / 1000, Math.round(objectScale.y * 1000) / 1000),
new fabric.Point(7.649, 4.707)
);
});

QUnit.test('dirty flag on set property', function(assert) {
Expand Down

0 comments on commit 88b425c

Please sign in to comment.