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 'points' attribute #25206

Merged
merged 1 commit into from
Dec 31, 2022

Conversation

ozekik
Copy link
Contributor

@ozekik ozekik commented Dec 30, 2022

Description

SVGLoader() parses the values of points attribute of <polyline>/<polygon> wrongly.
For example, it parses 1e2,50 to a pair of 2 and 50, not 100 and 50; the problem is that the value of the attribute is defined to be a list of <number>+ 1, which allows numerals in the exponential notation2, but SVGLoader()'s regex does not.

Demo: https://stackblitz.com/edit/js-ulgby6?devToolsHeight=33&file=index.js

This is caused by the following regex in SVGLoader.js for parsing <polyline>/<polygon>. This PR fixes the regex accordingly.

const regex = /(-?[\d\.?]+)[,|\s](-?[\d\.?]+)/g;

const regex = /(-?[\d\.?]+)[,|\s](-?[\d\.?]+)/g;

Footnotes

  1. https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/points

  2. https://developer.mozilla.org/en-US/docs/Web/SVG/Content_type#number

@Mugen87 Mugen87 added this to the r149 milestone Dec 31, 2022
@ozekik ozekik force-pushed the fix/SVGLoader-regex branch from 2abd648 to d6470d3 Compare December 31, 2022 10:25
@ozekik
Copy link
Contributor Author

ozekik commented Dec 31, 2022

Thanks for the review. I just noticed further that [,|\s] in the original regex should be replaced by (?:,|\s), so I made an edit to the commit. I'm sorry but please re-review if necessary.

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 31, 2022

The test files still do work 😊 !

@Mugen87 Mugen87 merged commit 2ca30cf into mrdoob:dev Dec 31, 2022
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.

2 participants