Expose a method to get gravity for any physics body

[aaronfranke]

This PR exposes a new method for getting gravity on physics bodies, and uses it in the script templates.

Implements godotengine/godot-proposals#8054 see the proposal for the justification.

Supersedes #83087 (but keeping it open for now, in case the internal code reorganization is helpful).

Minimal test project: character_gravity.zip (but replace "compute_gravity" with "get_gravity")

[RedMser]

Untested suggestions, but the C# template was inconsistent with gdscript.

[KoBeWi]

Tested the 2D part and it works correctly. The updated script template is a nice touch.

[akien-mga]

Thanks!

[mihe]

I was wondering why my review comments were going ignored, but apparently I had forgot to actually press the "Submit review" button. I guess I'll just dump my comments here, in case it's of any interest to anyone:

I believe using body_get_direct_state in a scene node like this is a bit of an anti-pattern, as you can then theoretically use this get_gravity method in _process and end up with the gravity of what should rightfully be for the next _process, since the physics server is ahead of _process by one tick.

You also can't actually use body_get_direct_state in the context of _process if you have the physics/*d/run_on_separate_thread project setting enabled, since it'll then throw an error due to not being in-sync with the physics server, as seen here:

godot/servers/physics_3d/godot_physics_server_3d.cpp

Lines 923 to 924 in 63d6bda

	PhysicsDirectBodyState3D *GodotPhysicsServer3D::body_get_direct_state(RID p_body) {
	ERR_FAIL_COND_V_MSG((using_threads && !doing_sync), nullptr, "Body state is inaccessible right now, wait for iteration or physics process notification.");

What you likely should be doing instead is to add this as a variable in the scene nodes (like RigidBody*D) and populate that variable in something like RigidBody*D::_sync_body_state when the physics server invokes that as part of the callback passed to body_set_state_sync_callback, which would effectively then cache the last relevant value, which you could then safely return from get_gravity.

You can see other examples of things being synchronized here:

godot/scene/3d/physics_body_3d.cpp

Lines 493 to 507 in 63d6bda

	void RigidBody3D::_sync_body_state(PhysicsDirectBodyState3D *p_state) {
	set_ignore_transform_notification(true);
	set_global_transform(p_state->get_transform());
	set_ignore_transform_notification(false);
	linear_velocity = p_state->get_linear_velocity();
	angular_velocity = p_state->get_angular_velocity();
	inverse_inertia_tensor = p_state->get_inverse_inertia_tensor();
	if (sleeping != p_state->is_sleeping()) {
	sleeping = p_state->is_sleeping();
	emit_signal(SceneStringNames::get_singleton()->sleeping_state_changed);
	}
	}

The problem with this approach is that you also want this new get_gravity method to be available in physics bodies like CharacterBody*D, which doesn't currently utilize the state synchronization, because its state is only ever "pushed" to the physics server and never "pulled".

What the solution to that problem would be isn't super clear to me. From what I've observed these state synchronization callbacks only get invoked when kinematic bodies actually move, and not continously, which would obviously be required for this accumulated gravity to be accurate, since you could have areas flying around all over the place (and affecting gravity) while the character is standing perfectly still.

I would also perhaps question why this new method is called get_gravity as opposed to get_total_gravity. It seems mildly useful to be able to disambiguate between global gravity and accumulated/total gravity. If nothing else it would make things consistent with the method found in PhysicsDirectBodyState*D.

[aaronfranke]

@mihe Thanks for looking into improving this! I think you're a lot more familiar with this code than I am, I'm inclined to go with your suggestions. Adding a variable in the scene nodes makes sense to me.

[mihe]

Note the caveat with regards to kinematic bodies though. CharacterBody*D lacks any sort of state synchronization right now, and while AnimatableBody*D (and frozen RigidBody*D) does have state synchronization it's only ever invoked when the body has a non-zero velocity, if I remember correctly. So while adding the variable is simple, actually populating it is likely not so simple.

This could all be changed of course, but the performance implications are not trivial. The state synchronization is already quite an expensive part of the whole physics machinery once you start adding a lot of physics bodies. I'm not sure what other alternatives there are though.

[mihe]

I only just now realized that this change encompasses StaticBody*D as well, which is arguably more problematic than the kinematic bodies. That particular body should probably not be doing any sort of state synchronization in any case.

[mihe]

you can then theoretically use this get_gravity method in _process and end up with the gravity of what should rightfully be for the next _process

I managed to create a repro for this desync issue in #87996, in case anyone wants to see it for themselves.

[aaronfranke]

@mihe Should we change the method to show an error when it's used outside of _physics_process or other physics methods?

[mihe]

I must admit I don't love the idea of having a method on the physics bodies that's only okay to use during _physics_process and _integrate_forces, but in the interest of pragmatism I suppose that would be the best near-term solution, so long as the error message (and documentation) makes things clear. There is some precedence already with get_contact_count, which similarly relies on body_get_direct_state, but I'll likely make a PR to rectify that one, since that one can (and should) rely on the state synchronization in RigidBody*D.

The only way of detecting whether this new method is safe to call would be to either expose some new getter on the physics server interface to get at what Godot Physics calls the doing_sync variable, as seen here, or to introduce some new variable/getter to the Engine class (like these) that says whether we're doing the synchronization or not, like is_doing_physics_sync() or is_in_physics_sync() or something.

I would probably opt for the Engine class thing myself, just to not add to the burden of implementing the physics server getter correctly from any potential physics server extensions out there.