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

Add SVG elliptical arc support #1475

Merged
merged 8 commits into from May 7, 2021

Conversation

newell
Copy link
Contributor

@newell newell commented May 6, 2021

Changelog / Overview

Motivation

Add SVG elliptical arc support to manim.

See: https://www.w3.org/TR/SVG11/paths.html#PathDataEllipticalArcCommands

Explanation for Changes

Adds support to manim for SVGs that contain paths with elliptical arcs.

Testing Status

manim rendering https://www.w3.org/TR/SVG11/images/paths/arcs01.svg

TestArcs01_ManimCE_v0 6 0

Further Comments

Checklist

  • I have read the Contributing Guidelines
  • I have written a descriptive PR title (see top of PR template for examples)
  • I have written a changelog entry for the PR or deem it unnecessary
  • My new functions/classes either have a docstring or are private
  • My new functions/classes have tests added and (optional) examples in the docs
  • My new documentation builds, looks correctly formatted, and adds no additional build warnings

Reviewer Checklist

  • The PR title is descriptive enough
  • The PR is labeled correctly
  • The changelog entry is completed if necessary
  • Newly added functions/classes either have a docstring or are private
  • Newly added functions/classes have tests added and (optional) examples in the docs
  • Newly added documentation builds, looks correctly formatted, and adds no additional build warnings

Copy link
Collaborator

@markromanmiller markromanmiller left a comment

Choose a reason for hiding this comment

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

Overall the code is good - I saw a version of this earlier today.

The only blocking changes are the ones done by the CI - have you run black and isort?

One improvement I will suggest but not require is handing a commands with 7*n numbers, not just 7. If more than 7 numbers appear after an arc command, the next 7 are interpreted as their own arc command, and so on. While this is part of the spec, I don't expect to see it all that often, so it's not a blocking issue.

@markromanmiller
Copy link
Collaborator

Build looks good, but looks like I don't have edit access as a maintainer, so I can't update with the most recent version to perform the merge.

Can you either update and ping me or allow access for maintainers? (tbh not sure where that option is)

@Aathish04 Aathish04 merged commit 57a90b7 into ManimCommunity:master May 7, 2021
@behackl behackl added the enhancement Additions and improvements in general label May 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Additions and improvements in general
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants