SVGLoader: Implement custom createShapes() method.

[Ttommeke]

Related issue: Fixed #16950.

Description

The current implementation of ShapePath only allowed the toShapes() function to generate shapes with holes correctly if the path orientation(CW or CCW) was known before hand. This made it difficult to import and convert a user generated SVG via the SVGLoader into a Shape object.

This implementation adds a scanline that will automatically determine the outside of a shape and its orientation. By passing the fillRule argument (same as the SVG style fill-rule), it can proceed to generate a correct shape.

Some issues that need to be addressed:

• This solution can't yet transform self intersecting paths correctly like the star shape in the documentation. WindingRule
• It breaks the old implementation because It got rid of the isCCW and noHoles parameter and replaced them with fillRule.
• I have not written unit tests... Am I going to burn on the stake now?

[Ttommeke]

I see that i still have to do some linting.

[Mugen87]

It breaks the old implementation because It got rid of the isCCW and noHoles parameter and replaced them with fillRule.

I don't think this PR can be accepted then. A lot of user level code will going to break.

Would it be possible to move this implementation to SVGLoader so the loader does not rely on ShapePath anymore?

[Mugen87]

Please remove all changes to package.json and package-lock.json.

[Ttommeke]

Done! I however have no idea why he flags a change in the last rule of package-lock.json.

[Ttommeke]

It breaks the old implementation because It got rid of the isCCW and noHoles parameter and replaced them with fillRule.

I don't think this PR can be accepted then. A lot of user level code will going to break.

Would it be possible to move this implementation to SVGLoader so the loader does not rely on ShapePath anymore?

Thanks for the feedback.
I understand. I'm not sure if it is possible to remove ShapePath from the implementation, but i could create a createShape(ShapePath) on SVGLoader instead.

[Mugen87]

but i could create a createShape(ShapePath) on SVGLoader instead.

That sounds like a plan 👍 .

[mrdoob]

but i could create a createShape(ShapePath) on SVGLoader instead.

That sounds like a plan 👍 .

Yep, sounds much more safe 👍

[yomboprime]

Hi, I've done some tests. I hate to bring bad news, here are some results. I will look with more insight in some days or when you make more commits.

For the tests I've made the following:

// const shapes = path.toShapes( true );
const shapes = SVGLoader.createShapes( path );

Results:

• The tiger gives error 'baseIntersections[0] is undefined'.
• The file 'Units' shows some letter holes which are obscured:

(BTW, you forgot to modify the js and not the jsm)

[yomboprime]

With the following changes the tiger shows up. I've not managed to make the text holes look correct.

Added this if before this line:

if ( baseIntersections.length === 0 ) return { identifier: simplePath.identifier, isHole: false, for: null };

const firstXOfPath = baseIntersections[ 0 ].point.x;

And added this check in this if:

if ( ! amIAHole.isHole && p.points.length > 0 ) {

	const shape = new Shape( p.points );

[Ttommeke]

@yomboprime That is a good fix. I'll sort out the text holes. Shouldn't be that difficult.

[Ttommeke]

(BTW, you forgot to modify the js and not the jsm)

I feel a bit stupid for asking, but i'm not sure what i have to do now. Do i have to edit both? Or only one?

[Mugen87]

You always edit files in exampels/js and then use node utils/modualrize.js to generate the ES6 module file.

[yomboprime]

BTW, I think all the polarAngle calculations can be removed, since the theta angle is not used anywhere?

[Ttommeke]

• That angle is indeed unnecessary. I Removed it.
• The 0.0000001 is replaced with Number.Epsilon.
• I removed beforehand any empthy paths to fix the baseintersection[0] is undefined.
• The undetected holes where caused by intersections through points. I fixed the issue by merging intersections with a t value within a margin of Number.EPSILON.

[yomboprime]

Please, could you add this SVG file to the example in test/testDefs/? (renaming to .svg)
defs5.txt

Closes #16950

[Ttommeke]

Sorry for the wait.

@yomboprime That looks great. I've added to the code. I've a question out of curiosity. Why did you choose to not return the classification of an intersection and instead use a variable(classifyResult) that you overwrite every time. Is it faster that way?

I believe to have fixed all of the issues @Mugen87 found in my pull request.

[yomboprime]

I've a question out of curiosity. Why did you choose to not return the classification of an intersection and instead use a variable(classifyResult) that you overwrite every time. Is it faster that way?

Yes, should be faster as the JS engine doesn't have to create an additional object to return it and then just forget it after use. The V8 probably does a good job with such return-and-forget objects but it still should be faster. Since the intersection call is deeply nested in paths and segments loops, the optimization is multiplied potentially thousands of times.

[Mugen87]

https://raw.githack.com/Ttommeke/three.js/issue/ShapePath-toShapes-scanlineMethod/examples/webgl_loader_svg.html

[Mugen87]

@yomboprime Do you know why the test files Defs and Defs2 are not working?

[Mugen87]

@Ttommeke Do you mind changing this line in the editor, too?

three.js/editor/js/Loader.js

Line 495 in 5635d4b

var shapes = path.toShapes( true );

Besides, please update the code examples in the documentation and also add the new method:

https://threejs.org/docs/index.html#examples/en/loaders/SVGLoader
https://threejs.org/docs/index.html#examples/zh/loaders/SVGLoader

[yomboprime]

@yomboprime Do you know why the test files Defs and Defs2 are not working?

I noticed it some time ago and was going to look at it when I have some spare time.

[Mugen87]

Okay. Just wanted to make sure this is unrelated to the PR 👍

[Mugen87]

@yomboprime Found the issue, see 330b186#r48264467.

[Ttommeke]

I was writing the documentation when i realized that most other loaders do not define static methods. They also all implement a parse method. For the sake of keeping things organized and the same, would it be a good idea to refactor the function createShapes(as a static method) to a parse(non static) method?

[yomboprime]

I was writing the documentation when i realized that most other loaders do not define static methods. They also all implement a parse method. For the sake of keeping things organized and the same, would it be a good idea to refactor the function createShapes(as a static method) to a parse(non static) method?

I don't understand... SVGLoader already has a parse method.

Also, the utility functions like pointsToStroke and createShapes are OK as static because the user can use them at their convenience.

[Ttommeke]

You are right. I was wrong. I shouldn't be thinking about refactoring code at 1 am.

[mrdoob]

Is this ready to merge then?

[Mugen87]

It seems #21380 (comment) is still a todo.

[yomboprime]

Is this ready to merge then?

I think yes. But Tom was doing documentation, I understood.

[Ttommeke]

Yes. I'm finishing up now.

[Ttommeke]

Believe it's ready.

[mrdoob]

Alright!

[mrdoob]

Thanks! 🙏

[Ttommeke]

It was a pleasure! Thanks for the great experience! @yomboprime @Mugen87 @mrdoob

First public PR ever.