-
Notifications
You must be signed in to change notification settings - Fork 361
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix events for infinite symbols #8611
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't say that I fully understand the issue but I've not worked with animations for quite some time. But you do, so I trust that this is an improvement.
I am quite confident that it is an imporvement necessary. I am confident (although a little less so) that the modification I made to the code is the correct one. But I guess if it is wrong, we will figure it out. My tests were fine. |
This PR affects the output of this test ( |
I added # GMT_KNOWN_FAILURE in the test script that has to be updated. I will update the reference image (in DVC) in another PR. I have doubts on how to do it. |
What are those error with the tests? May I merge it anyway? I would think so but I am not very sure. |
The tests are unrelated to changes in this PR. So please merge. |
Intro
The description about the coda phase in the events documentation mentions:
Also, at the end of the -M section, it says:
This implies that symbols without a defined duration (infinite) shouldn’t reach the coda phase. However, currently, this is what is happening (as seen in animation #8607 and animation 08).
This also means that I have to define the symbol properties via the coda phase modifiers (-M+c). In fact, I must include these modifiers to make the symbols visible (e.g., like -Mt+c0), because the default behavior is for them to become invisible.
Proposed Fix
I made a fix to address this issue. The problem was (I believe) that only events with finite duration could reach the normal phase. So, in this PR, I remove that condition. I tested it on my PC (with the same script from the previous link) and got this animation, which looks fine to me:
Tutorial_1_Test.mp4
I think this solution is better because now the symbols at the end retain the properties defined by -Sc and -G (without the -M+c modification). This means the code could be simplified:
I also try the original animation made by Paul (WED-A_Vid_1.sh) and I got the same animation.