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

Improve Skeleton3D::find_bone() performance #77307

Merged
merged 1 commit into from May 23, 2023

Conversation

44zb
Copy link
Contributor

@44zb 44zb commented May 21, 2023

Indexing Skeleon3D bones using HashMap

results of the script from #75915 before/after this fix
before
Time find_bone(): 1055
Time Dictionary: 59

After
Time find_bone(): 65
Time Dictionary: 57

Fix #75915

@44zb 44zb requested a review from a team as a code owner May 21, 2023 13:31
@smix8 smix8 added this to the 4.1 milestone May 21, 2023
@fire fire requested a review from a team May 22, 2023 11:27
scene/3d/skeleton_3d.cpp Outdated Show resolved Hide resolved
scene/3d/skeleton_3d.cpp Outdated Show resolved Hide resolved
scene/3d/skeleton_3d.cpp Outdated Show resolved Hide resolved
@44zb 44zb force-pushed the skeleton-find-bone-performance branch from 4d3bd63 to bbe292c Compare May 23, 2023 14:33
@44zb 44zb requested a review from kleonc May 23, 2023 14:35
Copy link
Member

@kleonc kleonc left a comment

Choose a reason for hiding this comment

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

@44zb LGTM besides the PR/commit title. No need for these to link to the issue being fixed (#75915)(it could be done in the PR/commit description). Also more human-readable titles are preferred. So I suggest changing both PR title and commit title (first line of the commit message) to e.g. "Improve Skeleton3D::find_bone() performance".

Also seems like there's some formatting issue, should be addressed too (see the failing test)(Yeah, I suggested it like that, oops! 🙃).

@44zb 44zb changed the title Skeleton3D::find_bone() performance #75915 Improve Skeleton3D::find_bone() performance #75915 May 23, 2023
@44zb 44zb changed the title Improve Skeleton3D::find_bone() performance #75915 Improve Skeleton3D::find_bone() performance May 23, 2023
@44zb 44zb force-pushed the skeleton-find-bone-performance branch from bbe292c to 8cccf7f Compare May 23, 2023 14:46
@kleonc
Copy link
Member

kleonc commented May 23, 2023

@44zb Oh, you actually still haven't changed the commit message, it's still "Skeleton3D::find_bone() performance #75915". You can change it when rebasing by using -r/--reword for the given commit.

@44zb 44zb force-pushed the skeleton-find-bone-performance branch from 8cccf7f to f645eee Compare May 23, 2023 15:03
@44zb
Copy link
Contributor Author

44zb commented May 23, 2023

@kleonc yes, sorry missed the commit part. But now it's all good :)

@fire
Copy link
Member

fire commented May 23, 2023

As long as this works for all the input cases and the performance is faster I'm fine with it.

I don't think this portion of the code requires special memory allocation strategies like not allocating.

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Looks great!

@akien-mga akien-mga changed the title Improve Skeleton3D::find_bone() performance Improve Skeleton3D::find_bone() performance May 23, 2023
@akien-mga akien-mga merged commit 4e9e5e8 into godotengine:master May 23, 2023
13 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@YuriSizov
Copy link
Contributor

Cherry-picked for 4.0.4 with #77874.

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.

Skeleton3D find_bone() is too slow
7 participants