Add a setting to control applying area transform to gravity #79268
Conversation
…hen calculating gravity.
akien-mga
changed the title
|
Thanks for the contribution! The physics team is assigned for review, but it's currently short on contributors, so it might take some time. Feel free to drop by the #physics channel on the Contributors Chat to discuss your proposal. For the record, your commit seems not to be linked to your GitHub account. See: Why are my commits linked to the wrong user? for more info. |
|
Should be added to the 2D physics as well for completeness IMO, if added |
|
Added the fact that this breaks compatibility by the default behaviour of the property, and it's change of behaviour with the point gravity if turned off Can be removed if a converter is added, but that feels convoluted, and it still breaks compatibility by behavior, changing the default behavior, unless it is to fix a buggy behavior, breaks compat |
AThousandShips
changed the title
AThousandShips
changed the title
AThousandShips
changed the title
…ot* a point. Also change transform to only apply the basis, so that translation is not applied to the gravity vector.
|
I've applied the requested changes to the 3D code. I'll work on making the same feature in the 2D code. |
|
Okay I've also gone ahead and implemented the same functionality into Area2D. I've updated my example project to include a 2D scene: |
|
The documentation has to be in alphabetical order, you get this automatically if you compile the code and run |
| @@ -90,6 +90,9 @@ | |||
| The distance at which the gravity strength is equal to [member gravity]. For example, on a planet 100 pixels in radius with a surface gravity of 4.0 px/s², set the [member gravity] to 4.0 and the unit distance to 100.0. The gravity will have falloff according to the inverse square law, so in the example, at 200 pixels from the center the gravity will be 1.0 px/s² (twice the distance, 1/4th the gravity), at 50 pixels it will be 16.0 px/s² (half the distance, 4x the gravity), and so on. | |||
| The above is true only when the unit distance is a positive number. When this is set to 0.0, the gravity will be constant regardless of distance. | |||
| </member> | |||
| <member name="gravity_apply_transform" type="bool" setter="set_gravity_apply_transform" getter="is_gravity_apply_transform" default="false"> | |||
| If [code]true[/code], gravity is transformed using the area's transform. In other words, rotating the area with this setting active will also rotate the gravity vector. | |||
There was a problem hiding this comment.
This implies that only rotation will apply, I think a more general wording is better
|
Correct me if I'm wrong, but if this feature proposal had an associated Godot Improvement Proposal, then the answer to
would be yes: store the original vector in a |
|
That is true, reasonable workaround, maybe add it to the issue and we can discuss that part more there |
| } else if (gravity_apply_transform) { | ||
| r_gravity = get_transform().get_basis().xform(get_gravity_vector()) * get_gravity(); |
There was a problem hiding this comment.
With this code, point gravity will always take the transform into account, but directional gravity will only take the transform into account when gravity_apply_transform is true. This seems like an odd inconsistency. However, making it consistent would require breaking behavior compatibility, which is what PR #90166 does.
There was a problem hiding this comment.
I'd say there's a valid use case for not applying transform to the direction, much less so for the point, if we are to break compatibility I'd argue it would be to make both controllable, not make the directional one local
Change for #79243
This change adds a new parameter to Area3D, which determines whether or not the gravity calculation of an area should take the area's transform into account.
The default behavior before this setting was to take the transform into account when using the gravity is a point setting, but not when using a simple vector to specify the gravity. The new default value will be true in both cases.
I still need to add some logic to the project upgrade functionality which should disable this setting in older projects for areas using a vector to specify gravity, so that this does not break compatibility, but I couldn't find where this upgrade is done (except the one which upgrades godot 3 -> 4). I would appreciate if someone could point me in the right direction for this.
test_gravity.zip
Here's a project containing the new setting showing how it works.