-
-
Notifications
You must be signed in to change notification settings - Fork 365
Add support for saving multiple frames when using animation #2785
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
Conversation
|
\ci fast |
f3e233c to
47248d7
Compare
|
\ci full |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2785 +/- ##
==========================================
+ Coverage 96.64% 96.65% +0.01%
==========================================
Files 142 142
Lines 12918 12960 +42
==========================================
+ Hits 12484 12526 +42
Misses 434 434 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
765b866 to
dbe49c3
Compare
|
Looks quite good! How does it behave with |
I'm pretty sure it's overriden for now, so animation time is ignored. What would be the expected behavior ? |
mwestphal
left a comment
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.
some suggestions
|
Definitely needs a @snoyer review |
mwestphal
left a comment
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.
LGTM, needs a doc update and a @snoyer review :)
mwestphal
left a comment
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.
small doc change again
b7a4252 to
974b6b1
Compare
mwestphal
left a comment
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.
small suggestion, @snoyer please review
Co-authored-by: Mathieu Westphal <[email protected]>
3fd0469 to
b66688b
Compare
mwestphal
left a comment
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.
a test missing for full coverage
229c634 to
0f27291
Compare
|
lets just for a last @snoyer review :) |
snoyer
left a comment
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.
As mentioned in the Discord thread, I still feel like the code paths are more complex than they need to be but I haven't had time to sit down and see if they can be untangled.
Otherwise it looks like it work and the UI is good from what I can tell so we can always refactor and optimize later.
|
all suggestions applied |
Describe your changes
Add support for saving multiple frames when using animation with
--outputandframetemplate filenameIssue ticket number and link if any
#46
Checklist for finalizing the PR
.github/workflows/versions.json, I have updateddocker_timestampContinuous integration
Please write a comment to run CI, eg:
\ci fast.See here for more info.