Improve handling of sub-path orientation in CompoundPath.

Remove automatic orientation on insertion, as it caused more troubles than solved problems, in favor of the new PathItem#reorient() method, or the even-odd fill-rule.

Closes #590, #1029
This commit is contained in:
Jürg Lehni 2016-07-21 15:21:45 +02:00
parent fc72c05e69
commit a0417040f8
10 changed files with 151 additions and 217 deletions

View file

@ -157,7 +157,7 @@ new function() { // Injection scope for various item event handlers
this._setProject(project);
} else {
(hasProps && props.parent || project)
._insertItem(undefined, this, true, true);
._insertItem(undefined, this, true); // _created = true
}
// Filter out Item.NO_INSERT before _set(), for performance reasons.
if (hasProps && props !== Item.NO_INSERT) {
@ -1373,9 +1373,9 @@ new function() { // Injection scope for various item event handlers
return this._children;
},
setChildren: function(items, _preserve) {
setChildren: function(items) {
this.removeChildren();
this.addChildren(items, _preserve);
this.addChildren(items);
},
/**
@ -2287,8 +2287,8 @@ new function() { // Injection scope for hit-test functions shared with project
* @param {Item} item the item to be added as a child
* @return {Item} the added item, or `null` if adding was not possible
*/
addChild: function(item, _preserve) {
return this.insertChild(undefined, item, _preserve);
addChild: function(item) {
return this.insertChild(undefined, item);
},
/**
@ -2300,8 +2300,8 @@ new function() { // Injection scope for hit-test functions shared with project
* @param {Item} item the item to be inserted as a child
* @return {Item} the inserted item, or `null` if inserting was not possible
*/
insertChild: function(index, item, _preserve) {
var res = item ? this.insertChildren(index, [item], _preserve) : null;
insertChild: function(index, item) {
var res = item ? this.insertChildren(index, [item]) : null;
return res && res[0];
},
@ -2313,8 +2313,8 @@ new function() { // Injection scope for hit-test functions shared with project
* @param {Item[]} items the items to be added as children
* @return {Item[]} the added items, or `null` if adding was not possible
*/
addChildren: function(items, _preserve) {
return this.insertChildren(this._children.length, items, _preserve);
addChildren: function(items) {
return this.insertChildren(this._children.length, items);
},
/**
@ -2327,9 +2327,7 @@ new function() { // Injection scope for hit-test functions shared with project
* @return {Item[]} the inserted items, or `null` if inserted was not
* possible
*/
insertChildren: function(index, items, _preserve) {
// CompoundPath#insertChildren() requires _preserve and _type:
// _preserve avoids changing of the children's path orientation
insertChildren: function(index, items) {
var children = this._children;
if (children && items && items.length > 0) {
// We need to clone items because it may be an Item#children array.
@ -2386,7 +2384,7 @@ new function() { // Injection scope for hit-test functions shared with project
* @param {Number} offset the offset at which the item should be inserted
* @return {Item} the inserted item, or `null` if inserting was not possible
*/
_insertAt: function(item, offset, _preserve) {
_insertAt: function(item, offset) {
var res = this;
if (res !== item) {
var owner = item && item._getOwner();
@ -2394,7 +2392,7 @@ new function() { // Injection scope for hit-test functions shared with project
// Notify parent of change. Don't notify item itself yet,
// as we're doing so when adding it to the new owner below.
res._remove(false, true);
owner._insertItem(item._index + offset, res, _preserve);
owner._insertItem(item._index + offset, res);
} else {
res = null;
}
@ -2408,8 +2406,8 @@ new function() { // Injection scope for hit-test functions shared with project
* @param {Item} item the item above which it should be inserted
* @return {Item} the inserted item, or `null` if inserting was not possible
*/
insertAbove: function(item, _preserve) {
return this._insertAt(item, 1, _preserve);
insertAbove: function(item) {
return this._insertAt(item, 1);
},
/**
@ -2418,8 +2416,8 @@ new function() { // Injection scope for hit-test functions shared with project
* @param {Item} item the item below which it should be inserted
* @return {Item} the inserted item, or `null` if inserting was not possible
*/
insertBelow: function(item, _preserve) {
return this._insertAt(item, 0, _preserve);
insertBelow: function(item) {
return this._insertAt(item, 0);
},
/**

View file

@ -378,13 +378,13 @@ var Project = PaperScopeItem.extend(/** @lends Project# */{
// Project#_insertItem() and Item#_insertItem() are helper functions called
// in Item#copyTo(), and through _getOwner() in the various Item#insert*()
// methods. They are called the same to facilitate so duck-typing.
_insertItem: function(index, item, _preserve, _created) {
_insertItem: function(index, item, _created) {
item = this.insertLayer(index, item)
// Anything else than layers needs to be added to a layer first.
// If none exists yet, create one now, then add the item to it.
|| (this._activeLayer || this._insertItem(undefined,
new Layer(Item.NO_INSERT), true, true))
.insertChild(index, item, _preserve);
new Layer(Item.NO_INSERT), true)) // _created = true
.insertChild(index, item);
// If a layer was newly created, also activate it.
if (_created && item.activate)
item.activate();

View file

@ -106,7 +106,7 @@ var CompoundPath = PathItem.extend(/** @lends CompoundPath# */{
}
},
insertChildren: function insertChildren(index, items, _preserve) {
insertChildren: function insertChildren(index, items) {
// If we're passed a segment array describing a simple path instead of a
// compound-path, wrap it in another array to turn it into the array
// notation for compound-paths.
@ -124,28 +124,13 @@ var CompoundPath = PathItem.extend(/** @lends CompoundPath# */{
if (list === items && !(item instanceof Path))
list = Base.slice(list);
if (Array.isArray(item)) {
var path = new Path({ segments: item, insert: false });
// Fix natural clockwise value, so it's not automatically
// determined when inserted into the compound-path.
// TODO: Remove reorientation code instead.
path.setClockwise(path.isClockwise());
list[i] = path;
list[i] = new Path({ segments: item, insert: false });
} else if (item instanceof CompoundPath) {
list.splice.apply(list, [i, 1].concat(item.removeChildren()));
item.remove();
}
}
list = insertChildren.base.call(this, index, list, _preserve);
// All children except for the bottom one (first one in list) are set
// to anti-clockwise orientation, so that they appear as holes, but
// only if their orientation was not already specified before
// (= _clockwise is defined).
for (var i = 0, l = !_preserve && list && list.length; i < l; i++) {
var item = list[i];
if (item._clockwise === undefined)
item.setClockwise(item._index === 0);
}
return list;
return insertChildren.base.call(this, index, list);
},
// DOCS: reduce()
@ -167,22 +152,6 @@ var CompoundPath = PathItem.extend(/** @lends CompoundPath# */{
return reduce.base.call(this);
},
/**
* Specifies whether the compound path is oriented clock-wise.
*
* @bean
* @type Boolean
*/
isClockwise: function() {
var child = this.getFirstChild();
return child && child.isClockwise();
},
setClockwise: function(clockwise) {
if (this.isClockwise() ^ !!clockwise)
this.reverse();
},
/**
* Specifies whether the compound-path is fully closed, meaning all its
* contained sub-paths are closed path.

View file

@ -134,17 +134,12 @@ var Path = PathItem.extend(/** @lends Path# */{
copyContent: function(source) {
this.setSegments(source._segments);
this._closed = source._closed;
var clockwise = source._clockwise;
if (clockwise !== undefined)
this._clockwise = clockwise;
},
_changed: function _changed(flags) {
_changed.base.call(this, flags);
if (flags & /*#=*/ChangeFlag.GEOMETRY) {
// Clockwise state becomes undefined as soon as geometry changes.
// Also clear cached mono curves used for winding calculations.
this._length = this._area = this._clockwise = undefined;
this._length = this._area = undefined;
if (flags & /*#=*/ChangeFlag.SEGMENTS) {
this._version++; // See CurveLocation
} else if (this._curves) {
@ -854,29 +849,6 @@ var Path = PathItem.extend(/** @lends Path# */{
return area;
},
/**
* Specifies whether the path is oriented clock-wise.
*
* @bean
* @type Boolean
*/
isClockwise: function() {
if (this._clockwise !== undefined)
return this._clockwise;
return this.getArea() >= 0;
},
setClockwise: function(clockwise) {
// Only revers the path if its clockwise orientation is not the same
// as what it is now demanded to be.
// On-the-fly conversion to boolean:
if (this.isClockwise() != (clockwise = !!clockwise))
this.reverse();
// Reverse only flips _clockwise state if it was already set, so let's
// always set this here now.
this._clockwise = clockwise;
},
/**
* Specifies whether an path is selected and will also return `true` if the
* path is partially selected, i.e. one or more of its segments is selected.
@ -1084,9 +1056,7 @@ var Path = PathItem.extend(/** @lends Path# */{
path = this;
} else {
path = new Path(Item.NO_INSERT);
// Pass true for _preserve, in case of CompoundPath, to avoid
// reversing of path direction, which would mess with segments!
path.insertAbove(this, true);
path.insertAbove(this);
path.copyAttributes(this);
}
path._add(segs, 0);
@ -1264,9 +1234,6 @@ var Path = PathItem.extend(/** @lends Path# */{
}
// Clear curves since it all has changed.
this._curves = null;
// Flip clockwise state if it's defined
if (this._clockwise !== undefined)
this._clockwise = !this._clockwise;
this._changed(/*#=*/Change.GEOMETRY);
},

View file

@ -55,7 +55,9 @@ PathItem.inject(new function() {
.transform(null, true, true);
if (closed)
res.setClosed(true);
return closed ? res.resolveCrossings().reorient(true) : res;
return closed
? res.resolveCrossings().reorient(res.getFillRule() === 'nonzero')
: res;
}
function createResult(ctor, paths, reduce, path1, path2) {
@ -990,11 +992,8 @@ PathItem.inject(new function() {
var length = paths.length,
item;
if (length > 1 && children) {
if (paths !== children) {
// TODO: Fix automatic child-orientation in CompoundPath,
// and stop passing true for _preserve.
this.setChildren(paths, true); // Preserve orientation
}
if (paths !== children)
this.setChildren(paths);
item = this;
} else if (length === 1 && !children) {
if (paths[0] !== this)
@ -1005,7 +1004,7 @@ PathItem.inject(new function() {
// and attempt to replace this item with it.
if (!item) {
item = new CompoundPath(Item.NO_INSERT);
item.addChildren(paths, true); // Preserve orientation
item.addChildren(paths);
item = item.reduce();
item.copyAttributes(this);
this.replaceWith(item);
@ -1015,49 +1014,44 @@ PathItem.inject(new function() {
/**
* Fixes the orientation of the sub-paths of a compound-path, assuming
* that non of its sub-paths intersect, by reorienting sub-paths so that
* they are of different winding direction than their containing path,
* except for disjoint sub-paths, i.e. islands, which are reoriented so
* that non of its sub-paths intersect, by reorienting them so that they
* are of different winding direction than their containing paths,
* except for disjoint sub-paths, i.e. islands, which are oriented so
* that they have the same winding direction as the the biggest path.
*
* Additionally, if the compound-path has the `'nonzero'`
* {@link #getFillRule()}, the winding of each nested path is counted,
* and sub-paths that do not contribute to the final result are
* discarded.
*
* @param {Boolean} [sort=false] controls if the sub-paths should be
* sorted according to their area from largest to smallest, or if
* normal sequence should be preserved
* @param {Boolean} [nonZero=false] controls if the non-zero fill-rule
* is to be applied, by counting the winding of each nested path and
* discarding sub-paths that do not contribute to the final result
* @return {PahtItem} a reference to the item itself, reoriented
* @see #getFillRule()
*/
reorient: function(sort) {
reorient: function(nonZero) {
var children = this._children,
length = children && children.length;
if (length > 1) {
children = this.removeChildren();
// First order the paths by their areas.
var sorted = children.slice().sort(function (a, b) {
// Build a lookup table with information for each path's
// original index and winding contribution.
var lookup = Base.each(children, function(path, i) {
this[path._id] = {
winding: path.isClockwise() ? 1 : -1,
index: i
};
}, {}),
// Now sort the paths by their areas, from large to small.
sorted = this.removeChildren().sort(function (a, b) {
return abs(b.getArea()) - abs(a.getArea());
}),
// Get reference to the first, largest path and insert it
// already.
first = sorted[0],
paths = [first],
isNonZero = this.getFillRule() === 'nonzero',
// We only need to build a lookup table with information for
// each path if we process with non-zero fill-rule, or if we
// are to preserve the original sequence in the result.
lookup = (isNonZero || !sort) && Base.each(children,
function(path, i) {
this[path._id] = {
winding: path.isClockwise() ? 1 : -1,
index: i
};
}, {});
// Walk through sorted paths, from largest to smallest.
// The first, largest path can be skipped.
paths = [];
// Always insert paths at their original index. With exclusion,
// this produces null entries, but #setChildren() handles those.
paths[lookup[first._id].index] = first;
// Walk through the sorted paths, from largest to smallest.
// Skip the first path, as it is already added.
for (var i1 = 1; i1 < length; i1++) {
var path1 = sorted[i1],
entry1 = lookup && lookup[path1._id],
entry1 = lookup[path1._id],
point = path1.getInteriorPoint(),
isContained = false,
container = null,
@ -1073,8 +1067,8 @@ PathItem.inject(new function() {
// contains the current path, and then set the
// orientation to the opposite of the containing path.
if (path2.contains(point)) {
var entry2 = lookup && lookup[path2._id];
if (isNonZero && !isContained) {
var entry2 = lookup[path2._id];
if (nonZero && !isContained) {
entry1.winding += entry2.winding;
// Remove path if rule is nonzero and winding
// of path and containing path is not zero.
@ -1086,7 +1080,7 @@ PathItem.inject(new function() {
isContained = true;
// If the containing path is not excluded, we're
// done searching for the orientation defining path.
container = !(entry2 && entry2.exclude) && path2;
container = !entry2.exclude && path2;
}
}
if (!exclude) {
@ -1096,18 +1090,10 @@ PathItem.inject(new function() {
path1.setClockwise(container
? !container.isClockwise()
: first.isClockwise());
if (!sort) {
// If asked to preserve sequence (not sort children
// according to their area), insert back at their
// original index. With exclusion this produces null
// entries, but #setChildren() can handle those.
paths[entry1.index] = path1;
} else {
paths.push(path1);
}
paths[entry1.index] = path1;
}
}
this.setChildren(paths, true); // Preserve orientation
this.setChildren(paths);
}
return this;
},

View file

@ -90,6 +90,24 @@ var PathItem = Item.extend(/** @lends PathItem# */{
return this;
},
/**
* Specifies whether the path is oriented clock-wise.
*
* @bean
* @type Boolean
*/
isClockwise: function() {
return this.getArea() >= 0;
},
setClockwise: function(clockwise) {
// Only revers the path if its clockwise orientation is not the same
// as what it is now demanded to be.
// On-the-fly conversion to boolean:
if (this.isClockwise() != (clockwise = !!clockwise))
this.reverse();
},
/**
* The path's geometry, formatted as SVG style path data.
*
@ -97,7 +115,6 @@ var PathItem = Item.extend(/** @lends PathItem# */{
* @bean
* @type String
*/
setPathData: function(data) {
// NOTE: #getPathData() is defined in CompoundPath / Path
// This is a very compact SVG Path Data parser that works both for Path

View file

@ -34,10 +34,10 @@ test('moveTo() / lineTo()', function() {
}, 2);
});
test('CompoundPath() and default clockwise settings', function() {
var path1 = new Path.Rectangle([200, 200], [100, 100]);
test('CompoundPath#reorient()', function() {
var path1 = new Path.Rectangle([300, 300], [100, 100]);
var path2 = new Path.Rectangle([50, 50], [200, 200]);
var path3 = new Path.Rectangle([0, 0], [400, 400]);
var path3 = new Path.Rectangle([0, 0], [500, 500]);
equals(function() {
return path1.clockwise;
@ -49,7 +49,9 @@ test('CompoundPath() and default clockwise settings', function() {
return path3.clockwise;
}, true);
var compound = new CompoundPath(path1, path2, path3);
var compound = new CompoundPath({
children: [path1, path2, path3],
}).reorient();
equals(function() {
return compound.lastChild == path3;
@ -59,29 +61,10 @@ test('CompoundPath() and default clockwise settings', function() {
}, true);
equals(function() {
return path1.clockwise;
}, true);
}, false);
equals(function() {
return path2.clockwise;
}, false);
equals(function() {
return path3.clockwise;
}, false);
});
test('CompoundPath() and predefined clockwise settings', function() {
var path1 = new Path.Rectangle([200, 200], [100, 100]);
var path2 = new Path.Rectangle([50, 50], [200, 200]);
var path3 = new Path.Rectangle([0, 0], [400, 400]);
path1.clockwise = false;
path2.clockwise = true;
path3.clockwise = true;
var compound = new CompoundPath(path1, path2, path3);
equals(function() {
return path1.clockwise;
}, false);
equals(function() {
return path2.clockwise;
}, true);
equals(function() {
return path3.clockwise;
}, true);

View file

@ -696,7 +696,8 @@ test('hit-testing compound-paths', function() {
});
var compoundPath = new CompoundPath({
children: [path1, path2],
fillColor: 'blue'
fillColor: 'blue',
fillRule: 'evenodd'
});
// When hit-testing a side, we should get a result on the torus
equals(function() {

View file

@ -96,40 +96,50 @@ test('CompoundPath#contains() (donut)', function() {
new Path.Circle([0, 0], 25)
]);
testPoint(path, new Point(0, -50), true,
'The top center point of the outer circle should be inside the donut.');
testPoint(path, new Point(0, 0), false,
'The center point should be outside the donut.');
testPoint(path, new Point(-35, 0), true,
'A vertically centered point on the left side should be inside the donut.');
testPoint(path, new Point(35, 0), true,
'A vertically centered point on the right side should be inside the donut.');
testPoint(path, new Point(0, 49), true,
'The near bottom center point of the outer circle should be inside the donut.');
testPoint(path, new Point(0, 50), true,
'The bottom center point of the outer circle should be inside the donut.');
testPoint(path, new Point(0, 51), false,
'The near bottom center point of the outer circle should be outside the donut.');
testPoint(path, new Point({ length: 50, angle: 30 }), true,
'A random point on the periphery of the outer circle should be inside the donut.');
testPoint(path, new Point(-25, 0), true,
'The left center point of the inner circle should be inside the donut.');
testPoint(path, new Point(0, -25), true,
'The top center point of the inner circle should be inside the donut.');
testPoint(path, new Point(25, 0), true,
'The right center point of the inner circle should be inside the donut.');
testPoint(path, new Point(0, 25), true,
'The bottom center point of the inner circle should be inside the donut.');
testPoint(path, new Point(-50, -50), false,
'The top left point of bounding box should be outside the donut.');
testPoint(path, new Point(50, -50), false,
'The top right point of the bounding box should be outside the donut.');
testPoint(path, new Point(-50, 50), false,
'The bottom left point of bounding box should be outside the donut.');
testPoint(path, new Point(50, 50), false,
'The bottom right point of the bounding box should be outside the donut.');
testPoint(path, new Point(-45, 45), false,
'The near bottom left point of bounding box should be outside the donut.');
function testDonut(path, title) {
title = 'fillRule = ' + title + ': ';
testPoint(path, new Point(0, -50), true, title +
'The top center point of the outer circle should be inside the donut.');
testPoint(path, new Point(0, 0), false, title +
'The center point should be outside the donut.');
testPoint(path, new Point(-35, 0), true, title +
'A vertically centered point on the left side should be inside the donut.');
testPoint(path, new Point(35, 0), true, title +
'A vertically centered point on the right side should be inside the donut.');
testPoint(path, new Point(0, 49), true, title +
'The near bottom center point of the outer circle should be inside the donut.');
testPoint(path, new Point(0, 50), true, title +
'The bottom center point of the outer circle should be inside the donut.');
testPoint(path, new Point(0, 51), false, title +
'The near bottom center point of the outer circle should be outside the donut.');
testPoint(path, new Point({ length: 50, angle: 30 }), true, title +
'A random point on the periphery of the outer circle should be inside the donut.');
testPoint(path, new Point(-25, 0), true, title +
'The left center point of the inner circle should be inside the donut.');
testPoint(path, new Point(0, -25), true, title +
'The top center point of the inner circle should be inside the donut.');
testPoint(path, new Point(25, 0), true, title +
'The right center point of the inner circle should be inside the donut.');
testPoint(path, new Point(0, 25), true, title +
'The bottom center point of the inner circle should be inside the donut.');
testPoint(path, new Point(-50, -50), false, title +
'The top left point of bounding box should be outside the donut.');
testPoint(path, new Point(50, -50), false, title +
'The top right point of the bounding box should be outside the donut.');
testPoint(path, new Point(-50, 50), false, title +
'The bottom left point of bounding box should be outside the donut.');
testPoint(path, new Point(50, 50), false, title +
'The bottom right point of the bounding box should be outside the donut.');
testPoint(path, new Point(-45, 45), false, title +
'The near bottom left point of bounding box should be outside the donut.');
}
path.fillRule = 'evenodd';
// testDonut(path, '\'evenodd\'');
path.reorient();
testDonut(path, '\'evenodd\' + reorient()');
path.fillRule = 'nonzero';
testDonut(path, '\'nonzero\' + reorient()');
});
test('Shape#contains()', function() {

View file

@ -350,7 +350,7 @@ test('#870', function() {
});
test('#875', function() {
var cp = new Path({
var p1 = new Path({
segments: [
[158.7, 389.3, 0, 0, -4.95, 4.95],
[158.7, 407.2, -4.95, -4.95, 4.95, 4.95],
@ -360,26 +360,29 @@ test('#875', function() {
],
closed: true
});
var p = new Path.Circle(260, 320, 100);
var p2 = new Path.Circle(260, 320, 100);
compareBoolean(function() { return cp.subtract(p); },
compareBoolean(function() { return p1.subtract(p2); },
'M158.7,407.2c4.95,4.95 12.95,4.95 17.9,0c4.95,-4.95 4.95,-12.95 0,-17.9c-4.95,-4.95 -12.95,-4.95 -17.9,0c-4.95,4.95 -4.95,12.95 0,17.9z');
});
test('#877', function() {
var cp = new CompoundPath([
new Path.Circle(100, 60, 50),
new Path.Circle(100, 60, 30),
new Path({
segments: [
[120, 140],
[150, 140],
[150, 190],
[120, 190]
],
closed: true
})
]);
var cp = new CompoundPath({
children: [
new Path.Circle(100, 60, 50),
new Path.Circle(100, 60, 30),
new Path({
segments: [
[120, 140],
[150, 140],
[150, 190],
[120, 190]
],
closed: true
}),
],
fillRule: 'evenodd'
});
var p = new Path({
segments: [