From 12a393505d482107d55e7cf98203f17cafaa601d Mon Sep 17 00:00:00 2001 From: Flint O'Brien Date: Sun, 24 Apr 2016 16:43:20 -0400 Subject: [PATCH] Refactoring and performance improvements for getStrokedBBox. canvas.getStrokedBBox internals refactored to svgutils. getStrokedBBox/getCheckedBBox renamed to svgedit.utilities.getBBoxWithTransform Removed duplicate calls to native getBBox. Refactored slow transformed BBox from temporary DOM append/remove to matrix calculations. Lots of tests. Added qunit/qunit-assert-close.js. --- editor/svgcanvas.js | 135 +------- editor/svgutils.js | 165 ++++++++- test/all_tests.html | 1 + test/qunit/qunit-assert-close.js | 106 ++++++ test/svgutils_bbox_test.html | 511 ++++++++++++++++++++++++++++ test/svgutils_performance_test.html | 247 ++++++++++++++ 6 files changed, 1022 insertions(+), 143 deletions(-) create mode 100644 test/qunit/qunit-assert-close.js create mode 100644 test/svgutils_bbox_test.html create mode 100644 test/svgutils_performance_test.html diff --git a/editor/svgcanvas.js b/editor/svgcanvas.js index a7c61afc..1511bc30 100644 --- a/editor/svgcanvas.js +++ b/editor/svgcanvas.js @@ -601,140 +601,9 @@ var getIntersectionList = this.getIntersectionList = function(rect) { // // Returns: // A single bounding box object -getStrokedBBox = this.getStrokedBBox = function(elems) { +var getStrokedBBox = this.getStrokedBBox = function(elems) { if (!elems) {elems = getVisibleElements();} - if (!elems.length) {return false;} - // Make sure the expected BBox is returned if the element is a group - var getCheckedBBox = function(elem) { - // TODO: Fix issue with rotated groups. Currently they work - // fine in FF, but not in other browsers (same problem mentioned - // in Issue 339 comment #2). - - var bb = svgedit.utilities.getBBox(elem); - if (!bb) { - return null; - } - var angle = svgedit.utilities.getRotationAngle(elem); - - if ((angle && angle % 90) || - svgedit.math.hasMatrixTransform(svgedit.transformlist.getTransformList(elem))) { - // Accurate way to get BBox of rotated element in Firefox: - // Put element in group and get its BBox - var good_bb = false; - // Get the BBox from the raw path for these elements - var elemNames = ['ellipse', 'path', 'line', 'polyline', 'polygon']; - if (elemNames.indexOf(elem.tagName) >= 0) { - bb = good_bb = canvas.convertToPath(elem, true); - } else if (elem.tagName == 'rect') { - // Look for radius - var rx = elem.getAttribute('rx'); - var ry = elem.getAttribute('ry'); - if (rx || ry) { - bb = good_bb = canvas.convertToPath(elem, true); - } - } - - if (!good_bb) { - // Must use clone else FF freaks out - var clone = elem.cloneNode(true); - var g = document.createElementNS(NS.SVG, 'g'); - var parent = elem.parentNode; - parent.appendChild(g); - g.appendChild(clone); - bb = svgedit.utilities.bboxToObj(g.getBBox()); - parent.removeChild(g); - } - - // Old method: Works by giving the rotated BBox, - // this is (unfortunately) what Opera and Safari do - // natively when getting the BBox of the parent group -// var angle = angle * Math.PI / 180.0; -// var rminx = Number.MAX_VALUE, rminy = Number.MAX_VALUE, -// rmaxx = Number.MIN_VALUE, rmaxy = Number.MIN_VALUE; -// var cx = round(bb.x + bb.width/2), -// cy = round(bb.y + bb.height/2); -// var pts = [ [bb.x - cx, bb.y - cy], -// [bb.x + bb.width - cx, bb.y - cy], -// [bb.x + bb.width - cx, bb.y + bb.height - cy], -// [bb.x - cx, bb.y + bb.height - cy] ]; -// var j = 4; -// while (j--) { -// var x = pts[j][0], -// y = pts[j][1], -// r = Math.sqrt( x*x + y*y ); -// var theta = Math.atan2(y,x) + angle; -// x = round(r * Math.cos(theta) + cx); -// y = round(r * Math.sin(theta) + cy); -// -// // now set the bbox for the shape after it's been rotated -// if (x < rminx) rminx = x; -// if (y < rminy) rminy = y; -// if (x > rmaxx) rmaxx = x; -// if (y > rmaxy) rmaxy = y; -// } -// -// bb.x = rminx; -// bb.y = rminy; -// bb.width = rmaxx - rminx; -// bb.height = rmaxy - rminy; - } - return bb; - }; - - var full_bb; - $.each(elems, function() { - if (full_bb) {return;} - if (!this.parentNode) {return;} - full_bb = getCheckedBBox(this); - }); - - // This shouldn't ever happen... - if (full_bb == null) {return null;} - - // full_bb doesn't include the stoke, so this does no good! -// if (elems.length == 1) return full_bb; - - var max_x = full_bb.x + full_bb.width; - var max_y = full_bb.y + full_bb.height; - var min_x = full_bb.x; - var min_y = full_bb.y; - - // FIXME: same re-creation problem with this function as getCheckedBBox() above - var getOffset = function(elem) { - var sw = elem.getAttribute('stroke-width'); - var offset = 0; - if (elem.getAttribute('stroke') != 'none' && !isNaN(sw)) { - offset += sw/2; - } - return offset; - }; - var bboxes = []; - $.each(elems, function(i, elem) { - var cur_bb = getCheckedBBox(elem); - if (cur_bb) { - var offset = getOffset(elem); - min_x = Math.min(min_x, cur_bb.x - offset); - min_y = Math.min(min_y, cur_bb.y - offset); - bboxes.push(cur_bb); - } - }); - - full_bb.x = min_x; - full_bb.y = min_y; - - $.each(elems, function(i, elem) { - var cur_bb = bboxes[i]; - // ensure that elem is really an element node - if (cur_bb && elem.nodeType == 1) { - var offset = getOffset(elem); - max_x = Math.max(max_x, cur_bb.x + cur_bb.width + offset); - max_y = Math.max(max_y, cur_bb.y + cur_bb.height + offset); - } - }); - - full_bb.width = max_x - min_x; - full_bb.height = max_y - min_y; - return full_bb; + return svgedit.utilities.getStrokedBBox( elems, addSvgElementFromJson, pathActions) }; // Function: getVisibleElements diff --git a/editor/svgutils.js b/editor/svgutils.js index 718609f5..31e0961c 100644 --- a/editor/svgutils.js +++ b/editor/svgutils.js @@ -767,6 +767,160 @@ svgedit.utilities.convertToPath = function(elem, attrs, addSvgElementFromJson, p }; +// Function: getBBoxWithTransform +// Get bounding box that includes any transforms. +// +// Parameters: +// elem - The DOM element to be converted +// addSvgElementFromJson - Function to add the path element to the current layer. See canvas.addSvgElementFromJson +// pathActions - If a transform exists, pathActions.resetOrientation() is used. See: canvas.pathActions. +// +// Returns: +// A single bounding box object +svgedit.utilities.getBBoxWithTransform = function(elem, addSvgElementFromJson, pathActions) { + // TODO: Fix issue with rotated groups. Currently they work + // fine in FF, but not in other browsers (same problem mentioned + // in Issue 339 comment #2). + + var bb = svgedit.utilities.getBBox(elem); + + if (!bb) { + return null; + } + + var tlist = svgedit.transformlist.getTransformList(elem) + var angle = svgedit.utilities.getRotationAngleFromTransformList(tlist); + + if (angle || svgedit.math.hasMatrixTransform(tlist)) { + + var good_bb = false; + // Get the BBox from the raw path for these elements + // TODO: why ellipse and not circle + var elemNames = ['ellipse', 'path', 'line', 'polyline', 'polygon']; + if (elemNames.indexOf(elem.tagName) >= 0) { + bb = good_bb = svgedit.utilities.getBBoxOfElementAsPath(elem, addSvgElementFromJson, pathActions); + } else if (elem.tagName == 'rect') { + // Look for radius + var rx = elem.getAttribute('rx'); + var ry = elem.getAttribute('ry'); + if (rx || ry) { + bb = good_bb = svgedit.utilities.getBBoxOfElementAsPath(elem, addSvgElementFromJson, pathActions); + } + } + + if (!good_bb) { + + var matrix = svgedit.math.transformListToTransform( tlist).matrix; + bb = svgedit.math.transformBox(bb.x, bb.y, bb.width, bb.height, matrix).aabox; + + // Old technique that was exceedingly slow with large documents. + // + // Accurate way to get BBox of rotated element in Firefox: + // Put element in group and get its BBox + // + // Must use clone else FF freaks out + //var clone = elem.cloneNode(true); + //var g = document.createElementNS(NS.SVG, 'g'); + //var parent = elem.parentNode; + //parent.appendChild(g); + //g.appendChild(clone); + //var bb2 = svgedit.utilities.bboxToObj(g.getBBox()); + //parent.removeChild(g); + } + + } + return bb; +}; + +// TODO: This is problematic with large stroke-width and, for example, a single horizontal line. The calculated BBox extends way beyond left and right sides. +function getStrokeOffsetForBBox(elem) { + var sw = elem.getAttribute('stroke-width'); + return (!isNaN(sw) && elem.getAttribute('stroke') != 'none') ? sw/2 : 0; +}; + +// Function: getStrokedBBox +// Get the bounding box for one or more stroked and/or transformed elements +// +// Parameters: +// elems - Array with DOM elements to check +// addSvgElementFromJson - Function to add the path element to the current layer. See canvas.addSvgElementFromJson +// pathActions - If a transform exists, pathActions.resetOrientation() is used. See: canvas.pathActions. +// +// Returns: +// A single bounding box object +svgedit.utilities.getStrokedBBox = function(elems, addSvgElementFromJson, pathActions) { + if (!elems || !elems.length) {return false;} + + var full_bb; + $.each(elems, function() { + if (full_bb) {return;} + if (!this.parentNode) {return;} + full_bb = svgedit.utilities.getBBoxWithTransform(this, addSvgElementFromJson, pathActions); + }); + + // This shouldn't ever happen... + if (full_bb === undefined) {return null;} + + // full_bb doesn't include the stoke, so this does no good! + // if (elems.length == 1) return full_bb; + + var max_x = full_bb.x + full_bb.width; + var max_y = full_bb.y + full_bb.height; + var min_x = full_bb.x; + var min_y = full_bb.y; + + // If only one elem, don't call the potentially slow getBBoxWithTransform method again. + if( elems.length === 1) { + var offset = getStrokeOffsetForBBox(elems[0]); + min_x -= offset; + min_y -= offset; + max_x += offset; + max_y += offset; + } else { + $.each(elems, function(i, elem) { + var cur_bb = svgedit.utilities.getBBoxWithTransform(elem, addSvgElementFromJson, pathActions); + if (cur_bb) { + var offset = getStrokeOffsetForBBox(elem); + min_x = Math.min(min_x, cur_bb.x - offset); + min_y = Math.min(min_y, cur_bb.y - offset); + // TODO: The old code had this test for max, but not min. I suspect this test should be for both min and max + if (elem.nodeType == 1) { + max_x = Math.max(max_x, cur_bb.x + cur_bb.width + offset); + max_y = Math.max(max_y, cur_bb.y + cur_bb.height + offset); + } + } + }); + } + + full_bb.x = min_x; + full_bb.y = min_y; + full_bb.width = max_x - min_x; + full_bb.height = max_y - min_y; + return full_bb; +}; + + +// Function: svgedit.utilities.getRotationAngleFromTransformList +// Get the rotation angle of the given transform list. +// +// Parameters: +// tlist - List of transforms +// to_rad - Boolean that when true returns the value in radians rather than degrees +// +// Returns: +// Float with the angle in degrees or radians +svgedit.utilities.getRotationAngleFromTransformList = function(tlist, to_rad) { + if(!tlist) {return 0;} // elements have no tlist + var N = tlist.numberOfItems; + var i; + for (i = 0; i < N; ++i) { + var xform = tlist.getItem(i); + if (xform.type == 4) { + return to_rad ? xform.angle * Math.PI / 180.0 : xform.angle; + } + } + return 0.0; +}; // Function: svgedit.utilities.getRotationAngle // Get the rotation angle of the given/selected DOM element @@ -781,16 +935,7 @@ svgedit.utilities.getRotationAngle = function(elem, to_rad) { var selected = elem || editorContext_.getSelectedElements()[0]; // find the rotation transform (if any) and set it var tlist = svgedit.transformlist.getTransformList(selected); - if(!tlist) {return 0;} // elements have no tlist - var N = tlist.numberOfItems; - var i; - for (i = 0; i < N; ++i) { - var xform = tlist.getItem(i); - if (xform.type == 4) { - return to_rad ? xform.angle * Math.PI / 180.0 : xform.angle; - } - } - return 0.0; + return svgedit.utilities.getRotationAngleFromTransformList(tlist, to_rad) }; // Function getRefElem diff --git a/test/all_tests.html b/test/all_tests.html index c595e0e7..270a1a6e 100644 --- a/test/all_tests.html +++ b/test/all_tests.html @@ -12,6 +12,7 @@ + diff --git a/test/qunit/qunit-assert-close.js b/test/qunit/qunit-assert-close.js new file mode 100644 index 00000000..4b71e3df --- /dev/null +++ b/test/qunit/qunit-assert-close.js @@ -0,0 +1,106 @@ +/** + * Checks that the first two arguments are equal, or are numbers close enough to be considered equal + * based on a specified maximum allowable difference. + * + * @example assert.close(3.141, Math.PI, 0.001); + * + * @param Number actual + * @param Number expected + * @param Number maxDifference (the maximum inclusive difference allowed between the actual and expected numbers) + * @param String message (optional) + */ +function close(actual, expected, maxDifference, message) { + var actualDiff = (actual === expected) ? 0 : Math.abs(actual - expected), + result = actualDiff <= maxDifference; + message = message || (actual + " should be within " + maxDifference + " (inclusive) of " + expected + (result ? "" : ". Actual: " + actualDiff)); + QUnit.push(result, actual, expected, message); +} + + +/** + * Checks that the first two arguments are equal, or are numbers close enough to be considered equal + * based on a specified maximum allowable difference percentage. + * + * @example assert.close.percent(155, 150, 3.4); // Difference is ~3.33% + * + * @param Number actual + * @param Number expected + * @param Number maxPercentDifference (the maximum inclusive difference percentage allowed between the actual and expected numbers) + * @param String message (optional) + */ +close.percent = function closePercent(actual, expected, maxPercentDifference, message) { + var actualDiff, result; + if (actual === expected) { + actualDiff = 0; + result = actualDiff <= maxPercentDifference; + } + else if (actual !== 0 && expected !== 0 && expected !== Infinity && expected !== -Infinity) { + actualDiff = Math.abs(100 * (actual - expected) / expected); + result = actualDiff <= maxPercentDifference; + } + else { + // Dividing by zero (0)! Should return `false` unless the max percentage was `Infinity` + actualDiff = Infinity; + result = maxPercentDifference === Infinity; + } + message = message || (actual + " should be within " + maxPercentDifference + "% (inclusive) of " + expected + (result ? "" : ". Actual: " + actualDiff + "%")); + + QUnit.push(result, actual, expected, message); +}; + + +/** + * Checks that the first two arguments are numbers with differences greater than the specified + * minimum difference. + * + * @example assert.notClose(3.1, Math.PI, 0.001); + * + * @param Number actual + * @param Number expected + * @param Number minDifference (the minimum exclusive difference allowed between the actual and expected numbers) + * @param String message (optional) + */ +function notClose(actual, expected, minDifference, message) { + var actualDiff = Math.abs(actual - expected), + result = actualDiff > minDifference; + message = message || (actual + " should not be within " + minDifference + " (exclusive) of " + expected + (result ? "" : ". Actual: " + actualDiff)); + QUnit.push(result, actual, expected, message); +} + + +/** + * Checks that the first two arguments are numbers with differences greater than the specified + * minimum difference percentage. + * + * @example assert.notClose.percent(156, 150, 3.5); // Difference is 4.0% + * + * @param Number actual + * @param Number expected + * @param Number minPercentDifference (the minimum exclusive difference percentage allowed between the actual and expected numbers) + * @param String message (optional) + */ +notClose.percent = function notClosePercent(actual, expected, minPercentDifference, message) { + var actualDiff, result; + if (actual === expected) { + actualDiff = 0; + result = actualDiff > minPercentDifference; + } + else if (actual !== 0 && expected !== 0 && expected !== Infinity && expected !== -Infinity) { + actualDiff = Math.abs(100 * (actual - expected) / expected); + result = actualDiff > minPercentDifference; + } + else { + // Dividing by zero (0)! Should only return `true` if the min percentage was `Infinity` + actualDiff = Infinity; + result = minPercentDifference !== Infinity; + } + message = message || (actual + " should not be within " + minPercentDifference + "% (exclusive) of " + expected + (result ? "" : ". Actual: " + actualDiff + "%")); + + QUnit.push(result, actual, expected, message); +}; + + +//QUnit.extend(QUnit.assert, { +// close: close, +// notClose: notClose +//}); \ No newline at end of file diff --git a/test/svgutils_bbox_test.html b/test/svgutils_bbox_test.html new file mode 100644 index 00000000..ef12703c --- /dev/null +++ b/test/svgutils_bbox_test.html @@ -0,0 +1,511 @@ + + + + + Unit Tests for svgutils.js BBox functions + + + + + + + + + + + + + + + + +

Unit Tests for svgutils.js BBox functions

+

+

+
    +
    + + diff --git a/test/svgutils_performance_test.html b/test/svgutils_performance_test.html new file mode 100644 index 00000000..79c6eb8c --- /dev/null +++ b/test/svgutils_performance_test.html @@ -0,0 +1,247 @@ + + + + + + + Performance Unit Tests for svgutils.js + + + + + + + + + + + + + + + + +

    Performance Unit Tests for svgutils.js

    +

    +

    +
      + +
      +
      + + + + + + + + +
      + + + + + + + + + Layer 1 + + + + + + + + + + + + Some text + + + + Layer 2 + + + + +
      +
      +
      + + +