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

Cache value of Jolt Physics project setting bounce_velocity_threshold #101237

Merged
merged 1 commit into from
Jan 7, 2025

Conversation

mihe
Copy link
Contributor

@mihe mihe commented Jan 7, 2025

Currently a number of Jolt-related project settings are being cached after their initial read, due to being used in potentially hot codepaths where the overhead and/or thread synchronization from ProjectSettings::get_setting_with_override and StringName may be detrimental to performance.

This pull request makes that same change to the physics/jolt_physics_3d/simulation/bounce_velocity_threshold project setting, which is currently being read in JoltContactListener3D::_try_add_contacts (see here) which is potentially executed in parallel from multiple threads during the simulation step, and as such would likely suffer from the thread synchronization mentioned above.

I'll admit I haven't profiled this, but it's a small and risk-free change either way.

(This is also already happening in the Godot Jolt extension, where every project setting is cached on first read, in case there's any worry about correctness.)

@akien-mga
Copy link
Member

akien-mga commented Jan 7, 2025

With the ProjectSettings.settings_changed signal, you could likely make it so that all Jolt settings are cached on start, and re-read only when settings change, which would still allow modifying them at runtime.

Instead of having getters that are just aliases for querying project settings, they could get the cached value / be direct accesses to a struct with all settings as members.

There's somewhat of a pattern with that signal in some editor plugins already:

ProjectSettings::get_singleton()->connect("settings_changed", callable_mp(this, &CanvasItemEditor::_project_settings_changed));

You can also check ThemeCache structs for reference (based on NOTIFICATION_THEME_CHANGED, but the idea is the same).

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Shouldn't hurt, but as I pointed out this could likely be improved further.

@akien-mga akien-mga merged commit 0c76360 into godotengine:master Jan 7, 2025
20 checks passed
@akien-mga
Copy link
Member

Thanks!

@mihe
Copy link
Contributor Author

mihe commented Jan 7, 2025

With the ProjectSettings.settings_changed signal, you could likely make it so that all Jolt settings are cached on start, and re-read only when settings change, which would still allow modifying them at runtime.

That does sound a lot cleaner, and should be a quick enough change. I'll throw together a PR for it.

Thanks for the tip!

@mihe mihe deleted the jolt/cache-bounce-velocity branch January 7, 2025 22:54
@mihe
Copy link
Contributor Author

mihe commented Jan 8, 2025

I went ahead and made the suggested change in #101254.

I set the milestone to 4.x, since it seemed mildly risky in terms of amount of files touched, but the change was pretty mechanical, so I would say it's probably safe 4.4 as well.

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