-
-
Notifications
You must be signed in to change notification settings - Fork 22.5k
SoftBody3D: Support physics Interpolation #106863
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
base: master
Are you sure you want to change the base?
Conversation
Just to check, as I haven't looked through thoroughly, but are you handling the case where physics interpolation is off? Is a good start though. 👍 It would also be good to get some input from @godotengine/rendering whether they are happy with this being done CPU side or whether they prefer in a shader (particularly for large soft bodies). Granted the soft body itself is being calculated CPU side already, so that's probably the lions share of the bottlenecks. I personally think it would be fine CPU side especially to get started, as it's far less invasive. |
6fcc2f2
to
7f13876
Compare
It is handled in: godot/scene/3d/physics/soft_body_3d.cpp Line 322 in 7f13876
godot/scene/3d/physics/soft_body_3d.cpp Line 94 in 7f13876
|
7f13876
to
ab8c7e9
Compare
Sorry BTW if it sounds like there are a lot of changes, you are doing great here. 👍 |
ab8c7e9
to
42be62c
Compare
@@ -470,12 +556,12 @@ void SoftBody3D::_prepare_physics_server() { | |||
mesh_rid = mesh->get_rid(); | |||
} | |||
PhysicsServer3D::get_singleton()->soft_body_set_mesh(physics_rid, mesh_rid); | |||
RS::get_singleton()->connect("frame_pre_draw", callable_mp(this, &SoftBody3D::_draw_soft_mesh)); | |||
set_process_internal(true); | |||
set_physics_process_internal(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to be using process_internal
and physics_process_internal
when physics interpolation is off?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
physics_process
is always required, while process
may be not. I have modified to upload mesh within process
only when physics interpolation is on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Existing implementation doesn't rely on physics_process()
though when physics interpolation is OFF, updates seem handled by the physics already.
Usually it can be a good idea to avoid adding NOTIFICATION_INTERNAL_PHYSICS_PROCESS
unless necessary, as the machinery isn't all that efficient, and it allows the physics to e.g. sleep when the soft body is no longer moving, or turn off when required.
It's fine to make this dependency when physics interpolation is ON, but am a little uneasy about adding unnecessary restriction when physics interpolation is OFF.
Golden rule I'm trying to go by as we roll out FTI, is not to make existing stuff slower (and hopefully not make existing code get over-complicated by the addition). It's not fair on users and maintainers who aren't using physics interpolation is my thinking.
Maybe like keeping the old commit_changes()
(that is a NOOP when physics interpolation is on) and adding a new one for physics interpolation, something like that.
Other than that, this is looking great. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why GitHub didn't mark the changes discussed here as outdated. I have disabled process
when physical interpolation is off. It shouldn't be slower than before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It did, what I am questioning is whether it should have to use INTERNAL_PHYSICS_PROCESS
at all when physics interpolation is off, the current code doesn't require this notification.
(sorry my fault I typed the wrong notification in previous post, have now corrected.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current code used frame_pre_draw
signal, which is similar to process
I think. We use notifications in most places, is there any reasons to use frame_pre_draw
instead of physics_process
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hadn't noticed that .. and was assuming it was being triggered entirely from the physics.
But now you have mentioned it 😁 :
frame_pre_draw
will occur before the frameNOTIFICATION_INTERNAL_PHYSICS_PROCESS
occurs before the physics tick
This means with higher tick rate than frame rate, it will be updating the soft mesh more than before (and more than necessary) I think?
The other thing is that frame_pre_draw
occurs at a different point than INTERNAL_PROCESS
(pre-empting if you wondering about changing it to that). INTERNAL_PROCESS
happens afaik before _process()
so users can still move an Object during the frame process, and with the old system, it will still be reflected on that frame.
If you changed it from PHYSICS_PROCESS
to PROCESS
this behaviour would change. That's a sacrifice that's fine for physics interpolation but we don't want to change existing behaviour.
Also could be important in the light of #86808 (which I just saw this morning). If indeed the physics needs a sync then where this is called could be important.
792f959
to
2df3d91
Compare
2df3d91
to
0b7209c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally, it works as expected. Code looks good to me.
Testing project: test_softbody_point_radius.zip
Example at 20 ticks per second:
Physics interpolation disabled
softbody_20_tps_no_interpolation.mp4
Physics interpolation enabled
softbody_20_tps_with_interpolation.mp4
Of course, SoftBody simulation quality greatly varies depending on the physics tick rate. In practice, you'll want to use 30 tps or more for cloth whenever possible.
Example at 1 tick per second so you can see it in more detail 🙂
softbody_1_tps_with_interpolation.mp4
During the first tick, all SoftBodies are positioned at the global world origin. They move to the correct position on the second physics tick. Is there a way we could avoid this?
Closes godotengine/godot-proposals#12448.
This PR interpolates the mesh buffer on CPU ((similar to MultiMesh, which is also CPU interpolation)
FTI off
00.mp4
FTI on
01.mp4
jolt-softbody-fti.zip