godot
godot copied to clipboard
godotengine
Avoid error spamming in animation_tree when path is not found
For this issue: https://github.com/godotengine/godot/issues/65608
Error spamming was bringing Godot (and eventually the whole system) to a crawl by spamming an error message when there was an error finding a path.
I fixed this by skipping the error message (the error is also logged somewhere else outside of the loop)
Bugsquad edit:
- Fix #65608
Use the clang-format tool to fix the static check failure. Squash the resulted commits after that.
Should not hide errors. Instead, the cause of the error should be resolved.
Should not hide errors. Instead, the cause of the error should be resolved.
The cause is if an animation is broken (missing nodes). User error. It is still logged before the loop
"Cache failure" and "node not found" are not equivalent, and they should output separate errors. If you want to avoid spamming, for example, you should consider using ERR_PRINT_ONCE. However, even in that case, the output should be for all broken casts. E.g. if the first and fifth caches are corrupt, but you only output an error for the first cache, it is the wrong implementation.
I might be missing something... but you first get a "node not found" error and then a "track not resolved" error for each missing node/track.
ERROR: Node not found: "Root/Skeleton3D:root.x" (relative to "/root/@@16122/@@633/@@634/@@643/@@645/@@651/@@657/@@658/@@659/@@681/@@682/@@721/MainScreen/@@9499/@@9328/@@9329/@@9330/@@9331/@@9332/@@9333/Player/Root"). at: get_node (scene/main/node.cpp:1356)
ERROR: AnimationTree: 'Player/Idle', couldn't resolve track: 'Root/Skeleton3D:root.x' at: _update_caches (scene/animation/animation_tree.cpp:587)
Then you get a check in process_graph() to check if the track has an assigned path (which it doesn't) and so you get this error (again) every time process_graph() is called. A better solution might be to not add that track to the graph at all.
The timing of update_cache is very limited. Therefore, considering the possibility that the cache may break for some reason, this error should not be removed.
There have already been reports (#39926, #59549) of problems with too few occurrences of update_cache apart from #65608. So we should give priority to fixing those first. When properly cached, errors occur less frequently. And yes, if a path cannot be solved, it should be removed at the time of caching. Anyway, it is EVIL to resolve an error by making it so that doesn't display the error.
I looked into AnimationPlayer, and it seems to be implemented similarly to this workaround. So avoiding this error may not be a problem. Very sorry. However, this fix alone does not completely solve the problem, so I have sent an alternative PR as #69480.
As I described #69480, even if this error is avoided, spamming of one frame path errors may occur immediately after playback.
However, it is better than having an error every frame, and since avoiding this error provides the same behavior as the AnimationPlayer, I reconsidered this as not a problem. I am sorry if my above comment scared you.
However, there is basically always some reason for the error output and it needed a very careful decision to remove it. Don't be afraid of this and I would be happy to send you another PR in the future :)
Great! I was just looking at your solution from the other PR. Definitely way beyond my current knowledge! I could fix it in my own project by deleting the references to the missing armatures. But that doesn't solve the underlying problem. I will keep an eye out for other PRs... I think I need some time to look more into the codebase. But I'm still sticking around