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 TrackCache conflict when tracks have same name but different type #86687

Conversation

TokageItLab
Copy link
Member

@TokageItLab TokageItLab commented Jan 1, 2024

Co-authored-by: Rémi Verschelde rverschelde@gmail.com

Using it could easily lead to use-after-free crashes, as the object may have been freed. Caching object pointers like this is unsafe. Instead, we rely only on the ObjectID and query ObjectDB with it each time we need access to the instance.

This PR is a salvaged version of #49411 with more proper solution. Merge Value and Bezier track into the same cache. Also, make the hash generated from those track types and track paths the key of the cache map instead of the only track path.

However, as I mentioned in #49411 (comment), the Bezier and Value Tracks have different paths in most cases, and will not blend unless the paths are manually matched. So I believe it will be necessary to rework Bezier tracks to match path name with Value Tracks and manage subpath in a different way from current in the future.

scene/resources/animation.cpp Outdated Show resolved Hide resolved
scene/animation/animation_mixer.h Outdated Show resolved Hide resolved
@TokageItLab TokageItLab force-pushed the AnimationMixer-remove-object-pointer branch from 1090488 to 573365c Compare January 1, 2024 16:13
@TokageItLab TokageItLab requested review from a team as code owners January 1, 2024 16:13
@TokageItLab TokageItLab force-pushed the AnimationMixer-remove-object-pointer branch from 573365c to c32cdc5 Compare January 1, 2024 16:14
@TokageItLab TokageItLab force-pushed the AnimationMixer-remove-object-pointer branch 2 times, most recently from 3f9b49d to c440f8b Compare January 1, 2024 16:15
@TokageItLab TokageItLab force-pushed the AnimationMixer-remove-object-pointer branch from c440f8b to 6ef2bfb Compare January 2, 2024 16:40
@akien-mga akien-mga added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Jan 2, 2024
@TokageItLab TokageItLab force-pushed the AnimationMixer-remove-object-pointer branch 9 times, most recently from 1b3c71a to 33a871d Compare January 5, 2024 17:35
Co-authored-by: Rémi Verschelde <rverschelde@gmail.com>
@TokageItLab TokageItLab force-pushed the AnimationMixer-remove-object-pointer branch from 33a871d to a51958a Compare January 8, 2024 12:48
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks great!

Confirmed that it fixes the MRP in #85572.

@akien-mga akien-mga merged commit a7e3474 into godotengine:master Jan 8, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@YuriSizov YuriSizov removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Jan 25, 2024
@YuriSizov
Copy link
Contributor

It breaks compatibility, so not suitable for cherry-picking.

@YuriSizov
Copy link
Contributor

We discussed before how this could be done without breaking compat (basically, public API would remain the same, but internally we'd use ObjectIDs instead). So this could be submitted as a backport without breaking compat in this manner, if desired.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment