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

Add Cycle through animations in glTF files #1155

Merged
merged 40 commits into from
Mar 30, 2024

Conversation

kathleenhang
Copy link
Contributor

@kathleenhang kathleenhang commented Jan 11, 2024

Closes #902

@mwestphal
Copy link
Contributor

From: @kathleenhang :

I have done a keybind to "W" for cycling through the animations.

Perfect

CycleAnimationsFor{FILE_TYPE}() increments through the circular rotated array indices of {FILE_TYPE}

What is FILE_TYPE here ?

GetAnimationDescription() displays information for current animation keyframe

Ok

Animation options are set after "W" key is pressed

Ok

When a user imports their GLTF file into F3D, where does the data flow? I would assume it goes through vtkF3DGenericImporter which would contain a vtkGLTFReader. That would then allow me to access the data structure array containing each individual animation keyframe within the imported GLTF file. Finally, I could retrieve the file name property from each keyframe to display onto the frontend.

That depends. There is two ways to open a gltf file. through the vtkGLTFReader and through the vtkGLTFImporter.

In any case, this should not matter to you in the context of this implementation. You should work using the vtkImporter API directly. The fact that it is coming from a reader or an importer should not matter.

@kathleenhang
Copy link
Contributor Author

CycleAnimationsFor{FILE_TYPE}() increments through the circular rotated array indices of {FILE_TYPE}

What is FILE_TYPE here ?

Originally, "FILE_TYPE" was in reference to any file type which supports animations.

For example:

CycleAnimationsForEX2()
CycleAnimationsForE()
CycleAnimationsForEXO()
CycleAnimationsForG()
CycleAnimationsForGLTF()
CycleAnimationsForGLB()
CycleAnimationsForFBX()
CycleAnimationsForDAE()
CycleAnimationsForX()

However, thanks to your feedback, I learned that it is entirely unnecessary to differentiate!

@kathleenhang
Copy link
Contributor Author

I'm working on some code. So far what I have is:

  • Once the user presses "W", CycleAnimation() updates "scene.animation.index" with a new value.

I don't understand how to render the next animation onto the screen. self->Window.render() doesn't seem to work.

Therefore, I believe I need to reload the file with the updated animation index?

I would assume a new function would have to be created within animationManager.cxx, similar to the existing LoadAtTime(double timeValue). However, something like LoadAtAnimationIndex(int indexValue). Within this function, I could update the animation index property of the file (not sure how). Then update the renderer to allow the next animation to appear on the screen.

@mwestphal Help appreciated!

@mwestphal
Copy link
Contributor

Hi @kathleenhang

Indeed, adding a logic to enable and load a specific animation in animationManager is needed.
See animationManager.cxx:80 for how to select a specific animationIndex.

TBH I dont think you need a LoadAtAnimationIndex. Just a SelectAnimationIndex should be enough imo.

@kathleenhang
Copy link
Contributor Author

Thanks @mwestphal ! However, I am still a bit confused, and I will appreciate tips on loading.

How can I load a specific animation in animationManager after its animation index has been updated? I would assume that I would have to reinitialize the animationManager in order for it to read in the updated options. I could self->AnimationManager->Initialize(), but I'm unsure how I would fill in the parameters. I'm also unsure how or if reinitializing AnimationManager would affect self->Window.render(). self->Loader.loadScene() seems related.

@mwestphal
Copy link
Contributor

I would assume that I would have to reinitialize the animationManager in order for it to read in the updated options

You need to put the animation index selection into its own method and call it whenever needed, in Initialize and when pressing the key. You can then just call LoadAtTime.

@kathleenhang
Copy link
Contributor Author

@mwestphal Thanks, that was helpful! It's working now.

However, there is an issue with the animation cycling. Once the final animation of the file has been reached, animation cycling begins to break. Pressing "W" repeatedly shows the final animation, instead of the desired behavior of looping back to show the first animation again. The indices are all within bounds, so I believe there is an issue with properly managing and resetting animation states.

Also, I see log::debug(""); sprinkled around the code base. I am wondering how I can access the logs? At the moment, I'm running the app within terminal using cmake ../src followed by make within the project folder to build and run the app. I'm not sure how I can run the app within VSCode.

Help appreciated !

@Meakk
Copy link
Member

Meakk commented Jan 24, 2024

Also, I see log::debug(""); sprinkled around the code base. I am wondering how I can access the logs?

Just use the option --verbose when running f3d

@mwestphal
Copy link
Contributor

HI @kathleenhang

I've done a quick review pass. It's definitely not an easy one, good luck for the correction and thanks for your persistence!
Do not hesitate to come and ask on discord for faster feedback :)

@kathleenhang
Copy link
Contributor Author

Okay, it looks like the animation cycling feature has been successfully implemented. I'm planning to add some tests to cover the edge cases. Let me know what you think.

@mwestphal
Copy link
Contributor

Great! I will review.

In any case, you do need to add tests and also put that PR out of draft status so that CI is run.

@mwestphal
Copy link
Contributor

Also please "resolve" discussions that you have adressed.

@mwestphal
Copy link
Contributor

And also rebase on the last master :)

@mwestphal
Copy link
Contributor

I tried to answer your questions, if anything is unclear, do not hesitate to ask on discord: https://discord.f3d.app :)

@kathleenhang
Copy link
Contributor Author

  1. How can I reset the positions of the models? For instance, if I play "all animations" of ./bin/f3d ../src/testing/data/InterpolationTest.glb, then cycle onto a different animation, the positions of all the models will remain where the last animation left off. I would want all of their positions to be reset before playing the next animation. I think loadAtTime() is related?

  2. What kinds of tests are required? Some tests I'm planning to add (Actually, I might not know how to add some of these). Let me know what you think!

  • No animations, 1 animation
  • First animation, last animation
  • Cycles through every available animation
  • Correct animation name displayed
  • Pauses/Resumes properly
  • Proper timing of animation
  • Resumes properly after interruptions
  • Handles corrupted data

@mwestphal
Copy link
Contributor

How can I reset the positions of the models?

Let me check that, but it may be the responsability of the importers, hence unrelated to this change.

What kinds of tests are required?

No animations, 1 animation
First animation, last animation
Cycles through every available animation

Just looping through once should be enough.

Correct animation name displayed

Indeed

Pauses/Resumes properly

Already covered

Proper timing of animation

Already covered

Resumes properly after interruptions

I think already covered ?

Handles corrupted data

Already covered

I might not know how to add some of these

You need to look at interaction test, let me know if you need a pointers

@mwestphal
Copy link
Contributor

By the way, please rebase on master and Undraft your PR :)

@mwestphal
Copy link
Contributor

CI is not clean, you have styles and a segfaults to fix :)

@kathleenhang
Copy link
Contributor Author

@mwestphal I'm not sure how to fix the segfaults. I am guessing it is an issue with cycling out of index range? I tried to fix it with the last commit.

@kathleenhang
Copy link
Contributor Author

All the requested changes have been added !

@kathleenhang kathleenhang requested a review from mwestphal March 28, 2024 23:36
doc/CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@mwestphal mwestphal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

recover baselines from CI for the last failing tests

Copy link

codecov bot commented Mar 29, 2024

Codecov Report

Attention: Patch coverage is 96.55172% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 96.62%. Comparing base (340c56d) to head (e87d94c).
Report is 9 commits behind head on master.

Files Patch % Lines
library/src/animationManager.cxx 93.93% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1155      +/-   ##
==========================================
+ Coverage   96.44%   96.62%   +0.17%     
==========================================
  Files         146      147       +1     
  Lines        8746     8907     +161     
==========================================
+ Hits         8435     8606     +171     
+ Misses        311      301      -10     

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

@kathleenhang
Copy link
Contributor Author

@mwestphal Just in time for the release !

@kathleenhang kathleenhang requested a review from mwestphal March 30, 2024 14:24
@mwestphal mwestphal merged commit cd7f7e5 into f3d-app:master Mar 30, 2024
41 checks passed
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.

Cycle through animations in glTF files
4 participants