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

Fix crash on RigidBody _direct_state_changed (3.x) #47484

Merged
merged 1 commit into from
Apr 23, 2021
Merged

Fix crash on RigidBody _direct_state_changed (3.x) #47484

merged 1 commit into from
Apr 23, 2021

Conversation

rafallus
Copy link
Contributor

Fixes: #46003 for the 3.x branch

@rafallus rafallus requested a review from a team as a code owner March 30, 2021 06:42
@akien-mga akien-mga added this to the 3.3 milestone Mar 30, 2021
@rafallus rafallus requested a review from a team as a code owner April 7, 2021 22:52
@rafallus
Copy link
Contributor Author

rafallus commented Apr 7, 2021

The input is now checked in all classes with _direct_state_changed method (including 2D nodes). As @madmiraal pointed out in #47485 (review) there are other classes than RidigBody using this method

@rafallus
Check if _direct_state_changed() argument is valid
e075b6b 
- Modified classes: RigidBody, PhysicalBone, VehicleBody, RigidBody2D, KinematicBody2D
- The input argument is untrusted even in release mode
@pouleyKetchoupp pouleyKetchoupp requested a review from a team April 8, 2021 15:21
pouleyKetchoupp
pouleyKetchoupp approved these changes Apr 8, 2021
View reviewed changes
Copy link
Contributor

@pouleyKetchoupp pouleyKetchoupp left a comment

Choose a reason for hiding this comment

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

This looks like the proper fix for 3.x since we can't use a callable to avoid exposing the method on that branch.

@akien-mga akien-mga modified the milestones: 3.3, 3.4 Apr 14, 2021
@akien-mga
Copy link
Member

How much of an optimization are we trading off here by removing the "trust it" release path for RigidBodies?

I believe this could also work with just the ERR_FAIL_COND added, but maybe the cost of the cast done (so far) in debug is negligible?

@pouleyKetchoupp
Copy link
Contributor

@akien-mga I can't find a measurable difference. It looks like it's negligible compared to propagating the transform, and in general the bottleneck will be somewhere else in cases with lots of rigid bodies in the scene, so it should be fine.

It makes sense to be on the safe side, since the function is still exposed to scripts on 3.x.

@akien-mga akien-mga merged commit e572be0 into godotengine:3.x Apr 23, 2021
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

Cherry-picked for 3.3.1.

@rafallus rafallus deleted the fix/rigidbody-crash3.x branch May 2, 2021 04:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pouleyKetchoupp pouleyKetchoupp pouleyKetchoupp approved these changes

Assignees
No one assigned
Labels
bug crash topic:physics
Projects
None yet
Milestone
3.4
Development

Successfully merging this pull request may close these issues.
3 participants
@rafallus @akien-mga @pouleyKetchoupp