Skip to content
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

Autoplay #1065

Merged
merged 5 commits into from
Nov 14, 2023
Merged

Autoplay #1065

merged 5 commits into from
Nov 14, 2023

Conversation

technologeli
Copy link
Contributor

Brief description of changes:

  • Created boolean scene.animation.autoplay, false by default
  • Added CLI option --animation-autoplay
  • When scene.animation.autoplay is true, files with animations will automatically start their animation.

Copy link

codecov bot commented Nov 12, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f037d90) 96.22% compared to head (4f834cb) 96.23%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1065   +/-   ##
=======================================
  Coverage   96.22%   96.23%           
=======================================
  Files         120      120           
  Lines        7026     7031    +5     
=======================================
+ Hits         6761     6766    +5     
  Misses        265      265           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mwestphal
Copy link
Contributor

So, if I get this right, it means that autoplay will automatically play a file on load. Eg, when changing file, it will start playing the animation. Is that correct @technologeli ?

Wdyt @Meakk ?

@Meakk
Copy link
Contributor

Meakk commented Nov 12, 2023

Looks like the correct approach to me. 👍
I don't know if it's possible to cover that with a test, any input @mwestphal ?

@mwestphal
Copy link
Contributor

Definitely possible! Animation is already tested with an interaction test. Just removing the interaction with the same test should provide the same output.

@mwestphal
Copy link
Contributor

@technologeli let me know if you need help with adding an test.

@Meakk
Copy link
Contributor

Meakk commented Nov 13, 2023

Adding a simple application test with --animation-autoplay option should be enough, right @mwestphal?

@technologeli see https://github.com/f3d-app/f3d/blob/f037d908c081cc2960999f7c53eaf86f89950816/application/testing/CMakeLists.txt#L140C37-L140C37

Adding a new line like:

f3d_test(NAME TestGLTFReaderWithAnimationAutoplay DATA BoxAnimated.gltf ARGS --geometry-only --animation-autoplay DEFAULT_LIGHTS)

Then you should be able to run ctest -R TestGLTFReaderWithAnimationAutoplay -VV to get the baseline file to copy to testing/baselines.
When the test pass, you can push the test and the coverage CI job should be happy.

@technologeli
Copy link
Contributor Author

Thank you for the pointers, I had a test written but was unsure of how to get it passing while using ctest.

@mwestphal
Copy link
Contributor

I had a test written but was unsure of how to get it passing while using ctest.

ctest -R Autoplay :)

@mwestphal
Copy link
Contributor

Thanks a lot for your contribution! Do not hesitate to reach out if you need any help fixing another issue!

@mwestphal mwestphal merged commit 01565ff into f3d-app:master Nov 14, 2023
41 checks passed
@Meakk Meakk mentioned this pull request Nov 14, 2023
mwestphal added a commit that referenced this pull request Feb 10, 2024
mwestphalnew pushed a commit to mwestphalnew/f3d that referenced this pull request Feb 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants