-
-
Notifications
You must be signed in to change notification settings - Fork 21.1k
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 mixer breaking animations with redundant check #88554
Fix animation mixer breaking animations with redundant check #88554
Conversation
…breaking animations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you look at the inside of get_node_and_resource()
, you can see that get_node()
is called in the second line.
Node *Node::get_node_and_resource(const NodePath &p_path, Ref<Resource> &r_res, Vector<StringName> &r_leftover_subpath, bool p_last_is_property) const {
ERR_THREAD_GUARD_V(nullptr);
Node *node = get_node(p_path);
This means that if the node is not found, an error will occur that cannot be controlled by check_path
, so the check is necessary. Perhaps some additional method for checking that does not target path needs to be implemented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I confirm that this fixes broken animations on https://github.com/gdquest-demos/godot-4-3d-third-person-controller and https://github.com/godotengine/tps-demo/
Warning: The Godot TPS demo has another rendering regression since 4.3-dev3, which makes crazily flashing lights, this is not epilepsy safe.
@TokageItLab's assessment seems correct, but the demos I mentioned do not seem to raise any |
Ok right, see #88428 (comment) which @TCROC linked in the OP. The difference is the value of the So you should just check |
diff --git a/scene/animation/animation_mixer.cpp b/scene/animation/animation_mixer.cpp
index 706d210064..eec70e737f 100644
--- a/scene/animation/animation_mixer.cpp
+++ b/scene/animation/animation_mixer.cpp
@@ -661,7 +661,7 @@ bool AnimationMixer::_update_caches() {
Ref<Resource> resource;
Vector<StringName> leftover_path;
- if (!parent->has_node_and_resource(path)) {
+ if (!parent->has_node(path)) {
if (check_path) {
WARN_PRINT_ED(mixer_name + ": '" + String(E) + "', couldn't resolve track: '" + String(path) + "'. This warning can be disabled in Project Settings.");
} This is a more minimal fix that should still prevent errors when |
@akien-mga I think it's a problem when resource is not found. I think it would be straighforward to add a Probably the most intelligent way is to add an optional argument to |
I wouldn't change |
Ah right, |
I would just add this change to this PR: diff --git a/scene/main/node.cpp b/scene/main/node.cpp
index e814e4f0cd..f827f68def 100644
--- a/scene/main/node.cpp
+++ b/scene/main/node.cpp
@@ -3025,9 +3025,9 @@ Array Node::_get_node_and_resource(const NodePath &p_path) {
Node *Node::get_node_and_resource(const NodePath &p_path, Ref<Resource> &r_res, Vector<StringName> &r_leftover_subpath, bool p_last_is_property) const {
ERR_THREAD_GUARD_V(nullptr);
- Node *node = get_node(p_path);
r_res = Ref<Resource>();
r_leftover_subpath = Vector<StringName>();
+ Node *node = get_node_or_null(p_path);
if (!node) {
return nullptr;
} And then I think it's good to go. It's probably reasonable for |
However, since path checking should be enabled by default (to detect errors for invalid path), the following code would be necessary. if (check_path && !parent->has_node(path)) {
WARN_PRINT_ED(mixer_name + ": '" + String(E) + "', couldn't resolve track: '" + String(path) + "'. This warning can be disabled in Project Settings.");
continue;
} |
@TokageItLab This is already handled by the next check:
|
@akien-mga Ah, make sense. So just replacing get_node to get_node_or_null is sufficient. |
Superseded by #88557 which adds the Thanks for the contribution! |
Absolutely! :) Glad to help! :) |
The animation mixer is breaking animations as described here: #88428
Shoutout to @monxa and @personalmountains for helping with the fix! Simply removing the incorrect and redundant check fixes it.
Incorrect reasoning here: #88428 (comment)
Redundant reasoning here: #88428 (comment)