Gravity override of Area3D does not take transform into account

[lejar]

Godot version

4.0, 4.1

System information

Godot v4.1.stable - macOS 13.4.1 - Vulkan (Forward+) - integrated Apple M2 - Apple M2 (8 Threads)

Issue description

When applying a gravity with Area3D, any rotation to the area does not change the gravity vector. I am assuming this is a bug, because if you use the gravity point feature, that one takes the transform of the area into account (calculated in servers/physics_3d/godot_area_3d.cpp:GodotArea3D::compute_gravity; meaning the behavior between the two modes is inconsistent.

For some context as to how/why I'm using this: I have a rotating space station built out of segments and I want the gravity to be applied downwards in each segment. It would be trivial to just duplicate a single area and rotate it to each segment, but right now I have to calculate the correct downward vector, and if I start actually spinning the station that won't work.

I have a patch ready. I do not apply the scale to the gravity vector in my patch, but the case where a gravity point is specified does take the scale into account, I think. I would propose to additionally modify the gravity point mode to not take scale into consideration.

--- a/servers/physics_3d/godot_area_3d.cpp
+++ b/servers/physics_3d/godot_area_3d.cpp
@@ -327,9 +329,13 @@ void GodotArea3D::call_queries() {
 }
 
 void GodotArea3D::compute_gravity(const Vector3 &p_position, Vector3 &r_gravity) const {
+	// The value returns by get_gravity_vector is reused for the directional and point modes. When the gravity is
+	// specified with respect to a point, it is that point in local coordinate space. When the value is not a point,
+	// it represents the direction of the gravity vector in the local basis.
 	if (is_gravity_point()) {
 		const real_t gr_unit_dist = get_gravity_point_unit_distance();
 		Vector3 v = get_transform().xform(get_gravity_vector()) - p_position;
+		// The strength of the gravity falls off with distance to the specified point.
 		if (gr_unit_dist > 0) {
 			const real_t v_length_sq = v.length_squared();
 			if (v_length_sq > 0) {
@@ -338,11 +344,28 @@ void GodotArea3D::compute_gravity(const Vector3 &p_position, Vector3 &r_gravity)
 			} else {
 				r_gravity = Vector3();
 			}
+		// The strength of the gravity is the same in the whole area, it's just that the direction is with respect to
+		// the given point.
 		} else {
 			r_gravity = v.normalized() * get_gravity();
 		}
+	// The gravity value is just the gravity vector.
 	} else {
-		r_gravity = get_gravity_vector() * get_gravity();
+		r_gravity = get_transform().basis.xform(get_gravity_vector()).normalized() * get_gravity();
 	}
 }

Steps to reproduce

Create an Area3D with a gravity vector, and rotate the Area3D. Notice that the gravity vector is not transformed according to the Area3D rotation.

Minimal reproduction project

I don't think it's required. Problem is apparent from the diff in the description.

[AThousandShips]

Changing this breaks compat if it's not considered a bug, and I don't necessarily agree it is one

[lejar]

If the consensus is that this is not a bug per se, then I would be open to adding a new option to take transform into account as a feature (and I could implement it).

[AThousandShips]

Note that this was the way in 3.x as well (bullet and Godot physics), so not a bug but intended IMO

[lejar]

test_gravity.zip
I've added a simple test project for my incoming MR.

[rburing]

As mentioned in #79268 (comment), this issue could be worked around by copying the original gravity vector to a Vector3 variable on _ready, and applying the area transform to it whenever necessary, i.e. on _ready and whenever the area is transformed. If this is acceptable we could document it explicitly.

[AThousandShips]

Could someone test the performance and stability of that? Might there be some delay in synchronisation or similar?

Should be a simple workaround assuming it doesn't have any glitches

On the other hand the new feature is pretty simple, so I'm leaning towards supporting it, on the other hand I suspect it's not going to be used very much, since people hasn't run into this limitation seemingly before

[AThousandShips]

I think my position would be:
If the workaround works smoothly we should document it instead as this seems like a very edge case thing not necessarily justifying complicated code changes

Also should maybe be in _enter_tree or world unless transform update is called on entering the tree, will test this concept when I have time unless you try it first

Something like:

extends Area3D

var base_gravity_direction := Vector3(0, -1, 0)

...

func _notification(what: int):
    if what == NOTIFICATION_TRANSFORM_CHANGED:
        gravity_direction = global_transform.basis * base_gravity_direction

extends Area2D

var base_gravity_direction := Vector2(0, 1)

...

func _notification(what: int):
    if what == NOTIFICATION_TRANSFORM_CHANGED:
        gravity_direction = global_transform.basis_xform(base_gravity_direction)

[aaronfranke]

Aside from compatibility concerns, what is the use case for not taking the transform into account? I'm not sure I follow the logic that this is the intended behavior just because it was the same way in older Godot versions.

I suspect it's not going to be used very much, since people hasn't run into this limitation seemingly before

Yes, this is why I figured it would be acceptable to do in PR #90166, it's probably not used much. I have been doing advanced things with gravity only recently which is why I did not run into this before.

[AThousandShips]

What's the use case for taking the orientation of the area into account? I wouldn't personally expect it to be local

There doesn't seem to be much confusion or problems for people with this behavior, considering the reactions on this issue and lack of other reports, so people seem to not be bothered by it, but that implies they use the current behavior without issue

So either:

• People aren't rotating their areas with gravity, they wouldn't care either way
• Or they do, and account for it, they'd be negatively affected by this unless it's configurable

[aaronfranke]

but that implies they use the current behavior without issue

Or it means that they haven't tried to rotate an Area3D node with directional gravity.

I wouldn't personally expect it to be local

Point gravity is local, so it would make sense for directional to be the same.

What's the use case for taking the orientation of the area into account?

Imagine a Super Mario Galaxy style platform made of a body node (StaticBody3D/RigidBody3D/AnimatableBody3D/whatever) with 3 child nodes: a CollisionShape3D, MeshInstance3D, and Area3D. The Area3D node has gravity pointing down. The body node can be moved around freely and its children move along with it (since they keep the same local transform). Without taking the rotation into account, if you rotate the platform, its gravity will no longer work. You would need a workaround to make it work properly, like described in your / @AThousandShips's reply.

[AThousandShips]

Then that argues strongly in favor of having it configurable, right? Especially since assuming people aren't expecting the current behavior isn't a safe approach

[aaronfranke]

If it was made configurable, would it be removed for Godot 5.0 in a decade or so? I don't know what the use case is for having it not take into account the node transform.

Also note, if the goal is high configurability, I have PR #82878, which allows implementing custom gravity types in user code. Then anyone can implement whatever behavior they want. Local directional gravity would be as simple as this:

func _calculate_gravity_target(local_position: Vector3) -> Vector3:
	return local_position + Vector3.DOWN

The directional gravity use case is just one of many, the PR allows for users to implement any gravity type.

[AThousandShips]

I'd say that if we are to break compatibility we should first assess the actual user demands, there's little evidence for that either option is desired, given the lack of reports/support, breaking compatibility for a feature we don't know people demand isn't a good idea IMO, so either we should establish that people do want it, or have it be configurable

Configurability is in my opinion the best option for everyone, it doesn't harm anyone and it allows both options (and it's actually a bit more performant in the case people don't want it/care about it)

In this case I'd prefer a configurable situation like in #82878 instead of breaking compatibility

But the problem is this:

• There are little to no indication people demand this or expect it, given this report being the only one, and it and the linked PR having little support
• There's an unknown number of people, perhaps none, but probably not none, who rely on the current behavior

We should at least assess that