fix: sound timings for skipped animations

[BarryLarry17Jr]

Overview

Fix for issue #2449. The scene duration should not be updated for skipped animations.
If the scene duration must be updated for skipped animations we have a bigger issue at hand.

Further Information and Comments

Pre-existing tests should verify this fixes validity. However, more tests could be written to make sure.

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

[BarryLarry17Jr]

My modifications address the problem detailed in issue #2449. Nonetheless, there are other failing tests due to my alterations, as they result in the scene duration not being updated for animations that are skipped. I would appreciate it if someone with a deeper comprehension of scene duration could assess the validity of these modifications.

[behackl]

I just looked at this and tried to remember why I've implemented advancing time during skipped animations in #2220 -- I've found a partial answer.

I think in general it is correct that scene time should not advance if the skipping status is determined to be True via CairoRenderer.update_skipping_status(). There is, however, a problem: the file writer (which takes care of stitching together audio + video, and actually also produces the subcaption file) cannot distinugish between a partial movie file that is actually skipped and excluded from the output (as in the first case) and the case where a partial movie file is just not rerendered because there is a cached version of it available already.

In the first case, neither the time should be advanced, nor the .srt file should be added -- and corresponding music played in skipped sections should not be stitched to the file either. In the second case, where we use an existing cached partial movie file, both should happen.

Maybe it is possible to distinguish them and I just missed it when looking over it just now; or a new attribute for the renderer (or something along these lines) has to be added in order to deal with this. While I'd like to see both of these issues fixed simultaneously, I could be convinced to merge this partial solution preemptively. I'll think a bit more about it.

In an ideal world, the following scene would work under all possible circumstances and skipping configurations (skipping vs. not skipping the first section deliberately; using a cached version of the first play call vs. rendering using --disable_caching, and so on ...).

class TimeBug(Scene):
    def construct(self):
        self.next_section(skip_animations=True)
        self.wait()
        s = Square()
        self.add_subcaption("A square is created.")
        self.add_sound("click.wav")  # at time 1
        self.play(Create(s))
        self.wait()

        self.next_section(skip_animations=False)
        c = Circle()
        self.add_subcaption("This creates a circle.")
        self.add_sound("click.wav")  # at time 3
        self.play(Create(c, run_time=3))
        self.add_sound("click.wav")  # at time 6

[MrDiver]

Any updates ?

[behackl]

Closing due to inactivity.