Fix closed paths undercounting length (getPointAt / beginning / ending)

[Xuepoo]

Fixes #835.

_updateLength in src/path.js hardcoded closed = false, with the real check left commented out:

const closed = false; //this._closed || this.vertices[last]._command === Commands.close;

so the closing segment (last vertex → first vertex) of a closed path was never measured. Path#length undercounts the true perimeter, and both getPointAt(t) and beginning/ending trim rendering derive from it, so they skip roughly the last edge's worth of the shape.

Why this isn't just uncommenting that line

Restoring the closed check alone regresses: the zero-length guard a few lines down,

if ((i <= 0 && !closed) || a.command === Commands.move) {

unconditionally zeroes the wrap segment whenever the vertex's command is Commands.move — and vertex 0 of every path, open or closed, carries Commands.move. So the guard needs to know it's looking at vertex 0 of a closed path specifically, not just "any Commands.move vertex":

if (
  (i <= 0 && !closed) ||
  (a.command === Commands.move && !(i === 0 && closed))
) {

I also added a length > 0 guard on the closed detection itself, since Two.Points can call _updateLength with zero vertices (an empty Collection), and this.vertices[last] on an empty array is undefined — a bare comment-revert throws there.

Test

New QUnit test 'Two.Path closed length / getPointAt' in tests/suite/core.js:

• a closed 100×100 Two.Rectangle reports length === 400 (was 300)
• getPointAt(0.75) lands on the midpoint of the closing edge, not a point on an earlier edge
• an open Two.Line control confirms the fix doesn't touch the already-correct open-path case

Confirmed failing on dev before this change (length: 300, wrong point) and passing after.

Verification

Ran the full QUnit suite in a real headless Chrome (tests/index.html, not just Node): 157 tests, 742/747 assertions pass both before and after this change — the same 5 pre-existing failures (2× makeImage mode-switching timing, 1× getBoundingClientRect sub-pixel diff) are present identically before and after, unrelated to this change, no regressions.

[Copilot]

Pull request overview

This PR addresses incorrect perimeter calculations for closed Two.Path instances by updating how _updateLength determines closure and how it treats the first move vertex when accumulating segment lengths. Since Path#length is used by getPointAt(t) and beginning/ending trimming, the change is intended to prevent closed shapes from “missing” their closing segment.

Changes:

• Update src/path.js _updateLength to detect closed paths (with an empty-vertices guard) and to avoid zeroing the closing segment when vertex 0 is a Commands.move on a closed path.
• Add a QUnit regression test covering closed-path length and getPointAt, plus an open-path control case.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/path.js	Modifies closed-path detection and segment-length accumulation in _updateLength.
tests/suite/core.js	Adds a regression test for closed-path length and getPointAt behavior.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[jonobr1]

Suggested PR summary

The root issue is valid: closed paths currently omit their closing segment from length, getPointAt, and trim calculations. However, the proposed patch stores that segment in _lengths[0], breaking the indexing assumptions used by getPointAt, getIdByLength, beginning, and ending. This causes reversed traversal, repeated edges, and jumps at segment boundaries. Closure also needs consistent handling for compound paths, explicit Z commands, cache invalidation, and partially trimmed rendering. I recommend keeping the issue open but revising the implementation before merging.

Implementation plan

1. Define traversal invariants

   Establish these expectations before changing code:

   • Open paths start at vertex zero and end at the final vertex.
   • Closed paths satisfy getPointAt(0) === getPointAt(1).
   • Increasing t, beginning, or ending moves continuously in drawing order.
   • Exact segment boundaries do not jump when sampled at boundary ± epsilon.
   • Closure targets the latest M, not necessarily global vertex zero.
   • _updateLength, getPointAt, trimming, and renderers use the same closure semantics.

2. Add failing regression tests first

   Cover:

   • A closed rectangle at t = 0, .125, .25, .5, .75, .875, 1.
   • Samples immediately before and after each quarter boundary.
   • ending increasing through those same boundaries.
   • beginning increasing with ending = 1.
   • Closed curved shapes such as an ellipse.
   • Open paths as compatibility controls.
   • Explicit Z commands.
   • Compound paths containing multiple M commands.
   • Changing path.closed after length has already been read.
   • Empty, single-vertex, and vertex-removal cases.
   • Two.Points, since it borrows _updateLength.

3. Introduce one internal segment representation

   Build an ordered list of drawable segments, with each entry identifying:

   {
     from,
     to,
     length,
     closesSubpath
   }

   This avoids encoding two different concepts—vertices and segments—in _lengths indexes. _lengths can remain as a compatibility cache if needed, but traversal should use the segment list as its source of truth.

4. Make _updateLength build that segment list

   It should:

   • Reset or truncate old cached values.
   • Track the current subpath start whenever it encounters M.
   • Add ordinary line/curve segments in drawing order.
   • Add closure to the current subpath start for Z or applicable closed.
   • Sum segment lengths into _length.
   • Handle empty and zero-length segments safely.

5. Update consumers together

   Change getPointAt to walk ordered segments directly instead of reconstructing endpoints from _lengths[i].

   Update getIdByLength, contains, and trimming to use the same segment traversal. Exact boundaries should consistently choose either the ending segment or following segment without changing the returned point.

6. Fix partial-path rendering

   When beginning or ending trims a closed path, the resulting temporary renderer path should generally be open. Otherwise Canvas/SVG implicitly close the partial geometry with a diagonal chord.

   Only preserve implicit closure when the complete closed path is being rendered.

7. Fix cache invalidation

   The closed setter should set both:

   this._flagVertices = true;
   this._flagLength = true;

   Ensure command changes, insertions, removals, and automatic plotting similarly invalidate segment and length caches.

8. Verify behavior across renderers

   Run the full QUnit suite and add renderer-specific checks:

   • SVG partial paths should not end in Z.
   • Canvas partial paths should not call closePath().
   • Full closed paths should still close normally.
   • Existing open-path animation output should remain unchanged.

9. Update the PR description

   Explicitly call out the intentional behavior change: closed-path length becomes the true perimeter, so normalized positions may shift for callers that relied on the old undercount. Emphasize that traversal remains continuous and renderer-consistent.

[Xuepoo]

Thanks for the detailed review — you're right, the original patch had exactly the bugs you described. Pushed a redesign.

Root cause of the traversal bug

The original patch stored the closing edge's length in _lengths[0], overwriting the first real segment's slot. getPointAt's linear scan walks _lengths in index order, so the closing edge got visited first instead of last — reversed traversal, and (since it also collided with _lengths[0]'s original meaning) the first real segment's length was lost entirely. Storing it at global vertex 0 also broke compound paths, since only a path's final subpath is implicitly closed (earlier subpaths get their own explicit Commands.close vertex from the SVG parser).

Redesign

• _updateLength now appends the closing edge as one entry past the real vertices (_lengths[length]) instead of overwriting index 0, so it's scanned in true drawing order (last). Closure targets the start of the current subpath (most recent Commands.move), not global vertex 0.
• getPointAt resolves that appended index to (subpath start -> last vertex) in real drawing order.
• The main per-vertex loop in _update() can never synthesize a point past the last real vertex, so a trim ending landing inside the appended closing edge was silently dropped — added an explicit branch for that case.
• _update() now computes this._renderer.closed = closed && beginning === 0 && ending === 1. A trimmed closed path renders an open sub-arc, so canvas/webgl/svg must not implicitly close it with a chord back to the start. Wired all three renderers to read _renderer.closed instead of the raw _closed.
• The closed setter now also sets _flagLength = true (was missing this even before Closed paths undercount length, breaking getPointAt / beginning / ending #835).

Verification against your test plan

• getPointAt(0) === getPointAt(1) for closed shapes (rectangle + ellipse). ✓

• Quarter-boundary samples (t = 0, .25, .5, .75, 1) land at the correct corners in drawing order — the first patch landed one edge off (reversed). ✓

• No jump sampling t = 0.25 ± 1e-6. ✓

• Compound path (2 subpaths: one with an explicit Z vertex, one implicitly closed) — each closes to its own subpath start; length = sum of both perimeters; getPointAt doesn't bleed from subpath 1 into subpath 2. ✓

• Toggling closed after length was already read re-invalidates the cache. ✓

• Empty/single-vertex paths (Two.Points reuses _updateLength) don't crash. ✓

• A trimmed closed path (ending < 1) no longer implicitly closes — pixel-verified in real headless Chrome:

  Left to right: unpatched dev (undercounts length, ending=0.5 renders only a partial first edge), the original patch from this PR (traversal bug — getPointAt(0.5) lands at the top-right corner, 25% around, instead of the correct bottom-right corner at 50%), and this redesign (correct).

• Full QUnit suite in real headless Chrome: 157/157 tests run, 757/762 assertions pass, same 5 pre-existing unrelated failures before and after (2x makeImage mode-switching timing, 1x getBoundingClientRect sub-pixel diff — all headless-environment-specific, unrelated to this change). No regressions.

The regression test ('Two.Path closed length / getPointAt' in tests/suite/core.js) grew from 4 to 19 assertions covering the above, including the compound-path and cache-invalidation cases.

I scoped this to what your review flagged directly reachable from this bug (traversal order, compound-path closure targets, renderer partial-close, cache invalidation). I did not introduce the full segment-list abstraction from step 3 of your plan ({from, to, length, closesSubpath}) — the appended-slot approach keeps _lengths' existing index meaning for every real segment unchanged (so getIdByLength/contains, which read _lengths directly, needed no changes at all), and passes every scenario in your test plan without it. Happy to go further if you'd still prefer the segment-list model as the long-term representation — let me know and I can follow up with that as a separate, larger change.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (3)

tests/suite/core.js:1524

• These test comments reference “maintainer feedback item 6”, which isn’t available in-repo and makes the test less self-explanatory. Consider rephrasing to describe the behavior under test without relying on an external numbered list.

  // Trimming (`ending`) a closed path must not implicitly close the
  // partial render with a straight chord back to the start (maintainer
  // feedback item 6).

src/path.js:1501

• When trimming a closed path whose implicit closing edge is a curve (e.g., last vertex is Commands.curve), the newly appended trimmed point is forced to Commands.line. This makes the rendered trimmed segment a straight line even though _updateLength / getPointAt treat the closing edge as a cubic curve, producing incorrect geometry near ending in the closing segment. Preserve the segment type (curve vs line) and project the last vertex’s right handle the same way the existing i > high trimming branch does.

      if (!right && ending < 1 && this._needsImplicitClose && high === l - 1) {
        v = new Anchor();
        this.getPointAt(ending, v);
        v.command = Commands.line;
        this._renderer.vertices.push(v);

tests/suite/core.js:1452

• These test comments reference “maintainer feedback item 7”, which isn’t available in-repo and makes the test less self-explanatory. Consider rephrasing to describe the behavior under test without relying on an external numbered list.

This issue also appears on line 1522 of the same file.

  // Changing `closed` after `length` has already been read invalidates
  // the cache (maintainer feedback item 7).

[jonobr1]

Thanks for the contributions! Here's an outline of additional improvements your PR landed: https://codepen.io/editor/jonobr1/pen/01a04bef-078d-7739-8377-a20b0af1af92