manim icon indicating copy to clipboard operation
manim copied to clipboard

Coords to point accept sequence and allow omitted positions

Open k4pran opened this issue 3 years ago • 3 comments

Changelog / Overview

Fixes #1666

  • Allows coords_to_point to accept sequences
  • Adds padding for mobject's shift method when less than 3 dimensions are set

Motivation

Some type hints in coordinate systems said they accept sequences but only vargs were accepted, fixed typehints and allow sequences to be passed.

Explanation for Changes

Gives more flexibility for coords_to_point method while keeping it backwards compatible. Also fixes type hints

Testing Status

Further Comments

I am not sure of all the places in manim where coordinates are accepts but it forces you to state all three x, y and z dimensions. I am guessing this only addresses a small part of it. I think it would be better to create a class with x, y, z or something to be used across manim to keep it consistent

Checklist

  • [X] I have read the Contributing Guidelines
  • [X] I have written a descriptive PR title (see top of PR template for examples)
  • [X] I have written a changelog entry for the PR or deem it unnecessary
  • [X] My new functions/classes either have a docstring or are private
  • [X] My new functions/classes have tests added and (optional) examples in the docs
  • [X] 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

k4pran avatar Jun 17 '21 19:06 k4pran

In general, a great idea! Currently, I think it is a bit confusing if shift is working with only 2 parameters, and move_to still requires 3.

class Example(Scene):
    def construct(self):
        d=Dot(point=[1,2])
        d.shift([-3,0])
        d.move_to([4,-3]) <- not working
        self.add(d)

I made an issue https://github.com/ManimCommunity/manim/issues/539 for that some time a go, which I am closing now, as we can also have here. I think we should also make some careful speed testing for this new feature, as it might slow manim down.

kolibril13 avatar Jun 18 '21 14:06 kolibril13

In general, a great idea! Currently, I think it is a bit confusing if shift is working with only 2 parameters, and move_to still requires 3.

class Example(Scene):
    def construct(self):
        d=Dot(point=[1,2])
        d.shift([-3,0])
        d.move_to([4,-3]) <- not working
        self.add(d)

I made an issue #539 for that some time a go, which I am closing now, as we can also have here. I think we should also make some careful speed testing for this new feature, as it might slow manim down.

Yes and I imagine there is quite a few places in manim that will still require 3 coords, there was a discussion about this on discord and I think rather than plugging holes in different methods to ensure they accept less than 3 axis, we could use a Point class and begin using that across manim. It will be a bigger task but easier to make things like this consistent across all the methods

k4pran avatar Jun 18 '21 16:06 k4pran

Might be related to

  • #2739

@ryanmccauley211 do you want to test if this also fixes #1666 ? Then we could close this or move missing changes to a new PR?!

MrDiver avatar Jun 18 '22 17:06 MrDiver