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

SkeletonUtils: Fix retargetClip() duration and last frame handling. #27653

Merged
merged 3 commits into from
Feb 1, 2024

Conversation

AlaricBaraou
Copy link
Contributor

@AlaricBaraou AlaricBaraou commented Jan 30, 2024

Fixed #25288

Description

Description

This PR addresses two main issues in the animation retargeting process:

Issue 1: Incorrect Delta Calculation

Problem: The calculation of the relation between numFrames and delta was incorrect.

Expected Behavior: For an animation lasting 2 seconds with 3 frames spaced evenly, the expected delta should be 1s.

  • Frame 0: 0s
  • Frame 1: 1s
  • Frame 2: 2s

For a given 1.5 FPS ( 3 frames / 2 seconds ) = 1.5
amount of frames = Duration ( 2s ) * 1.5 FPS = 3 frames
New Formula: The revised formula correctly calculates the delta:
duration (2s) / (amount of frames (3) - 1) => 1s
Old Formula: Previously, the formula incorrectly calculated the delta:
1 second / FPS (1.5) => 0.666666s
Impact: This incorrect delta calculation resulted in the animation retrieving the wrong part, leading to a shortened and trimmed appearance.

Issue 2: Incorrect Last Frame in Loop

Problem: In the final loop iteration, the last frame was identical to the first frame, instead of representing the state of the last frame of the original animation.

To illustrate, here is how a 3 frame animation loop back to its start position without that fix.
Before adding if ( i === numFrames - 2 )
https://github.com/mrdoob/three.js/assets/7174039/c2bb49ef-3810-4b53-b6d4-a0e413a8140f
After
https://github.com/mrdoob/three.js/assets/7174039/226ed358-bc8d-4d38-bdde-5354b25c5953

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 31, 2024

I've locally tested the change with the fiddle from #25288 (comment). These are the console outputs:

original duration: 4.924997806549072
webgl_loader_bvh.html:75 original track0 frames: 592
webgl_loader_bvh.html:79 retarget duration: 4.924997806549072
webgl_loader_bvh.html:80 retarget track0 frames: 591

The duration is now correct but it seems the new keyframes are missing one entry (591 vs. 592). I would expect matching keyframe length.

@AlaricBaraou
Copy link
Contributor Author

How did you calculate the FPS option?
I assume you used the existing method from the fiddle that give this result.
If so, can you try after following what I explained in this comment ?
#25288 (comment)
I used the same example yesterday and it was working at the condition of using the right option.fps value.

@AlaricBaraou
Copy link
Contributor Author

Also, like mentioned in this comment #25288 (comment) it could make sense to default to the original animation FPS to avoid this small alteration.
But maybe that would be considered a breaking change ?
Personally I think it's not really breaking, if that's fine with you, I can add this change too.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 31, 2024

If so, can you try after following what I explained in this comment ?

Ah, the keyframe length is now correct!

Also, like mentioned in this comment #25288 (comment) it could make sense to default to the original animation FPS to avoid this small alteration.

The new suggested default makes sense to me.

@AlaricBaraou
Copy link
Contributor Author

Sorry my previous commit would work in most cases but it's not guaranteed that the first track is using the most frames.
Some animation that don't update the scale have a length of 2 on those tracks while the others have more.

In the last commit we're certain to use the correct fps.
It's not very efficient / readable but that way it runs only if no fps was provided.

Copy link
Collaborator

@Mugen87 Mugen87 left a comment

Choose a reason for hiding this comment

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

Let's give this a try!

@Mugen87 Mugen87 merged commit edb3971 into mrdoob:dev Feb 1, 2024
11 checks passed
@Mugen87 Mugen87 changed the title SkeletonUtils: fix retargetClip duration and last frame SkeletonUtils: Fix retargetClip() duration and last frame handling. Feb 1, 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.

SkeletonUtils retargetClip is always off by 1 frame.
2 participants