From e46c8ec3406471ba463ff84d5a0bf9557c8314ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=BCrg=20Lehni?= Date: Sat, 22 Apr 2017 12:55:42 +0200 Subject: [PATCH] Overhaul the caching of bounds and matrix decomposition. Improves reliability of Item#rotation and #scaling and fixes situations caused by wrongly cached #position and #bounds values. --- README.md | 16 ++-- src/basic/Matrix.js | 12 ++- src/item/Item.js | 189 ++++++++++++++++++++++++-------------- src/svg/SvgExport.js | 5 +- src/text/PointText.js | 4 +- test/tests/Item.js | 53 +++++++++++ test/tests/Item_Bounds.js | 30 +++++- 7 files changed, 217 insertions(+), 92 deletions(-) diff --git a/README.md b/README.md index 923e3dca..ca80c52c 100644 --- a/README.md +++ b/README.md @@ -51,17 +51,17 @@ generally not recommended to install Node.js through OS-supplied package managers, as the its development cycles move fast and these versions are often out-of-date. +On macOS, [Homebrew](http://brew.sh/) is a good option if one version of +Node.js that is kept up to date with `brew upgrade` is enough: + + [NVM](https://github.com/creationix/nvm) can be used instead to install and maintain multiple versions of Node.js on the same platform, as often required by different projects: -on OSX, [Homebrew](http://brew.sh/) is also a good option if one version of -Node.js that is kept up to date with `brew update` is enough: - - -Homebrew is recommended on OSX also if you intend to install Paper.js for -Node.js, as described in the next paragraph. +Homebrew is recommended on macOS also if you intend to install Paper.js with +rendering to the Canvas on Node.js, as described in the next paragraph. For Linux, see to locate 32-bit and 64-bit Node.js binaries as well as sources, or use NVM, as described in the paragraph above. @@ -83,14 +83,14 @@ different one: In order to install `paper-jsdom-canvas`, you need the [Cairo Graphics library](http://cairographics.org/) installed in your system: -##### Installing Cairo and Pango on OSX: +##### Installing Cairo and Pango on macOS: The easiest way to install Cairo is through [Homebrew](http://brew.sh/), by issuing the command: brew install cairo pango -Note that currently there is an issue on OSX with Cairo. If the above causes +Note that currently there is an issue on macOS with Cairo. If the above causes errors, the following will most likely fix it: PKG_CONFIG_PATH=/opt/X11/lib/pkgconfig/ npm install paper diff --git a/src/basic/Matrix.js b/src/basic/Matrix.js index 90d8f0ff..7bf833cf 100644 --- a/src/basic/Matrix.js +++ b/src/basic/Matrix.js @@ -377,7 +377,7 @@ var Matrix = Base.extend(/** @lends Matrix# */{ * @param {Matrix} matrix the matrix to append * @return {Matrix} this matrix, modified */ - append: function(mx) { + append: function(mx, _dontNotify) { if (mx) { var a1 = this._a, b1 = this._b, @@ -395,7 +395,8 @@ var Matrix = Base.extend(/** @lends Matrix# */{ this._d = b2 * b1 + d2 * d1; this._tx += tx2 * a1 + ty2 * c1; this._ty += tx2 * b1 + ty2 * d1; - this._changed(); + if (!_dontNotify) + this._changed(); } return this; }, @@ -407,7 +408,7 @@ var Matrix = Base.extend(/** @lends Matrix# */{ * @param {Matrix} matrix the matrix to prepend * @return {Matrix} this matrix, modified */ - prepend: function(mx) { + prepend: function(mx, _dontNotify) { if (mx) { var a1 = this._a, b1 = this._b, @@ -427,7 +428,8 @@ var Matrix = Base.extend(/** @lends Matrix# */{ this._d = c2 * c1 + d2 * d1; this._tx = a2 * tx1 + b2 * ty1 + tx2; this._ty = c2 * tx1 + d2 * ty1 + ty2; - this._changed(); + if (!_dontNotify) + this._changed(); } return this; }, @@ -671,7 +673,7 @@ var Matrix = Base.extend(/** @lends Matrix# */{ /** * Attempts to decompose the affine transformation described by this matrix - * into `scaling`, `rotation` and `shearing`, and returns an object with + * into `scaling`, `rotation` and `skewing`, and returns an object with * these properties if it succeeded, `null` otherwise. * * @return {Object} the decomposed matrix, or `null` if decomposition is not diff --git a/src/item/Item.js b/src/item/Item.js index be96cac1..08882ff0 100644 --- a/src/item/Item.js +++ b/src/item/Item.js @@ -827,14 +827,14 @@ new function() { // Injection scope for various item event handlers opts.cacheItem = this; // If we're caching bounds, pass on this item as cacheItem, so // the children can setup _boundsCache structures for it. - var bounds = this._getCachedBounds(hasMatrix && matrix, opts); + var rect = this._getCachedBounds(hasMatrix && matrix, opts).rect; // If we're returning '#bounds', create a LinkedRectangle that uses // the setBounds() setter to update the Item whenever the bounds are // changed: return !arguments.length - ? new LinkedRectangle(bounds.x, bounds.y, bounds.width, - bounds.height, this, 'setBounds') - : bounds; + ? new LinkedRectangle(rect.x, rect.y, rect.width, rect.height, + this, 'setBounds') + : rect; }, setBounds: function(/* rect */) { @@ -889,6 +889,14 @@ new function() { // Injection scope for various item event handlers return Item._getBounds(children, matrix, options); }, + _getBoundsCacheKey: function(options, internal) { + return [ + options.stroke ? 1 : 0, + options.handle ? 1 : 0, + internal ? 1 : 0 + ].join(''); + }, + /** * Private method that deals with the calling of _getBounds, recursive * matrix concatenation and handles all the complicated caching mechanisms. @@ -904,29 +912,43 @@ new function() { // Injection scope for various item event handlers cacheItem = options.cacheItem, _matrix = internal ? null : this._matrix._orNullIfIdentity(), // Create a key for caching, reflecting all bounds options. - cacheKey = cacheItem && (!matrix || matrix.equals(_matrix)) && [ - options.stroke ? 1 : 0, - options.handle ? 1 : 0, - internal ? 1 : 0 - ].join(''); + cacheKey = cacheItem && (!matrix || matrix.equals(_matrix)) + && this._getBoundsCacheKey(options, internal), + bounds = this._bounds; // NOTE: This needs to happen before returning cached values, since even // then, _boundsCache needs to be kept up-to-date. Item._updateBoundsCache(this._parent || this._symbol, cacheItem); - if (cacheKey && this._bounds && cacheKey in this._bounds) - return this._bounds[cacheKey].rect.clone(); - var bounds = this._getBounds(matrix || _matrix, options); + if (cacheKey && bounds && cacheKey in bounds) { + var cached = bounds[cacheKey]; + return { + rect: cached.rect.clone(), + nonscaling: cached.nonscaling + }; + } + var res = this._getBounds(matrix || _matrix, options), + // Support two versions of _getBounds(): One that directly returns a + // Rectangle, and one that returns a bounds object with nonscaling. + rect = res.rect || res, + style = this._style, + nonscaling = res.nonscaling || style.hasStroke() + && !style.getStrokeScaling(); // If we can cache the result, update the _bounds cache structure // before returning if (cacheKey) { - if (!this._bounds) - this._bounds = {}; - var cached = this._bounds[cacheKey] = { - rect: bounds.clone(), + if (!bounds) { + this._bounds = bounds = {}; + } + var cached = bounds[cacheKey] = { + rect: rect.clone(), + nonscaling: nonscaling, // Mark as internal, so Item#transform() won't transform it internal: internal }; } - return bounds; + return { + rect: rect, + nonscaling: nonscaling + }; }, /** @@ -1005,7 +1027,10 @@ new function() { // Injection scope for various item event handlers var x1 = Infinity, x2 = -x1, y1 = x1, - y2 = x2; + y2 = x2, + nonscaling = false; + // NOTE: As soon as one child-item has non-scaling strokes, the full + // bounds need to be considered non-scaling for caching purposes. options = options || {}; for (var i = 0, l = items.length; i < l; i++) { var item = items[i]; @@ -1013,17 +1038,23 @@ new function() { // Injection scope for various item event handlers // Pass true for noInternal, since even when getting // internal bounds for this item, we need to apply the // matrices to its children. - var rect = item._getCachedBounds( - matrix && matrix.appended(item._matrix), options, true); + var bounds = item._getCachedBounds( + matrix && matrix.appended(item._matrix), options, true), + rect = bounds.rect; x1 = Math.min(rect.x, x1); y1 = Math.min(rect.y, y1); x2 = Math.max(rect.x + rect.width, x2); y2 = Math.max(rect.y + rect.height, y2); + if (bounds.nonscaling) + nonscaling = true; } } - return isFinite(x1) + return { + rect: isFinite(x1) ? new Rectangle(x1, y1, x2 - x1, y2 - y1) - : new Rectangle(); + : new Rectangle(), + nonscaling: nonscaling + }; } } @@ -1122,8 +1153,22 @@ new function() { // Injection scope for various item event handlers scaling = Point.read(arguments, 0, { clone: true, readNull: true }); if (current && scaling && !current.equals(scaling)) { // See #setRotation() for preservation of _decomposed. - var decomposed = this._decomposed; - this.scale(scaling.x / current.x, scaling.y / current.y); + var rotation = this.getRotation(), + decomposed = this._decomposed, + matrix = new Matrix(), + center = this.getPosition(true); + // Create a matrix in which the scaling is applied in the non- + // rotated state, so it is always applied before the rotation. + // TODO: What about skewing? Do we need separately stored values for + // these properties, and apply them separately from the matrix? + matrix.translate(center); + if (rotation) + matrix.rotate(rotation); + matrix.scale(scaling.x / current.x, scaling.y / current.y); + if (rotation) + matrix.rotate(-rotation); + matrix.translate(center.negate()); + this.transform(matrix); if (decomposed) { decomposed.scaling = scaling; this._decomposed = decomposed; @@ -3376,11 +3421,9 @@ new function() { // Injection scope for hit-test functions shared with project // 'lines'. Default: ['objects', 'children'] transform: function(matrix, _applyMatrix, _applyRecursively, _setApplyMatrix) { - // If no matrix is provided, or the matrix is the identity, we might - // still have some work to do in case _applyMatrix is true - if (matrix && matrix.isIdentity()) - matrix = null; var _matrix = this._matrix, + // If no matrix is provided, or the matrix is the identity, we might + // still have some work to do in case _applyMatrix is true transform = matrix && !matrix.isIdentity(), applyMatrix = (_applyMatrix || this._applyMatrix) // Don't apply _matrix if the result of concatenating with @@ -3398,30 +3441,8 @@ new function() { // Injection scope for hit-test functions shared with project // non-invertible. This is then used again in setBounds to restore. if (!matrix.isInvertible() && _matrix.isInvertible()) _matrix._backup = _matrix.getValues(); - _matrix.prepend(matrix); - } - // Call #_transformContent() now, if we need to directly apply the - // internal _matrix transformations to the item's content. - // Application is not possible on Raster, PointText, SymbolItem, since - // the matrix is where the actual transformation state is stored. - if (applyMatrix) { - if (this._transformContent(_matrix, _applyRecursively, - _setApplyMatrix)) { - var pivot = this._pivot; - if (pivot) - _matrix._transformPoint(pivot, pivot, true); - // Reset the internal matrix to the identity transformation if - // it was possible to apply it. - _matrix.reset(true); - // Set the internal _applyMatrix flag to true if we're told to - // do so - if (_setApplyMatrix && this._canApplyMatrix) - this._applyMatrix = true; - } else { - applyMatrix = transform = false; - } - } - if (transform) { + // Pass `true` for _dontNotify, as we're handling this after. + _matrix.prepend(matrix, true); // When a new matrix was applied, we also need to transform gradient // color points. These always need transforming, regardless of // #applyMatrix, as they are defined in the parent's coordinate @@ -3438,39 +3459,65 @@ new function() { // Injection scope for hit-test functions shared with project if (strokeColor) strokeColor.transform(matrix); } + // Call #_transformContent() now, if we need to directly apply the + // internal _matrix transformations to the item's content. + // Application is not possible on Raster, PointText, SymbolItem, since + // the matrix is where the actual transformation state is stored. + if (applyMatrix && (applyMatrix = this._transformContent(_matrix, + _applyRecursively, _setApplyMatrix))) { + // Pivot is provided in the parent's coordinate system, so transform + // it along too. + var pivot = this._pivot; + if (pivot) + _matrix._transformPoint(pivot, pivot, true); + // Reset the internal matrix to the identity transformation if + // it was possible to apply it, but do not notify owner of change. + _matrix.reset(true); + // Set the internal _applyMatrix flag to true if we're told to + // do so + if (_setApplyMatrix && this._canApplyMatrix) + this._applyMatrix = true; + } // Calling _changed will clear _bounds and _position, but depending // on matrix we can calculate and set them again, so preserve them. var bounds = this._bounds, position = this._position; - // We always need to call _changed since we're caching bounds on all - // items, including Group. - this._changed(/*#=*/Change.GEOMETRY); + if (transform || applyMatrix) { + this._changed(/*#=*/Change.GEOMETRY); + } // Detect matrices that contain only translations and scaling // and transform the cached _bounds and _position without having to // fully recalculate each time. - var decomp = bounds && matrix && matrix.decompose(); - if (decomp && !decomp.shearing && decomp.rotation % 90 === 0) { - // Transform the old bound by looping through all the cached bounds - // in _bounds and transform each. + var decomp = transform && bounds && matrix.decompose(); + if (decomp && decomp.skewing.isZero() && decomp.rotation % 90 === 0) { + // Transform the old bound by looping through all the cached + // bounds in _bounds and transform each. for (var key in bounds) { var cache = bounds[key]; - // If these are internal bounds, only transform them if this - // item applied its matrix. - if (applyMatrix || !cache.internal) { + // If any item involved in the determination of these bounds has + // non-scaling strokes, delete the cache now as it can't be + // preserved through the transformation. + if (cache.nonscaling) { + delete bounds[key]; + } else if (applyMatrix || !cache.internal) { + // If these are internal bounds, only transform them if this + // item applied its matrix. var rect = cache.rect; matrix._transformBounds(rect, rect); } } - // If we have cached bounds, update _position again as its - // center. We need to take into account _boundsGetter here too, in - // case another getter is assigned to it, e.g. 'getStrokeBounds'. - var getter = this._boundsGetter, - rect = bounds[getter && getter.getBounds || getter || 'getBounds']; - if (rect) - this._position = rect.getCenter(true); this._bounds = bounds; - } else if (matrix && position) { - // Transform position as well. + // If we have cached bounds, try to determine _position as its + // center. Use _boundsOptions do get the cached default bounds. + var cached = bounds[this._getBoundsCacheKey( + this._boundsOptions || {})]; + if (cached) { + this._position = cached.rect.getCenter(true); + } + } else if (transform && position && this._pivot) { + // If the item has a pivot defined, it means that the default + // position defined as the center of the bounds won't shift with + // arbitrary transformations and we can therefore update _position: this._position = matrix._transformPoint(position, position); } // Allow chaining here, since transform() is related to Matrix functions diff --git a/src/svg/SvgExport.js b/src/svg/SvgExport.js index 31ee20fb..88b9c44e 100644 --- a/src/svg/SvgExport.js +++ b/src/svg/SvgExport.js @@ -51,9 +51,9 @@ new function() { if (!Numerical.isZero(scale.x - 1) || !Numerical.isZero(scale.y - 1)) parts.push('scale(' + formatter.point(scale) +')'); - if (skew && skew.x) + if (skew.x) parts.push('skewX(' + formatter.number(skew.x) + ')'); - if (skew && skew.y) + if (skew.y) parts.push('skewY(' + formatter.number(skew.y) + ')'); attrs.transform = parts.join(' '); } else { @@ -416,6 +416,7 @@ new function() { ? new Rectangle([0, 0], view.getViewSize()) : bounds === 'content' ? Item._getBounds(children, matrix, { stroke: true }) + .rect : Rectangle.read([bounds], 0, { readNull: true }), attrs = { version: '1.1', diff --git a/src/text/PointText.js b/src/text/PointText.js index 08102640..0886862e 100644 --- a/src/text/PointText.js +++ b/src/text/PointText.js @@ -115,9 +115,9 @@ var PointText = TextItem.extend(/** @lends PointText# */{ x -= width / (justification === 'center' ? 2: 1); // Until we don't have baseline measuring, assume 1 / 4 leading as a // rough guess: - var bounds = new Rectangle(x, + var rect = new Rectangle(x, numLines ? - 0.75 * leading : 0, width, numLines * leading); - return matrix ? matrix._transformBounds(bounds, bounds) : bounds; + return matrix ? matrix._transformBounds(rect, rect) : rect; } }); diff --git a/test/tests/Item.js b/test/tests/Item.js index 34912b13..d19626b4 100644 --- a/test/tests/Item.js +++ b/test/tests/Item.js @@ -876,3 +876,56 @@ test('Item#pivot', function() { equals(path2.pivot, pivot.add(difference), 'Changing position of an item with applyMatrix = true should change pivot'); }); + +test('Item#position with irregular shape, #pivot and rotation', function() { + var path1 = new Path([ [0, 0], [200, 100], [0, 100] ]); + var path2 = path1.clone(); + path2.pivot = path2.position; + equals(path1.position, new Point(100, 50), + 'path1.position, before rotation'); + path1.rotate(45); + equals(path1.position, new Point(64.64466, 50), + 'path1.position, after rotation'); + equals(path2.position, new Point(100, 50), + 'path2.position with pivot, before rotation'); + path2.rotate(45); + equals(path2.position, new Point(100, 50), + 'path2.position with pivot, after rotation'); +}); + +test('Item#scaling, #rotation', function() { + var expected = new Rectangle(100, 50, 100, 200); + + var rect1 = new Path.Rectangle({ + from: [100, 100], + to: [200, 200], + applyMatrix: false + }); + var rect2 = rect1.clone(); + + rect1.scaling = [2, 1]; + rect1.rotation = 90; + equals(rect1.bounds, expected, + 'rect1.bounds, setting rect1.scaling before rect1.rotation'); + + rect2.rotation = 90; + rect2.scaling = [2, 1]; + equals(rect2.bounds, expected, + 'rect2.bounds, setting rect2.scaling before rect2.rotation'); + + var shape1 = new Shape.Rectangle({ + from: [100, 100], + to: [200, 200] + }); + var shape2 = shape1.clone(); + + shape1.scaling = [2, 1]; + shape1.rotation = 90; + equals(shape1.bounds, expected, + 'shape1.bounds, setting shape1.scaling before shape1.rotation'); + + shape2.rotation = 90; + shape2.scaling = [2, 1]; + equals(shape2.bounds, expected, + 'shape2.bounds, setting shape2.scaling before shape2.rotation'); +}); diff --git a/test/tests/Item_Bounds.js b/test/tests/Item_Bounds.js index d6bf114a..b4a3ff59 100644 --- a/test/tests/Item_Bounds.js +++ b/test/tests/Item_Bounds.js @@ -694,12 +694,15 @@ test('path.strokeBounds with applyMatrix disabled', function() { strokeColor: 'red', strokeWidth: 10 }); - equals(path.strokeBounds, new Rectangle(5, 5, 30, 30), 'path.strokeBounds, applyMatrix enabled'); + equals(path.strokeBounds, new Rectangle(5, 5, 30, 30), + 'path.strokeBounds, applyMatrix enabled'); path.applyMatrix = false; - equals(path.strokeBounds, new Rectangle(5, 5, 30, 30), 'path.strokeBounds, applyMatrix disabled'); + equals(path.strokeBounds, new Rectangle(5, 5, 30, 30), + 'path.strokeBounds, applyMatrix disabled'); path.scale([4, 2], [0, 0]); var expected = new Rectangle(20, 10, 120, 60); - equals(path.strokeBounds, expected, 'path.strokeBounds after scaling, applyMatrix disabled'); + equals(path.strokeBounds, expected, + 'path.strokeBounds after scaling, applyMatrix disabled'); function testHitResult() { // Hit-testing needs to handle applyMatrix disabled with stroke scaling, // even when hit-testing on "distorted" stroke joins: @@ -714,10 +717,29 @@ test('path.strokeBounds with applyMatrix disabled', function() { testHitResult(); path.applyMatrix = true; expected = new Rectangle(35, 15, 90, 50); - equals(path.strokeBounds, expected, 'path.strokeBounds after scaling, applyMatrix enabled'); + equals(path.strokeBounds, expected, + 'path.strokeBounds after scaling, applyMatrix enabled'); testHitResult(); }); +test('TEST', function() { + var path = new Path.Rectangle({ + applyMatrix: false, + point: [10, 10], + size: [20, 20], + strokeScaling: true, + strokeColor: 'red', + strokeWidth: 10 + }); + path.scale([4, 2], [0, 0]); + equals(path.strokeBounds, new Rectangle(20, 10, 120, 60), + 'path.strokeBounds after scaling, applyMatrix disabled'); + path.applyMatrix = true; + equals(path.strokeBounds, new Rectangle(35, 15, 90, 50), + 'path.strokeBounds after scaling, applyMatrix enabled'); + +}); + test('symbolItem.bounds with strokeScaling disabled', function() { var path = new Path.Rectangle({ size: [20, 20],