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 crash when clicking on "Interpolation Mode" with nonexistent node path #81779

Conversation

RealMadvicius
Copy link
Contributor

@RealMadvicius RealMadvicius commented Sep 17, 2023

#81769 AnimationPlayer: Editor crashes when clicking on "Interpolation Mode" with nonexistent node path

  • adding a nullptr check on a Node pointer obtained from get_node(NodePath) in case it is null now we wont execute the next instruction

ClassDB::get_property_info(nd->get_class(), prop, &prop_info);

Which then prevents the crash and leave the PropertyInfo prop_info blank default, so when we reach next instruction

bool is_angle = prop_info.type == Variant::FLOAT && prop_info.hint_string.find("radians") != -1;

we will have prop_info.type = NIL and
prop_info.hint_string.find("radians") will return -1 since it is empty

So is_angle will be set to false and the code can continue to execute normaly without any crash

@fire fire changed the title issue reference [https://github.com/godotengine/godot/issues/81769] Fix crash when clicking on "Interpolation Mode" with nonexistent node path Sep 17, 2023
@fire fire requested a review from a team September 17, 2023 02:16
@RealMadvicius RealMadvicius force-pushed the fix/4.2/81769_animationplayer_crash branch from 31c7c07 to ab68f6a Compare September 17, 2023 02:48
@RealMadvicius
Copy link
Contributor Author

Normaly the typo is fixed to respect the clang format, lets cross fingers

@RealMadvicius RealMadvicius force-pushed the fix/4.2/81769_animationplayer_crash branch 2 times, most recently from 21ca76d to f2596c7 Compare September 17, 2023 12:35
Copy link
Contributor

@lyuma lyuma left a comment

Choose a reason for hiding this comment

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

I think it makes a lot of sense to do this check. This code generally doesn't touch nodes within the scene

This is the only place in the code that does either call to get_node() which is a big warning flag, so we need to be extra safe in this part of the code.

Your change is good, and I propose one additional check to be extra safe on top of what you already did.

editor/animation_track_editor.cpp Outdated Show resolved Hide resolved
@RealMadvicius RealMadvicius force-pushed the fix/4.2/81769_animationplayer_crash branch from f2596c7 to 7a1deec Compare October 7, 2023 09:09
editor/animation_track_editor.cpp Outdated Show resolved Hide resolved
editor/animation_track_editor.cpp Outdated Show resolved Hide resolved
editor/animation_track_editor.cpp Outdated Show resolved Hide resolved
@RealMadvicius RealMadvicius force-pushed the fix/4.2/81769_animationplayer_crash branch 2 times, most recently from a126109 to 2d0f6be Compare October 7, 2023 09:21
@AThousandShips
Copy link
Member

AThousandShips commented Oct 7, 2023

Please update your commit title to be more descriptive, the title of the PR is good

Also remove the "fixed conflict" part

… path

issue reference [godotengine#81769]
godotengine#81769 AnimationPlayer: Editor crashes when clicking on "Interpolation Mode" with nonexistent node path

- adding a nullptr check on a Node pointer obtained from get_node(NodePath) in case it is null now we wont execute the next instruction

> ClassDB::get_property_info(nd->get_class(), prop, &prop_info);

Which then prevents the crash
@RealMadvicius RealMadvicius force-pushed the fix/4.2/81769_animationplayer_crash branch from 2d0f6be to e7a35d1 Compare October 7, 2023 09:34
@RealMadvicius
Copy link
Contributor Author

I have edited the commit title to correspond to the PR title as asked

@akien-mga akien-mga added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Oct 9, 2023
@akien-mga akien-mga merged commit c4effea into godotengine:master Oct 9, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@RealMadvicius RealMadvicius deleted the fix/4.2/81769_animationplayer_crash branch October 9, 2023 16:29
@harrisyu
Copy link
Contributor

With this commit, we found a bug, after you moved a node, animationplayer lost it's path. does it execute when node in / out of tree?

@YuriSizov
Copy link
Contributor

@harrisyu Please report your bug as a new issue with a reproduction project attached.

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

Cherry-picked for 4.1.4.

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

Successfully merging this pull request may close these issues.

AnimationPlayer: Editor crashes when clicking on "Interpolation Mode" with nonexistent node path
6 participants