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

@@ -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:  
+<http://treehouse.github.io/installation-guides/mac/node-mac.html>
+
 [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:  
 <https://nodesource.com/blog/installing-node-js-tutorial-using-nvm-on-mac-os-x-and-ubuntu/>
 
-on OSX, [Homebrew](http://brew.sh/) is also a good option if one version of
+Homebrew is recommended on macOS also if you intend to install Paper.js with
-Node.js that is kept up to date with `brew update` is enough:  
+rendering to the Canvas on Node.js, as described in the next paragraph.
-<http://treehouse.github.io/installation-guides/mac/node-mac.html>
-
-Homebrew is recommended on OSX also if you intend to install Paper.js for
-Node.js, as described in the next paragraph.
 
 For Linux, see <http://nodejs.org/download/> 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

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,6 +395,7 @@ var Matrix = Base.extend(/** @lends Matrix# */{
             this._d = b2 * b1 + d2 * d1;
             this._tx += tx2 * a1 + ty2 * c1;
             this._ty += tx2 * b1 + ty2 * d1;
+            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,6 +428,7 @@ 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;
+            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

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,
+                ? new LinkedRectangle(rect.x, rect.y, rect.width, rect.height,
-                        bounds.height, this, 'setBounds')
+                    this, 'setBounds')
-                : bounds;
+                : 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)) && [
+            cacheKey = cacheItem && (!matrix || matrix.equals(_matrix))
-                options.stroke ? 1 : 0,
+                && this._getBoundsCacheKey(options, internal),
-                options.handle ? 1 : 0,
+            bounds = this._bounds;
-                internal ? 1 : 0
-            ].join('');
         // 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)
+        if (cacheKey && bounds && cacheKey in bounds) {
-            return this._bounds[cacheKey].rect.clone();
+            var cached = bounds[cacheKey];
-        var bounds = this._getBounds(matrix || _matrix, options);
+            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)
+            if (!bounds) {
-                this._bounds = {};
+                this._bounds = bounds = {};
-            var cached = this._bounds[cacheKey] = {
+            }
-                rect: bounds.clone(),
+            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(
+                    var bounds = item._getCachedBounds(
-                        matrix && matrix.appended(item._matrix), options, true);
+                        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;
+            var rotation = this.getRotation(),
-            this.scale(scaling.x / current.x, scaling.y / current.y);
+                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) {
+        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
-        if (matrix && matrix.isIdentity())
-            matrix = null;
-        var _matrix = this._matrix,
             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);
+            // Pass `true` for _dontNotify, as we're handling this after.
-        }
+            _matrix.prepend(matrix, true);
-        // 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) {
             // 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
+        if (transform || applyMatrix) {
-        // items, including Group.
             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();
+        var decomp = transform && bounds && matrix.decompose();
-        if (decomp && !decomp.shearing && decomp.rotation % 90 === 0) {
+        if (decomp && decomp.skewing.isZero() && decomp.rotation % 90 === 0) {
-            // Transform the old bound by looping through all the cached bounds
+            // Transform the old bound by looping through all the cached
-            // in _bounds and transform each.
+            // bounds in _bounds and transform each.
             for (var key in bounds) {
                 var cache = bounds[key];
+                // 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.
-                if (applyMatrix || !cache.internal) {
                     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) {
+            // If we have cached bounds, try to determine _position as its
-            // Transform position as well.
+            // 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

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',

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;
     }
 });

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');
+});

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],