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

[3.x] Fix physics on_floor_body crash #88946

Merged
merged 1 commit into from
Apr 16, 2024

Conversation

lawnjelly
Copy link
Member

@lawnjelly lawnjelly commented Feb 28, 2024

Physics body previously stored the RID of a collision object and accessed it on the next frame, leading to a crash if the object had been deleted. This PR stores the ObjectID in addition to the RID, and checks the object still exists prior to access.

Fixes #74732
Alternative to #77616

Notes

  • This is far simpler conceptually to [3.x] Add RIDReferences #77616 and will be a more familiar pattern, and is thus easier to review (for non-core maintainers).
  • The main disadvantage versus the proposed RID references is efficiency, ObjectDB::get_instance() used in this PR involved a lock and a hash table lookup, whereas RID references requires no lock and is very fast O(1).
  • My own personal preference is usually to go for efficiency in areas such as physics, but I do understand if others prefer this simpler approach. I haven't profiled and it is likely in most games with small numbers of these objects there won't be a great difference between the two approaches in terms of performance.
  • Like [3.x] Add RIDReferences #77616 this assumes a single threaded pattern. If another thread deletes the object between checking the ObjectDB and use, then all bets are off.

Physics body previously stored the RID of a collision object and accessed it on the next frame, leading to a crash if the object had been deleted.
This PR stores the ObjectID in addition to the RID, and checks the object still exists prior to access.
@lawnjelly lawnjelly added this to the 3.6 milestone Feb 28, 2024
@lawnjelly lawnjelly requested a review from a team as a code owner February 28, 2024 08:08
@akien-mga akien-mga changed the title [3.x] Fix physics on_floor_body crash [3.x] Fix physics on_floor_body crash Feb 29, 2024
@lawnjelly
Copy link
Member Author

@fabriceci would you be able to review this too?

It is essentially the same as #88947 in master.

Copy link
Member

@timothyqiu timothyqiu left a comment

Choose a reason for hiding this comment

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

LGTM. I guess the same could be done to the 2D version as well?

@lawnjelly
Copy link
Member Author

I guess the same could be done to the 2D version as well?

Absolutely, I must have missed this concentrating on the initial bug report, it looks identical. I might as well do in a separate PR though as this is already approved.

@lawnjelly lawnjelly merged commit ea68c2b into godotengine:3.x Apr 16, 2024
13 checks passed
@lawnjelly lawnjelly deleted the fix_physics_on_floor_body branch April 16, 2024 14:02
@lawnjelly
Copy link
Member Author

Thanks!

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.

2 participants