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

Fix animation compression going the wrong way #97290

Merged
merged 1 commit into from
Sep 25, 2024

Conversation

fire
Copy link
Member

@fire fire commented Sep 21, 2024

On the compression page break breaks the animation rotates wrongly.

Fixes: #96882

See also https://stackoverflow.com/a/47190699/381724 discussion about rint() and nearbyint(). Math::fast_ftoi uses rint which appears to be using FE_TONEAREST.

See https://stackoverflow.com/questions/311696/why-does-net-use-bankers-rounding-as-default about discussion about banker's rounding which is FE_TONEAREST.

@fire fire requested review from a team and TokageItLab September 21, 2024 17:33
@fire
Copy link
Member Author

fire commented Sep 21, 2024

Video of the mrp of animation compression with the fix applied.

https://youtu.be/b5wAQqX9XsM

Copy link
Member

@TokageItLab TokageItLab left a comment

Choose a reason for hiding this comment

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

It is fine as far as testing with MRP.

The pointing out that there is a precision problem (slight errors due to differences in algorithms) when truncating by int makes sense as the cause of the problem.

@TokageItLab TokageItLab added bug topic:import topic:animation cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Sep 22, 2024
@TokageItLab TokageItLab added this to the 4.4 milestone Sep 22, 2024
@fire
Copy link
Member Author

fire commented Sep 22, 2024

I'll rewrite the commit message to be slightly better.

@fire fire force-pushed the fix-animation-compression branch 2 times, most recently from fdae966 to f32d87c Compare September 22, 2024 02:19
When compressing animation key frame indices the truncation breaks the animation near the border of pages.

We use banker's rounding (FE_TONEAREST) as implemented by fast_ftoi to get the nearest integer frame.
@fire fire force-pushed the fix-animation-compression branch from f32d87c to dd9525b Compare September 22, 2024 02:20
@TokageItLab
Copy link
Member

TokageItLab commented Sep 22, 2024

I think this is the right way to go, since it seems right to use round(nearest) at least for bit sampling, and this PR is appropriate as Math::fast_ftoi() is faster than Math::round().

However, note that this fix eliminates glitching, it still causes the stutter in the same as Math::round() as I commented in #96882 (comment), and there are still areas that could be improved.

stutter.mp4

For example, it might be possible to do something like blending the prev page's last few bits and next page's first few bits when decoding to make it smooth like anti-aliasing.

Or it may simply be that there is still a boundary error lurking somewhere, as I commented in there.

@fire
Copy link
Member Author

fire commented Sep 22, 2024

Would it work to use a cubic interpolation for blending the prev page's last few bits and next page's first few bits?

@TokageItLab
Copy link
Member

Since this stutter does not occur unless we use a round()(or fast_ftoi()), I would expect that it is a boundary value error due to some boundary being implemented with assuming to floor truncating, but I do not know where that is.

@akien-mga
Copy link
Member

Should this be merged as is, or should some more work be done first to work on the other problem identified?

@TokageItLab
Copy link
Member

There is definitely still work to be done, but IMO the stutter still looks better than the glitch, so I think it is acceptable to merge this PR and postpone the work to eliminate the stutter in the later.

@akien-mga akien-mga merged commit 65c94ec into godotengine:master Sep 25, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release topic:animation topic:import
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Animation compression has incorrect rotation
3 participants