manim icon indicating copy to clipboard operation
manim copied to clipboard

Fix `VGroup.scale(scale_stroke=True)` to Respect Submobjects' Stroke Widths

Open irvanalhaq9 opened this issue 7 months ago • 2 comments

Overview: What does this pull request change?

  1. Fix: https://github.com/ManimCommunity/manim/issues/4229
  2. Add related test

Further Information and Comments

Example:

class FixVGroupScaling(Scene):
    def construct(self):
        square = Square(side_length=2).set_stroke(color=RED_E, width=20)
        square.set_stroke(color=WHITE, width=60, background=True)
        self.add(square)
        vg = VGroup(square)
        
        # Scale 1.0 (scale_stroke=True): No changes expected
        self.play(vg.animate.scale(1, scale_stroke=True))

        # Scale 0.5 (scale_stroke=True): Size and stroke width halved
        self.play(vg.animate.scale(0.5, scale_stroke=True))
        
        # Scale 2.0 (scale_stroke=False): Size doubled, stroke width unchanged
        self.play(vg.animate.scale(2))
        self.wait()

https://github.com/user-attachments/assets/89c344da-b8da-4c31-b34a-969f7a3b58a5

Reviewer Checklist

  • [ ] The PR title is descriptive enough for the changelog, and the PR is labeled correctly
  • [ ] If applicable: newly added non-private functions and classes have a docstring including a short summary and a PARAMETERS section
  • [ ] If applicable: newly added functions and classes are tested

irvanalhaq9 avatar Apr 29 '25 20:04 irvanalhaq9

Hello,

Thank you for the super quick fix. I have one question though: Isn't the fix super dependent on the implicit fact that get_family() returns self first? Because the set_stroke of self is called first, setting wrongly the value recursively to the submobjects, and then it's overloaded by calling again the set_stroke of each of the submobjects

https://github.com/ManimCommunity/manim/blob/f304bd93eab1ca450a86eff476c7f51bd81fe1fb/manim/mobject/mobject.py#L2311-L2339

Maybe, we could do the same thing, but explicitly, by calling first set_stroke on self and then on each of the self.submobjects.

ccalauzenes avatar Apr 30 '25 11:04 ccalauzenes

Hi @ccalauzenes Thanks for reviewing. You're right. The method .set_stroke() should only be applied to the mobject, not to its submobjects. I have fixed it by adding family=False. Test:

class FixVGroupScaling2(Scene):
    def construct(self):
        square = Square(side_length=2)
        square.set_stroke(color=RED_E, width=40)
        square.set_stroke(color=WHITE, width=120, background=True)
        rec = Rectangle(height=4,width=5)
        rec.set_stroke(color=RED, width=10)
        rec.set_stroke(color=YELLOW_A, width=20, background=True)
        
        vg = VGroup(square)
        rec.add(vg)
        self.add(rec)

        self.play(rec.animate.scale(1, scale_stroke=True))
        self.play(rec.animate.scale(0.5, scale_stroke=True))
        self.play(rec.animate.scale(2))
        self.wait()

https://github.com/user-attachments/assets/cba5c74b-4bc9-4e46-bb2c-2b54b04e33eb

irvanalhaq9 avatar Apr 30 '25 13:04 irvanalhaq9