feat(core): Added Path.combine and related changes, closes #5976

The discussion in #5976 is whether `Path.join()` should use a line
segment to close any gaps in the path caused by move operations, or by
differences in the end and start points of paths being joined.

The answer is yes, that is the intended behaviour, but people who read
_join_ might expect differently.

So I have made a few changes to clarify this:

- The new `Path.combine()` method combines multiple path instances into
  a single instance without making any changes to the drawing operations
- Since `Path.combine()` is variadic, I have also updated `Path.join()`
  to be variadic too, since that is more consistent.
- The old way of calling `Path.join(path, bool)` is deprecated and will
  log a warning. Calling `Path.join()` this way will be removed in v4.
- Related to this change is how `Path.length()` should behave when there
  are gaps in the path. Currently, it skips those. So I've added a
  parameter that when set to `true` will include them.
- Added documentation for `Path.combine()`
- Updated documentation for `Path.join()`
- Updated documentation for `Path.length()`

packages/core/src/path.mjs

@@ -266,6 +266,16 @@ Path.prototype.close = function () {
   return this
 }
 
+/**
+ * Combines one or more Paths into a single Path instance
+ *
+ * @param {array} paths - The paths to combine
+ * @return {Path} combo - The combined Path instance
+ */
+Path.prototype.combine = function (...paths) {
+  return __combinePaths(this, ...paths)
+}
+
 /**
  * Adds a curve operation via cp1 & cp2 to Point to
  *
@@ -512,28 +522,63 @@ Path.prototype.intersectsY = function (y) {
 /**
  * Joins this Path with that Path, and closes them if wanted
  *
- * @param {Path} that - The Path to join this Path with
- * @param {bool} closed - Whether or not to close the joint Path
+ * The legacy (prior to v3.2) form of this method too two parameters:
+ *   - The Path to join this path with
+ *   - A boolean expressing whether the joined path should be closed
+ * In retrospect, that was kind of a dumb idea, because if the path
+ * needs tobe closed, you can juse chain the .join() with a .close()
+ *
+ * So now, this method is variadic, and it will join as many paths as you want.
+ * However, we keep it backwards compatible, and raise a deprecation warning when used that way.
+ *
+ * @param {array} paths - The Paths to join this Path with
  * @return {Path} joint - The joint Path instance
  */
-Path.prototype.join = function (that, closed = false) {
-  if (that instanceof Path !== true)
+Path.prototype.join = function (...paths) {
+  if (paths.length < 1) {
     this.log.error('Called `Path.join(that)` but `that` is not a `Path` object')
-  return __joinPaths([this, that], closed)
+    return this
+  }
+
+  /*
+   * Check for legacy signature
+   */
+  if (paths.length === 2 && [true, false].includes(paths[1])) {
+    this.log.warn(
+      '`Path.join()` was called with the legacy signature passing a bool as second parameter. This is deprecated and will be removed in FreeSewing v4'
+    )
+    return paths[1] ? __joinPaths([this, paths[0]]).close() : __joinPaths([this, paths[0]])
+  }
+
+  /*
+   * New variadic approach
+   */
+  let i = 0
+  for (const path of paths) {
+    if (path instanceof Path !== true)
+      this.log.error(
+        `Called \`Path.join(paths)\` but the path with index \`${i}\` is not a \`Path\` object`
+      )
+    i++
+  }
+
+  return __joinPaths([this, ...paths])
 }
 
 /**
  * Return the length of this Path
  *
+ * @param {bool} withMoves - Include length of move operations inside the path
  * @return {float} length - The length of this path
  */
-Path.prototype.length = function () {
+Path.prototype.length = function (withMoves = false) {
   let current, start
   let length = 0
   for (let i in this.ops) {
     let op = this.ops[i]
     if (op.type === 'move') {
-      start = op.to
+      if (typeof start === 'undefined') start = op.to
+      else if (withMoves) length += current.dist(op.to)
     } else if (op.type === 'line') {
       length += current.dist(op.to)
     } else if (op.type === 'curve') {
@@ -861,8 +906,8 @@ Path.prototype.split = function (point) {
     }
   }
 
-  if (firstHalf.length > 0) firstHalf = __joinPaths(firstHalf, false)
-  if (secondHalf.length > 0) secondHalf = __joinPaths(secondHalf, false)
+  if (firstHalf.length > 0) firstHalf = __joinPaths(firstHalf)
+  if (secondHalf.length > 0) secondHalf = __joinPaths(secondHalf)
 
   return [firstHalf, secondHalf]
 }
@@ -946,9 +991,9 @@ Path.prototype.trim = function () {
           first = false
         }
         let joint
-        if (trimmedStart.length > 0) joint = __joinPaths(trimmedStart, false).join(glue)
+        if (trimmedStart.length > 0) joint = __joinPaths(trimmedStart).join(glue)
         else joint = glue
-        if (trimmedEnd.length > 0) joint = joint.join(__joinPaths(trimmedEnd, false))
+        if (trimmedEnd.length > 0) joint = joint.join(__joinPaths(trimmedEnd))
 
         return joint.trim()
       }
@@ -1187,6 +1232,21 @@ function __bbbbox(boxes) {
   return { topLeft: new Point(minX, minY), bottomRight: new Point(maxX, maxY) }
 }
 
+/**
+ * Combines path segments into a single path instance
+ *
+ * @private
+ * @param {Array} paths - An Array of Path objects
+ * @return {object} path - A Path instance
+ */
+function __combinePaths(...paths) {
+  const joint = new Path().__withLog(paths[0].log)
+  const ops = []
+  for (const path of paths) joint.ops.push(...path.ops)
+
+  return joint
+}
+
 /**
  * Returns an object holding topLeft and bottomRight Points of the bounding box of a curve
  *
@@ -1208,10 +1268,9 @@ function __curveBoundingBox(curve) {
  *
  * @private
  * @param {Array} paths - An Array of Path objects
- * @param {bool} closed - Whether or not to close the joined paths
  * @return {object} path - A Path instance
  */
-function __joinPaths(paths, closed = false) {
+function __joinPaths(paths) {
   let joint = new Path().__withLog(paths[0].log).move(paths[0].ops[0].to)
   let current
   for (let p of paths) {
@@ -1231,7 +1290,6 @@ function __joinPaths(paths, closed = false) {
       if (op.to) current = op.to
     }
   }
-  if (closed) joint.close()
 
   return joint
 }
@@ -1335,7 +1393,7 @@ function __pathOffset(path, distance) {
     if (!start) start = current
   }
 
-  return __joinPaths(offset, closed)
+  return closed ? __joinPaths(offset).close() : __joinPaths(offset)
 }
 
 /**

packages/core/tests/path.test.mjs

@@ -197,6 +197,7 @@ describe('Path', () => {
       .curve(new Point(0, 40), new Point(123, 34), new Point(230, 4))
     const joint = curve.join(line)
     expect(joint.ops.length).to.equal(4)
+    expect(joint.ops[2].type).to.equal('line')
   })
 
   it('Should join paths that have noop operations', () => {
@@ -209,7 +210,7 @@ describe('Path', () => {
     expect(joint.ops.length).to.equal(6)
   })
 
-  it('Should throw error when joining a closed paths', () => {
+  it('Should throw error when joining a closed path', () => {
     const line = new Path().move(new Point(0, 0)).line(new Point(0, 40))
     const curve = new Path()
       .move(new Point(123, 456))
@@ -218,6 +219,16 @@ describe('Path', () => {
     expect(() => curve.join(line)).to.throw()
   })
 
+  it('Should combine paths', () => {
+    const line = new Path().move(new Point(0, 0)).line(new Point(0, 40))
+    const curve = new Path()
+      .move(new Point(123, 456))
+      .curve(new Point(0, 40), new Point(123, 34), new Point(230, 4))
+    const combo = curve.combine(line)
+    expect(combo.ops.length).to.equal(4)
+    expect(combo.ops[2].type).to.equal('move')
+  })
+
   it('Should shift along a line', () => {
     const line = new Path().move(new Point(0, 0)).line(new Point(0, 40))
     expect(line.shiftAlong(20).y).to.equal(20)

packages/core/tests/pattern-draft.test.mjs

@@ -155,7 +155,6 @@ describe('Pattern', () => {
     const pattern = new Test()
     pattern.draft()
 
-    console.log(pattern.setStores[pattern.activeSet].logs.error[0])
     expect(pattern.setStores[pattern.activeSet].logs.error[0]).to.include(
       'Could not inject part `otherPart` into part `front`'
     )