Repository: ManimCommunity/manim
Pull request: Fix issue where SpiralIn doesn't show elements.
<!-- Thank you for contributing to Manim! Learn more about the process in our contributing guidelines: https://docs.manim.community/en/latest/contributing.html -->

## Overview: What does this pull request change?
<!-- If there is more information than the PR title that should be added to our release changelog, add it in the following changelog section. This is optional, but recommended for larger pull requests. -->
<!--changelog-start-->

<!--changelog-end-->

## Motivation and Explanation: Why and how do your changes improve the library?
<!-- Optional for bugfixes, small enhancements, and documentation-related PRs. Otherwise, please give a short reasoning for your changes. -->
Fixes a bug where SpiralIn doesn't show elements that have a fill_opacity of 0. This also generates test data and adds a test to confirm the SpiralIn functionality. Resolve #3514

## Links to added or changed documentation pages
<!-- Please add links to the affected documentation pages (edit the description after opening the PR). The link to the documentation for your PR is https://manimce--####.org.readthedocs.build/en/####/, where #### represents the PR number. -->
https://manimce--3589.org.readthedocs.build/en/3589/

## Further Information and Comments
<!-- If applicable, put further comments for the reviewers here. -->



<!-- Thank you again for contributing! Do not modify the lines below, they are for reviewers. -->
## 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


Problem statement:
`SpiralIn` is not working with the first Element
`SpiralIn` doesn't show the first Element sometimes, for examples just visit the current documentation of `SpiralIn`

- [ ] Tests are needed for SpiralIn to prevent this from happening again
- [ ] Implementation needs to be fixed

Hints from issue discussion:
Started looking into this issue. Here is an example that fails to show the test object when I run `manim -pqh test.py`:

```python
class SpiralInBugExample(Scene):
    def construct(self):
        test = MathTex(r"\pi").scale(5)
        # test = Tex('test').scale(5) # Uncomment to test Tex
        # test = SingleStringMathTex(tex_string='test').scale(5) # Uncomment to test SingleStringMathTex
        # test = Circle() # Uncomment to test Circle
        shapes = VGroup(test)
        self.play(SpiralIn(shapes))
```
In the above example, you can uncomment any of the test lines to see the behavior of that object. They all behave the same. 

Something interesting I found was the issue occurs or goes away depending on whether or not you run another play before running SpiralIn. Here are the following orders with their results after running `manim -pqh test.py`. To see these results, replace the last line in my first example with one of these groups.
```python
self.play(SpiralIn(shapes)) # Will not show
self.play(DrawBorderThenFill(shapes)) # Will not show
self.play(ShowSubmobjectsOneByOne(shapes)) # Will show
```
```python
self.play(SpiralIn(shapes)) # Will not show
self.play(ShowSubmobjectsOneByOne(shapes)) # Will show
self.play(DrawBorderThenFill(shapes)) # Will show
```
```python
self.play(ShowSubmobjectsOneByOne(shapes)) # Will show
self.play(DrawBorderThenFill(shapes)) # Will show
self.play(SpiralIn(shapes)) # Will show
```
```python
self.play(ShowSubmobjectsOneByOne(shapes)) # Will show
self.play(SpiralIn(shapes)) # Will show
self.play(DrawBorderThenFill(shapes)) # Will show
```
```python
self.play(DrawBorderThenFill(shapes)) # Will show
self.play(ShowSubmobjectsOneByOne(shapes)) # Will show
self.play(SpiralIn(shapes)) # Will show
```
```python
self.play(DrawBorderThenFill(shapes)) # Will show
self.play(SpiralIn(shapes)) # Will not show
self.play(ShowSubmobjectsOneByOne(shapes)) # Will show
```
Each of the above groups behaved the same for each object type I tested. Another note, if SpiralIn isn't the first play, the SpiralIn play will cause the object to just fade in. It will not spiral. I don't know if this is expected. 

Going to continue looking into this the bug.

Commit message: Create test for SpiralIn

Patch to review:
diff --git a/manim/animation/creation.py b/manim/animation/creation.py
index 6f8173e35a..2c8be99d22 100644
--- a/manim/animation/creation.py
+++ b/manim/animation/creation.py
@@ -476,7 +476,7 @@ def interpolate_mobject(self, alpha: float) -> None:
         for shape in self.shapes:
             shape.restore()
             shape.save_state()
-            opacity = shape.get_fill_opacity()
+            opacity = shape.get_fill_opacity() or 1
             new_opacity = min(opacity, alpha * opacity / self.fade_in_fraction)
             shape.shift((shape.final_position - shape.initial_position) * alpha)
             shape.rotate(TAU * alpha, about_point=self.shape_center)
diff --git a/tests/test_graphical_units/control_data/creation/SpiralIn.npz b/tests/test_graphical_units/control_data/creation/SpiralIn.npz
new file mode 100644
index 0000000000..8331ea24c0
Binary files /dev/null and b/tests/test_graphical_units/control_data/creation/SpiralIn.npz differ
diff --git a/tests/test_graphical_units/test_creation.py b/tests/test_graphical_units/test_creation.py
index c6cfc3e504..bcab0bafa4 100644
--- a/tests/test_graphical_units/test_creation.py
+++ b/tests/test_graphical_units/test_creation.py
@@ -94,3 +94,11 @@ def test_z_index_introducer(scene):
     b.set_z_index(-1)
     scene.play(Create(b))
     scene.wait()
+
+
+@frames_comparison(last_frame=False)
+def test_SpiralIn(scene):
+    circle = Circle().shift(LEFT)
+    square = Square().shift(UP)
+    shapes = VGroup(circle, square)
+    scene.play(SpiralIn(shapes))


Code review comments:
diff_hunk: @@ -476,7 +476,7 @@ def interpolate_mobject(self, alpha: float) -> None:
         for shape in self.shapes:
             shape.restore()
             shape.save_state()
-            opacity = shape.get_fill_opacity()
+            opacity = shape.get_fill_opacity() or 1
path: manim/animation/creation.py
text: @user1:
I'm not sure this is the correct solution here as it modifies the fill opacity of a given mobject -- this actually also happens in the test that you added; at the end of `SpiralIn`, the mobjects should be added to the scene with the opacity values they have been initialized with.

My suggestion for a slightly more complicated fix would be to split the current `opacity` into the typical `stroke_opacity` and `fill_opacity`, and then update both of them according to the rule given in `new_opacity`.

That way, no unexpected opacity changes happen.

@author:
Howdy @user1 ! Thank you for the feedback. I like your idea much more. I wasn't aware the mobject attributes could be set separately like that. That __getattr__ function is nifty! 
I changed the fix so it lines up with your idea as I understand it, and I updated my test data to reflect that change.

One issue is it looks like my change did throw off the test_banner test. The frame comparison shows that the change causes the starting square to show on the top of the video. Do you think my fix is bad, is there something new to track down, or do we just need to regenerate the test data for test_banner?