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.

Sign up for GitHub

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

Jump to bottom

Removing redundant errors during animation mixer cache update #85368

Closed
wants to merge 1 commit into from
Closed

Removing redundant errors during animation mixer cache update #85368

wants to merge 1 commit into from

Conversation

dqanxy-umich
Copy link

No description provided.

@dqanxy-umich dqanxy-umich marked this pull request as ready for review November 25, 2023 23:03
@dqanxy-umich dqanxy-umich requested a review from a team as a code owner November 25, 2023 23:03
@dqanxy-umich
Copy link
Author

Hello, I'm not sure how to link an issue to this. This is a PR to resolve this issue: #85316

@dqanxy-umich dqanxy-umich mentioned this pull request Nov 25, 2023
Closed
@YeldhamDev YeldhamDev added this to the 4.3 milestone Nov 26, 2023
@YeldhamDev YeldhamDev linked an issue Nov 26, 2023 that may be closed by this pull request
Animation cache errors immediately after deleting node #85316
Closed
@YeldhamDev
Copy link
Member

A more detailed commit message is necessary, you can learn how to edit those here: https://docs.godotengine.org/en/stable/contributing/workflow/pr_workflow.html#modifying-a-pull-request

TokageItLab
TokageItLab requested changes Nov 26, 2023
View reviewed changes
scene/animation/animation_mixer.cpp Outdated Show resolved Hide resolved
scene/animation/animation_mixer.cpp Outdated Show resolved Hide resolved
@TokageItLab
Copy link
Member

I remember we have removed similar errors in the past, but I feel that removing too many errors against path checking is inconvenient because the user cannot notice what is wrong in cases where the animation does not work. Perhaps we should reconsider implementations such as #69480.

@dqanxy-umich
Copy link
Author

Added a proper commit message, let me know if there are anything I should add to it. Currently have only removed a redundant error message (there were two error messages every time the cache update ran into an invalid node, down to 1), I do not feel qualified to make a decision whether or not to remove the error entirely. I personally believe being told that a node doesn't exist every time the cache is updated can be confusing to a programmer, especially when it throws that error for animations that aren't in use. If possible, it would be nice to post an error only when running an animation that contains an invalid node, I have no clue how to implement this currently though.

@dqanxy-umich dqanxy-umich changed the title Update animation_mixer.cpp Removing redundant errors during animation mixer cache update Nov 27, 2023
@dqanxy
Removing redundant errors during animation mixer cache update
caf8159 
Prevent Animation Mixer from posting unnecessary errors when updating cache.

When a node is invalid/deleted, any animations that reference that node throws two errors. This occurs for every animation in the animation mixer (even if those animations aren't being played), every time the cache is updated (whenever any animation is started, for instance). This quickly leads to a ton of error messages.

This PR removes one of those error messages, the one for failing get_node by first doing a check with has_node_and_resource.

Fixes #85316
@TokageItLab
Copy link
Member

TokageItLab commented Nov 27, 2023 •
edited
Loading

I personally believe being told that a node doesn't exist every time the cache is updated can be confusing to a programmer, especially when it throws that error for animations that aren't in use. If possible, it would be nice to post an error only when running an animation that contains an invalid node, I have no clue how to implement this currently though.

I think it would be fine if AnimationPlayer could be checked at the play() function and AnimationTree could be checked at the timing of internal seek to 0 for each AnimationNode, but there are certainly use cases where path checking is not desired, so I think an option is needed apart from those modifications.

@TokageItLab TokageItLab mentioned this pull request Dec 29, 2023
Merged
@akien-mga
Copy link
Member

Superseded by #86608. Thanks for the contribution!

@AThousandShips AThousandShips removed this from the 4.3 milestone Feb 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TokageItLab TokageItLab TokageItLab requested changes

Assignees
No one assigned
Labels
archived bug regression topic:animation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Animation cache errors immediately after deleting node
6 participants
@dqanxy-umich @YeldhamDev @TokageItLab @akien-mga @AThousandShips @dqanxy