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

Refactor SVG path parser to accept scientific notation #873

Merged
merged 4 commits into from
Dec 12, 2024

Conversation

Junology
Copy link
Contributor

@Junology Junology commented Dec 9, 2024

Refactor PShapeSVG.parsePath() so that it now accepts SVG with coordinates in scientific notation such as 3.14e-2.
It fixes #870 (and #816 too?).

I added a test case, but I am afraid it may not be enough.
In particular, the behavior of the function might change to some extent especially for 'ill-formed' SVG paths.
Please let me know if you have good test cases.

Thank you.

@Stefterv
Copy link
Collaborator

Screenshot 2024-12-10 at 08 53 47 Tested on macOS with your example and another found svg

@Stefterv Stefterv self-requested a review December 10, 2024 07:55
@Stefterv
Copy link
Collaborator

Thank you Junology for this pull request! Really cool to see such a quick fix!

Copy link
Collaborator

@Stefterv Stefterv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great addition! One optional change, for future readability

* 3: on a digit or a sign in exponent in scientific notation, e.g. 3.14e-2)
* 4: on a digit sequence in exponent
*/
int lexState = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understandable, could it be with a enum instead of an integer? I'm afraid we might lose this documentation at some stage

@SableRaf
Copy link
Collaborator

Note: I updated the #816 placeholder to include @SaumyaKarnwal's Diff and full description

@SableRaf
Copy link
Collaborator

I added a test case, but I am afraid it may not be enough.
In particular, the behavior of the function might change to some extent especially for 'ill-formed' SVG paths.
Please let me know if you have good test cases.

@Stefterv @Junology: Should we test this with a wider range of SVGs?

@Stefterv
Copy link
Collaborator

Yes, but it shouldn't block this PR

@Junology
Copy link
Contributor Author

@Stefterv I made the type of lexState into an enum LexState. The if-branches got more self-documented now. Thank you for advice.

@SableRaf Variety of test cases would be very helpful when someone tries to extend the SVG parser (e.g. tag).

@SableRaf SableRaf merged commit 8f37ee9 into processing:main Dec 12, 2024
6 checks passed
@SableRaf
Copy link
Collaborator

Merged! Thanks @Junology for you contribution and @Stefterv for your review 💙

Variety of test cases would be very helpful when someone tries to extend the SVG parser (e.g. tag).

Thanks for clarifying. Please feel free to open a separate issue on adding SVG test cases.

@SableRaf
Copy link
Collaborator

@all-contributors please add @Junology for code

Copy link
Contributor

@SableRaf

I've put up a pull request to add @Junology! 🎉

@hx2A
Copy link
Collaborator

hx2A commented Dec 12, 2024

Thank you, @Junology , for this PR! A welcome enhancement to Processing's SVG support.

@Junology
Copy link
Contributor Author

I tested #750 (and #816) can be closed now.

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.

loadShape() cannot load SVG with the scientific notation
4 participants