Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SVGLoader: Fix parsing of flags in compressed definitions. #21485

Merged
merged 2 commits into from
Mar 21, 2021

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Mar 19, 2021

Related issue: Fixed #20608.

Description

SVGLoader assumes sequences of floats like 7.51,7.51,0,0,1,1.311,3.879 are separated by specific values (signs, points, comma or whitespaces). The problem is that a mangled version like 7.51 7.51 0 011.311 3.879 is impossible to parse without knowing the semantics of the respective command.

For example 011.311 is actually 0,1,1.311. However, without knowing that the first and second numbers in that string are SVG flags, you will end up with 0,11.311 (because you can't detect whether the 1 is part of the subsequent number or not).

With this PR parseFloats() is enhanced with two parameters that allow the caller to specify optional semantics about the given definition. An additional array defines the indices of flags (meaning where flags occur in a definition) and the total length of a single definition. This should fix the tooth SVG (which is now a test asset of webgl_loader_svg) and the SVG from https://discourse.threejs.org/t/svgloader-parse-errors/24537.

@karimbeyrouti Do you mind testing this PR?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Mar 19, 2021

@Mugen87 Mugen87 requested a review from yomboprime March 19, 2021 13:32
@mrdoob mrdoob added this to the r127 milestone Mar 19, 2021
@yomboprime
Copy link
Collaborator

yomboprime commented Mar 19, 2021

I've noticed that the parse time now is much bigger (the recent createShapes PR did had much lower impact) Sorry, this was my fault.

The last example, "Zero Radius" doesn't show up in the githack link.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Mar 20, 2021

Should be fixed now. I've missed to update the parser's state at one place.

@mrdoob mrdoob merged commit 53d08e6 into mrdoob:dev Mar 21, 2021
@mrdoob
Copy link
Owner

mrdoob commented Mar 21, 2021

Thanks!

@karimbeyrouti
Copy link
Contributor

Pointed our code at the dev branch, and found some more path issues, mentioned here:

#20608 (comment)

Should I raise a new issue ?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Mar 26, 2021

I'm already looking at them.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Mar 26, 2021

#20608 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SVGLoader - Incorrect/mangled path rendering
4 participants