ManimCommunity / ManimCommunity/manim

Extract fit-to-size methods from mobject.py into a mixin module

Open
#4,735 3 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Python
Stars
40.9k
Forks
3.1k
Avg merge
3d 12h
Merged PRs (30d)
25

Description

`manim/mobject/mobject.py` is by far the largest file in `manim/` at 3512 lines, well ahead of the next-biggest. Going through it I noticed that a fair amount of recent bug-fix activity also concentrates there, which I think makes sense: as a file grows past a certain size, the cost of changing any one part of it goes up because every change has to reason about more local context, and that cost tends to show up empirically as more defects.

I don't think a wholesale split is the right first move; too disruptive and too easy to get wrong. But it seems worth taking one cohesive group of methods, moving them into their own module, and seeing how the diff reviews. Small enough to be safe, large enough to be useful.

While reading the file I went looking for a group that would extract cleanly. The fit-to-size family is the best candidate I found. It's seven methods around lines 1752–1870:

- `rescale_to_fit` is the helper
- `scale_to_fit_width` and `stretch_to_fit_width`
- `scale_to_fit_height` and `stretch_to_fit_height`
- `scale_to_fit_depth` and `stretch_to_fit_depth`

What makes the group attractive for extraction is that it's a closed sub-graph. The six fit/stretch wrappers all delegate to `rescale_to_fit`, and `rescale_to_fit` itself only calls out to `length_over_dim`, `scale`, and `stretch`, all defined elsewhere on `Mobject`. Nothing inside the family is called by anything outside the family within `mobject.py`. So extracting it doesn't require untangling references in any other part of the file; it really is just relocation.

The shape I'd propose is a mixin. Create `manim/mobject/_fit.py` with a `_FitMixin` class containing the seven methods, then change the class declaration in `mobject.py` from `class Mobject:` to `class Mobject(_FitMixin):` and delete the methods from the body. At runtime, `Square().scale_to_fit_width(5)` keeps working exactly as before because Python's MRO resolves the methods through inheritance. The public API is unchanged, every existing caller continues to work, and `mobject.py` shrinks by about 119 lines.

I considered two alternatives. The first is module-level helper functions (`_fit.scale_to_fit_width(mob, width)`). That would break the public API and require updating every call site, including user code. That's much too invasive. The second is splitting `Mobject` into a proper base class consolidating multiple extractable groups at once. That would be more cohesive long-term, but it's a much bigger design call and I'd want maintainer input before going there. The mixin is the smallest commitment: it does one thing without claiming a position on what comes next.

There's one trade-off I want to flag explicitly. The mixin's methods call `self.length_over_dim`, `self.scale`, and `self.stretch`, which are defined on the host class `Mobject`, not on the mixin itself. This is the well-known mypy-with-mixins limitation: the checker can't see through the inheritance at type-check time. I worked around it with three targeted `# type: ignore[attr-defined]` markers at the three call sites, with a docstring at the top of `_fit.py` explaining why they're there. I want to be upfront that these are still `type: ignore` markers, just structural ones with a documented reason rather than blanket suppressions. If that's an unacceptable cost in this codebase I'd rather know before opening the PR than after.

I have a working version locally. `ruff` is clean, `mypy` is clean on `_fit.py`, MRO inspection on `Square`, `Circle`, and `Triangle` confirms all seven methods resolve to the mixin, and the pytest suite passes on every test I can run on Windows (the three failures I see are LaTeX-subprocess tests that fail identically against unmodified `main`, environmental rather than regressions).

The main thing I'd like input on before opening the PR is the mixin vs. base-class question. Mixin is what I have prepared, but if there's a preference for thinking about the broader split now rather than later, I'd want to know.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the seven fit-to-size methods around lines 1752–1870 in manim/mobject/mobject.py, then inspect the proposed manim/mobject/_fit.py structure and Mobject inheritance. Compare the mixin approach with the issue's stated constraints, preserving method resolution and the public API. Run ruff, mypy on _fit.py, and the relevant pytest suite; done means the methods resolve through Mobject without regressions.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
computer-graphics
Issue type
Refactor
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.