ShapePath: Fix unlinked holes when path has hole on first AND last

[neofuji]

Related issue: none

Description

When holes on first AND last in the path, ShapePath.toShapes() incorrectly ignores the holes.
For example, the letter 激 in typeface.json has 5 holes, 1 shape, and 2 holes. The last 2 holes are incorrectly ignored.

Expected behavior

Actual behavior

typeface.json

{
  "glyphs": {
    "激": {
      "ha": 1389,
      "x_min": 5,
      "x_max": 1389,
      "o": "m 802 545 l 751 545 l 751 530 l 817 536 l 802 545 m 1104 519 l 1116 696 l 1072 696 l 1051 629 l 1088 637 l 1104 519 m 449 545 l 358 545 l 351 536 l 449 530 l 449 545 m 595 746 l 595 727 l 628 727 l 628 746 l 595 746 m 595 872 l 628 872 l 628 891 l 595 891 l 595 872 m 286 410 l 8 521 l 153 818 l 350 635 l 350 897 l 267 774 l 5 903 l 175 1185 l 350 1005 l 350 1078 l 434 1078 l 439 1184 l 773 1175 l 754 1078 l 852 1078 l 852 935 l 898 1183 l 1222 1154 l 1179 1019 l 1388 1024 l 1388 693 l 1355 696 l 1314 220 l 1389 199 l 1359 -175 l 1107 -49 l 864 -175 l 850 -62 l 844 -186 l 589 -179 l 581 -95 l 570 -182 l 328 -75 l 311 -180 l 5 -77 l 172 346 l 392 294 l 373 184 l 412 319 l 323 313 l 323 487 l 286 410 m 873 313 l 646 319 l 640 300 l 865 300 l 863 198 l 939 228 l 884 484 l 873 492 l 873 313 m 623 60 l 629 99 l 612 99 l 605 62 l 623 60 z "
    }
  },
  "familyName": "GN-KMBFont-UB-NewstyleKanaB",
  "ascender": 1194,
  "descender": -475,
  "underlinePosition": -130,
  "underlineThickness": 65,
  "boundingBox": {
    "yMin": -450,
    "xMin": -142,
    "yMax": 1298,
    "xMax": 1679
  },
  "resolution": 1000,
  "original_font_information": {
    "format": 0,
    "copyright": "Nagoriyuki/Getsuren",
    "fontFamily": "GN-KMBFont-UB-NewstyleKanaB",
    "fontSubfamily": "Ultra",
    "uniqueID": "GN-KMBFont-UB-NewstyleKanaB",
    "fullName": "GN-KMBFont-UB-NewstyleKanaB",
    "version": "Version 0.99b",
    "postScriptName": "GN-KMBFont-UB-NewstyleKanaB",
    "trademark": "GN-KMBFont-UB-NewstyleKanaB"
  },
  "cssFontWeight": "normal",
  "cssFontStyle": "normal"
}

[LeviPesin]

Why not just newShapeHoles[ newShapeHoles.length - 1 ].push( ...restShapeHoles )?

[mrdoob]

I find both ways hard to read 😅

[Mugen87]

Do you mind explaining in more detail how the error occurs? TBH, the implementation of ShapePath.toShapes() is borderline. Before adding more complexity it's important to understand why the code fails. And if the patch can be applied elsewhere.

[neofuji]

ShapePath.toShapes() has 3 steps to build shapes.

1. Divides this.subPaths into newShapes and newShapeHoles.
2. Moves the items in newShapeHoles if newShapeHoles[i][j] is outside of newShapes[i].
3. Apply newShapeHoles[i] to newShapes[i].

This problem occurs in the step 1.
Step 1 divides this.subPaths to a pair of array, newShapes: [shape#0, shape#1, ...] and newShapeHoles: [holes in shape#0, holes in shape#1, ...].
However, when holes on first AND last, the length of newShapeHoles is incorrectly more than newShapes.
Step 2 and 3 assumes the length of the arrays is same. If not same, the last holes are ignored incorrectly.

Shapes first, holes last
Input: [s0, h0, h1, s1, h2]
Output newShapes: [s0, s1]
Output newShapeHoles: [[h0, h1], [h2]]

Holes first, shapes last
Input: [h0, h1, s0, h2, s1]
Output newShapes: [s0, s1]
Output newShapeHoles: [[h0, h1], [h2]]

Holes first, HOLES last
Input: [h0, h1, s0, h2, s1, h3]
Output newShapes: [s0, s1]
Output newShapeHoles: [[h0, h1], [h2], [h3]] <- [h3] has no matching newShapes!

If step 1 is fixed to divide all subPaths correctly, step 2 and step 3 works correctly with no fix.
This PR fixes the step 1.

[Mugen87]

I think I remember now. We've discussed a similar problem in #16950.

Instead of using ShapePath.toShapes(), do you mind trying the static SVGLoader.createShapes() method? It would be interesting to know if this implementation fixes your issue.

I know it's odd importing a separate loader for this task but this is just for testing. If you need a code example, SVGLoader.createShapes() is used here:

three.js/examples/webgl_loader_svg.html

Line 182 in c03a6e8

const shapes = SVGLoader.createShapes( path );

[neofuji]

@Mugen87 SVGLoader.createShapes() works well but ShapePath.userData.style.fillRule must be set.

I tested by patching Font.prototype.generateShapes().

three.js/examples/jsm/loaders/FontLoader.js

Line 75 in cf82659

Array.prototype.push.apply( shapes, paths[ p ].toShapes() );

			// Array.prototype.push.apply( shapes, paths[ p ].toShapes() );
			paths[ p ].userData ??= {};
			paths[ p ].userData.style ??= {};
			paths[ p ].userData.style.fillRule ??= "nonzero";
			Array.prototype.push.apply( shapes, SVGLoader.createShapes( paths[ p ] ) );

[Mugen87]

SVGLoader.createShapes() works well but ShapePath.userData.style.fillRule must be set.

Fixed via #24293.

[Mugen87]

Closing. The workaround for this issue is the usage of SVGLoader.createShapes().

In the long term, the implementation of ShapePath.toShapes() will be replaced, see #24560 (comment).