ManimCommunity / manim

A community-maintained Python framework for creating mathematical animations.
https://www.manim.community
MIT License
21.45k stars 1.57k forks source link

Use a standard coordinate system by default #275

Closed leotrs closed 2 years ago

leotrs commented 4 years ago

By default, the top of the frame is at y=8.0 (see L61 in config.py). Why should this be the case? The choice of 8.0 seems arbitrary. A more standard choice would be to set the range of the vertical axis to be either (-frame_width, frame_width) or (-1.0, 1.0), but not (-8.0, 8.0).

Of course, any choice will be somewhat arbitrary, but some are more natural than others. In fact, matplotlib supports the usage of many different scales, see here.

EDIT: To be clear, there are two issues mentioned here.

  1. The choice of 8.0 seems very arbitrary and (I think) should be changed. @PgBiel suggests that it could be made customizable through the config system. I agree. This is an easy PR.

  2. I'd be great to support multiple coordinate systems, like matplotlib. This is a more substantial PR.

leotrs commented 4 years ago

I don't particularly like the way that matplotlib handles this. Something that I would love to see is to be able to support something like:

# by default, use pixels
mobj.move_to(144, -10, 0)

# use strings to determine different coordinate systems
mobj.move_to("10px", "-5px", "3.3px")       # same as mobj.move_to(10, -5, 3.3)
mobj.move_to("10%", "-5%", "0")             # as percentage of total frame height
mobj.move_to("30pt", "-15pt", "0")          # as points
# etc

I think this would be really easy to implement on a per-function basis, but not so sure about doing it in the whole code-base. Perhaps a decorator that parses the strings and converts them back to a single coordinate system? Would this be efficient?

PgBiel commented 4 years ago

Might be easier to have some class like

Unit(30, "px")

Also, regarding the max coords, those should probably be customizable

leotrs commented 4 years ago

I love the idea of using a Unit class!

ToxicGLaDOS commented 4 years ago

In most graphics libraries I've seen (0, 0) indicates the top left of the screen while positive x goes right and positive y goes down. So (frame_width, frame_height) would be the bottom-right corner. Although manim isn't necessarily a "graphics" library it's close enough that this is what I was expecting when I started using it. I think this would probably be the most standard and least arbitrary choice.

leotrs commented 4 years ago

@ToxicGLaDOS yes, this is also fairly standard and should be supported as well.

leotrs commented 4 years ago

Here is an idea for a Unit class that would (I think?) work across the codebase. The main idea is to introduce a class Unit such that we can do u = Unit(10, "px") and then u.value would give the correct position in the manim screen (currently, a value in the interval (-8.0, 8.0)). And then introduce a decorator

@accepts_unit
def move_to(x, y, z):
    pass

that would do the following:

def accepts_unit(f):
    @wraps(f)
    def wrapper(*args):
        return f(*[a.value if isinstance(a, Unit) else a for a in args])
    return wrapper

So if any argument was passed as a Unit, the decorator just replaces it by the correct numerical value. In this way, none of the functions have to change. Would this work? Am I missing anything? can anyone see anything wrong? Would this be efficient?

ToxicGLaDOS commented 4 years ago

So if any argument was passed as a Unit, the decorator just replaces it by the correct numerical value. In this way, none of the functions have to change. Would this work? Am I missing anything? can anyone see anything wrong? Would this be efficient?

I like the idea of a Unit class for sure, because of the benefits to end users, but doesn't this solution just "kick the can down the road"? If we're just automatically converting from whatever space the user is using (pixels here) to the (-8, 8) space we are currently using than all the code internal still has to use the current unintuitive coordinate space. Maybe that deserves it's own effort and can be done independently after this Units idea is done though.

leotrs commented 4 years ago

Yeah, so like I said at the top post, there are two different issues here.

On the topic of using different coordinate systems, the Unit class and decorator should be sufficient.

The topic of choosing a more standard coordinate system is still open.

kilacoda-old commented 4 years ago

What effect would such a system have on constants like UP, DOWN, IN, OUT etc.? Also, imo it doesn't really make sense to measure distance along the z-axis in pixels.

My suggestion would be to keep the unit class as a wrapper around the numpy array we use currently, i.e Unit(1,1,0)=np.array([1,1,0])

By default, the top of the frame is at y=8.0 (see L61 in config.py). Why should this be the case? The choice of 8.0 seems arbitrary.

https://github.com/ManimCommunity/manim/blob/7815051dda1a5bd62492369563f1ea5cff4e7aac/manim/config.py#L61

This is the height of the whole frame iirc, so the y axis ranges from [-4,4] (NOT [-8,8]). It is a bit weird that it's set as 8 and not 9 to match up with the 16:9 aspect ratio, because the following lines set up the frame width to be the aspect ratio into the frame height, which comes out to be 14.2 repeating with frame_height=8 and 16 with frame_height=9. Changing frame_height to 9 will then keep the y axis within [-4.5,4.5].

https://github.com/ManimCommunity/manim/blob/7815051dda1a5bd62492369563f1ea5cff4e7aac/manim/config.py#L62-L64

@accepts_unit
def move_to(x, y, z):
   pass

Functions like move_to aren't just used to move to an explicit set of coordinates though, but moving to the spot of another object as well (by calling its get_center() method internally) for example mobj1.move_to(mobj2), so that should be kept in mind as well.

leotrs commented 4 years ago

What effect would such a system have on constants like UP, DOWN, IN, OUT etc.?

Good q. I think they should be kept in whatever underlying units manim uses internally (either between -8 and 8, or, if that changes, between -1, 1). An alternative is to convert them to Units themselves: UP = Unit(0, 1, 0, "internal"), or whatever.

Also, imo it doesn't really make sense to measure distance along the z-axis in pixels.

This is a good point. However, I think it's fine to ask the user to bend their minds a little bit ;) Unit(100, "px") would be placed exactly 100 pixels away from the screen, if you were measuring that space with the same ruler as you use to measure pixels ON the screen. In this case we'd be using pixels as a units for measuring length, not actual pixels on the screen. I think it's fine, if a bit weird.

My suggestion would be to keep the unit class as a wrapper around the numpy array we use currently, i.e Unit(1,1,0)=np.array([1,1,0])

Yeah, if implemented, Unit should accept multiple coordinates as well (which I missed in my examples). Also, I agree that the underlying representation should be a numpy array expressed in whatever internal units manim is using.

This is the height of the whole frame iirc, so the y axis ranges from [-4,4]. It is a bit weird that it's set as 8 and not 9 to match up with the 16:9 aspect ratio, because the following lines set up the frame width to be the aspect ratio into the frame height, which comes out to be 14.2 repeating with frame_height=8 and 16 with frame_height=9. Changing frame_height to 9 will then keep the y axis within [-4.5,4.5].

This is perhaps yet another system of coordinates that Unit could support.

Functions like move_to aren't just used to move to an explicit set of coordinates though, but moving to the spot of another object as well (by calling its get_center() method internally) for example mobj1.move_to(mobj2), so that should be kept in mind as well.

You're right, I forgot about this. However, I think the current proposal of the decorator already handles this correctly. It would only change those arguments that are of Unit type. If you pass a Mobject instead, the decorator leaves it alone. (Which brings up another question: perhaps there could be another decorator that makes a function accept an arbitrary Mobject --just like the current proposal allows any function to accept a Unit instead of a number-- in that case, all of the input-checking done by each individual function can be scrapped across the whole codebase.

leotrs commented 4 years ago

I still think we should implement the Unit class. Also, if you grep the codebase, it seems that the standard of setting the height equal to 8 is referred to as MUnits :shrug:

PhilipEnchin commented 4 years ago

The topic of coordinates is of great interest to me here...

The idea of flexibility with different units, etc. is nice, though I don't buy that it's totally necessary, but I'll go with it...

Anyway, yes. 8x14-ish is arbitrary, and seemed really strange to me, but after reading everything here, I figure it's the relative closeness of 14:8 (or 7:4) to 16:9. However, with the ability to define any aspect ratio, and with the proliferation of square and portrait (shudder) videos, it feels even more arbitrary...

Whether there's coordinate-system flexibility or not, it seems to me that the three issues are:

  1. Where's the origin, or point (0, 0). Currently it's in the middle of the screen.
  2. Which axes point in which directions? Currently x goes left to right, and y goes from bottom to top.
  3. How big is a unit? Currently, it's 1/8 of the video height.

I have some holes in my knowledge here, in case it's not obvious. I haven't done anything in 3D, so I can't speak to how the z axis behaves, and I don't know if there's any flexibility built in already...

In any case, my personal favourite coordinate system has the origin in the middle, and the unit size is 1/2 of the video width, so the x coordinates range from -1 to 1, and the y coordinates are flexible depending on the aspect ratio.

My reason for finding my way to this issue is actually because I discovered that the units are fixed to the video height, but once upon a time they were fixed to the width instead. In other words, the height of the video is now always 8 units, whereas it used to be that the width of the video was fixed at 14(ish*) units. It seems more intuitive (to me) to have a fixed width and fluid height, and I couldn't find any PR or issue discussing this particular point, so I was wondering if perhaps this change was a mistake, or at least incidental...

* This is discussed already...

leotrs commented 4 years ago

@PhilipEnchin thanks very much for your thoughts! This issue is indeed in need of more discussion.

The idea of flexibility with different units, etc. is nice, though I don't buy that it's totally necessary, but I'll go with it...

It is indeed not necessary but it would give a lot of convenience to the user.

My reason for finding my way to this issue is actually because I discovered that the units are fixed to the video height, but once upon a time they were fixed to the width instead. In other words, the height of the video is now always 8 units, whereas it used to be that the width of the video was fixed at 14(ish*) units. It seems more intuitive (to me) to have a fixed width and fluid height, and I couldn't find any PR or issue discussing this particular point, so I was wondering if perhaps this change was a mistake, or at least incidental...

I do not remember when/if we changed this. Perhaps it was changed in 3b1b?

yoshiask commented 4 years ago

I'd like to point out that a more standard coordinate system would help devs attempting to build graphical editors for Manim (like myself), because we'd only have to scale the coordinates on objects instead of doing weird and buggy calculations to effectively move the origin.

PhilipEnchin commented 4 years ago

My reason for finding my way to this issue is actually because I discovered that the units are fixed to the video height, but once upon a time they were fixed to the width instead. In other words, the height of the video is now always 8 units, whereas it used to be that the width of the video was fixed at 14(ish*) units. It seems more intuitive (to me) to have a fixed width and fluid height, and I couldn't find any PR or issue discussing this particular point, so I was wondering if perhaps this change was a mistake, or at least incidental...

I do not remember when/if we changed this. Perhaps it was changed in 3b1b?

The current default in 3b1b is a fixed width at 14 2/9 (-7 1/9 to +7 1/9). I generated a couple videos to make sure I had this right, which I've included here. The coordinates of the dot are displayed as it moves from centre, to bottom left, to bottom right, and back. Made these tonight using the most recent versions of each Manim repo. Notice the effects on the sizes of the dots and text.

I'm not sure if this was done intentionally or if it was incidental as the code has been refactored... I've created a pull request 415 that switches it to a fixed width.

FixedWidth FixedHeight

cobordism commented 4 years ago

Just to chime in here: I sometimes make 9:16 videos (for the phone) instead of 16:9.

Usually I set my manim video manually to width of 9 and height of 16 (and also change the corresponding output video sizes). I have one vertical.cfg and one horizontal.cfg file for this purpose.

There is no a-priori reason for me to change manim units, I could go 8.0 x 8.0 is all cases (or 14.whatever) but it's nice to know that a shift(2*RIGHT) and a shift(2*DOWN) produce an equal sized shift on the screen.

All that is to say: don't assume a 16:9 format in all cases :)

PS I just saw:

and with the proliferation of square and portrait (shudder) videos,

There are legitimate uses here. For example if your video is an animation of a page of text being built up line by line - you don't want landscape... who writes maths notes in landscape mode in their notebooks? (shudder)

jvdoorn commented 3 years ago

https://github.com/ManimCommunity/manim/blob/fa0a82c2261a43a786e7f6b5ccb383f33e91d720/manim/config/config.py#L127

According to this issue, it should be configurable right? Perhaps, besides the --resolution flag, we should introduce a --aspect_ratio flag. This flag would determine pixel_width based on pixel_height and the given ratio.

I could work on this and see what I can do about the Unit class. Do we have a function anywhere that can convert "10%" to pixels par example?

leotrs commented 3 years ago

@jvdoorn thanks for volunteering (again!) to work on this.

As for the changes you mention, I would rather see a single change first that implements the Unit class and tests it thoroughly. Then we can start incorporating it to the rest of the codebase. And only then I would start thinking about user-facing changes such as adding more CLI flags and making it configurable etc.

I'm still partial to this way of doing it and keep in mind this comment too. But if you find a better way, please do that by all means.

jvdoorn commented 3 years ago

I'm a bit hesitant about how to implement the conversion of units though. Because a lot of it would depend on the config. Par example a shift of 10% is different for various resolutions and even directions possibly. Does anyone have any leads or examples currently implemented in the software or suggestions that might implement good unit conversion?

leotrs commented 3 years ago

Yeah this is very tricky indeed. Unfortunately you won't find anything about this in manim. It just always uses the default manim units. (These are sometimes called MUnits in the codebase.)

As a first pass, I would just compute 10% of whatever the value of config["frame_height"] is. It's fine if "10%" is different for different directions because it's meant to be a relative measure anyway. Now, "10px" should be always absolute, but in that case it depends on screen size.

I don't really know how other software does this. But for example inkscape and latex can do this natively very well. Matplotlib has a whole framework for converting between axes units and data units and so on.

That was very rambly and not really an answer to your question, but hope it helps :sweat_smile:

eulertour commented 3 years ago

It seems weird that manim should use pixels as a unit of distance, especially by default. Does that mean that the same scene rendered in different resolutions (but the same aspect ratio) would output different videos?

There's a subtle quality-of-life benefit in the fact that moving something by 1 munit basically does what you expect; even without knowing anything about the video resolution or even what a munit is, it just moves it "some". It's the same way for the fact that the default coordinate systems matches the coordinate plane you learn in school.

By contrast, moving things by 40 pixels or so (or god forbid computing A * sine(70) pixels when plotting) seems like a step in the wrong direction.

leotrs commented 3 years ago

I agree that pixels shouldn't be the default coordinate system, but it would be a nice-to-have, if at all possible to implement.

I think the first step is to support both MUnits (8x14.2 or whatever it is) as well as a more standard unitary frame of reference (1 x 9/16). Pixels can come later.

ad-chaos commented 2 years ago

Unit conversion was added via #742. Closing as that PR got merged.