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

Refactor post-step operations in Jolt module to be done as needed #101815

Merged
merged 1 commit into from
Jan 26, 2025

Conversation

mihe
Copy link
Contributor

@mihe mihe commented Jan 19, 2025

Fixes #101695.
Fixes #101721.

The general theme of this PR is to remove JoltObject3D::post_step, and instead replace whatever previously happened in JoltBody3D::post_step and JoltArea3D::post_step with more specific operations that are invoked as needed rather than for every active/awake body in the simulation on every physics step.

What mainly prompted this change was that JoltShapedObject3D::previous_jolt_shape was until now being cleared as part of JoltShapedObject3D::post_step, which worked fine prior to #100983 due to post_step always being invoked for every single body, sleeping or otherwise, but as of #100983 post_step will only be called for active/awake bodies. This meant that previous_jolt_shape would never be cleared for something like StaticBody3D, which is always sleeping, leading to the forced Area3D enter/exit events emitted by JoltContactListener3D::_flush_area_shifts to be queued up every single physics step as opposed to just once.

The solution to this is to add a new list in JoltSpace3D, called shapes_changed_list, that keeps track of any JoltShapedObject3D that have changed their shape since the last physics step, and use that list to clear all of their respective previous_jolt_shape after we've checked against it in JoltContactListener3D::post_step.

However, seeing as how this clearing of previous_jolt_shape was the only thing happening in JoltBody3D::post_step, we have no need to call it at all for bodies anymore. This meant that the only object being post-stepped was JoltArea3D, which currently uses it to check if there are any pending events and queue up the invocation of its JoltArea3D::call_queries for the next physics frame if there are, which upon closer inspection wasn't strictly necessary. We may just as well queue up JoltArea3D::call_queries as the individual events are being added instead, obviating the need for JoltArea3D::post_step (and thus also JoltObject3D::post_step) altogether.

Removing JoltArea3D::post_step also happens to resolve #101695, as JoltArea3D::post_step wasn't invoking the base implementation of JoltShapedObject3D::post_step, leading to its previous_jolt_shape never being cleared either.

Copy link
Member

@rburing rburing left a comment

Choose a reason for hiding this comment

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

This makes sense to me, and the code looks good.

I didn't understand how shapes were getting deallocated at first, but then I saw they are reference counted on the Jolt side.

@mihe mihe changed the title Refactor post-step operations in Jolt Physics module to be on-demand Refactor post-step operations in Jolt Physics module to be done on demand Jan 25, 2025
@mihe mihe changed the title Refactor post-step operations in Jolt Physics module to be done on demand Refactor post-step operations in Jolt module to be done as needed Jan 25, 2025
@mihe mihe force-pushed the jolt/no-post-step branch from caa25ef to a30410b Compare January 25, 2025 11:23
@mihe
Copy link
Contributor Author

mihe commented Jan 25, 2025

(That last push was just me changing the commit message to match the rephrased title.)

I didn't understand how shapes were getting deallocated at first, but then I saw they are reference counted on the Jolt side.

Yes!

For future reference, don't hesitate to ask if there's anything "architectural" (or even minor details) about the Jolt integration that you'd like me to explain in more detail. I realize some of the code in the Jolt module diverges from how Godot Physics does things, to varying degrees, and I certainly don't expect that every maintainer is going to be fluent in it right away.

I suppose I should ideally write up some technical documentation for it.

@Repiteo Repiteo merged commit e549802 into godotengine:master Jan 26, 2025
19 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Jan 26, 2025

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants