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] Store ObjectID instead of Object * for Shape Owners #57183

Merged
merged 1 commit into from
Jan 26, 2022

Conversation

timothyqiu
Copy link
Member

3.x version of #57182

@lawnjelly
Copy link
Member

lawnjelly commented Jan 25, 2022

Codewise it looks ok to me, so there is mainly just any performance implications to look at, I saw the ObjectDB is using a hashmap and don't know how fast it is. I guess it would be good to check nothing here is getting hit in a hot loop in e.g. physics, as there might be an alternate strategy if that were the case.

shape_owner_get_transform() doesn't seem to be used outside the editor from core (but is bound).
shape_owner_get_owner() is used from

KinematicCollision2D::get_local_shape
KinematicCollision2D::get_collider_shape

KinematicCollision::get_local_shape
KinematicCollision::get_collider_shape

I'm not absolutely sure on this, it looks like they are just bound so probably ok (maybe a physics guy could confirm this, or at least a 2nd pair of eyes).

I'm also having no idea how fast Hashmap is, if it is O (1) then then should be fine. I'm kind of not sure why it is a Hashmap rather than a Handle Pool, maybe the IDs of Godot objects increasing is required for some reason.

@timothyqiu
Copy link
Member Author

I'm also having no idea how fast Hashmap is, if it is O (1) then then should be fine.

Hashmap is O(1) if there is no hash collision. When there is, it'll go through a linear search among entries that have the same hash value.

Copy link
Member

@lawnjelly lawnjelly left a comment

Choose a reason for hiding this comment

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

As far as I can see is ok. I can't see any evidence of it being performance sensitive.

@akien-mga akien-mga merged commit 6b68d2f into godotengine:3.x Jan 26, 2022
@akien-mga
Copy link
Member

Thanks!

@timothyqiu timothyqiu deleted the shape-owner-3.x branch January 26, 2022 12:28
@akien-mga
Copy link
Member

Cherry-picked for 3.4.3.

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.

3 participants