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

look_at() doesn't work with RigidBody3D #83412

Closed
naptalky opened this issue Oct 15, 2023 · 10 comments · Fixed by #84799
Closed

look_at() doesn't work with RigidBody3D #83412

naptalky opened this issue Oct 15, 2023 · 10 comments · Fixed by #84799

Comments

@naptalky
Copy link

Godot version

4.2 beta 1

System information

Windows 11, Godot v4.2.beta1 - Windows 10.0.22621 - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 2070 SUPER (NVIDIA; 31.0.15.3742) - Intel(R) Core(TM) i7-9700K CPU @ 3.60GHz (8 Threads)

Issue description

I have a top-down minigame where I use look_at() in _integrate_forces() to rotate my RigidBody3D player character towards the mouse position. I noticed that in 4.2 beta 1 the code doesn't work. No errors, no warnings, the Vector3 I'm feeding it is just fine. The rest of the code works as expected. Did I miss something, or is it broken in the beta?

Tried using it in 4.1.2 and it works just fine.

Steps to reproduce

Just make a plane mesh with collisions as a ground. Add a RigidBody3D to the scene, lock rotation on X and Z axes, add it a script. Add _integrate_forces() with look_at(Vector3(0, position.y, 0)) inside. It should be rotating towards (0, position.y, 0) coordinates, but it doesn't. I suggest adding a front-facing rod mesh to visually see what's happening.

Minimal reproduction project

42_beta_testing.zip

@Calinou
Copy link
Member

Calinou commented Oct 15, 2023

Can you reproduce this in any of the 4.2 development builds? You can download them here.

@naptalky
Copy link
Author

naptalky commented Oct 15, 2023

@Calinou It does work on dev 1 and 2. Doesn't work on all the next versions.

Note: The reproduction project I uploaded has the player's "rod" facing Z-positive, while it should be Z-negative, hence the rod is pointing away from the green pillar (target). My mistake, made it in hurry.

@akien-mga akien-mga added this to the 4.2 milestone Oct 16, 2023
@akien-mga
Copy link
Member

CC @aaronfranke @TokageItLab

@Biokastin
Copy link
Contributor

Biokastin commented Oct 31, 2023

git bisect tracked it to commit c11825686589696f4c03948c11068a30c6c91796 "added state sync after call to _integrate_forces".

@akien-mga
Copy link
Member

That's #79977, CC @Owl-A @godotengine/physics.

@Owl-A
Copy link
Contributor

Owl-A commented Oct 31, 2023

Thanks for the ping. This is kind of the dual of initial problem. First we were not updating RigidBody3D with mutated state in PhysicsDirectBodyState3d now the problem is that we are overwriting mutated RigidBody3D with PhysicsDirectBodyState3d. I'll try to get a patch ready asap.

@Owl-A
Copy link
Contributor

Owl-A commented Oct 31, 2023

I have one concern though. If the user modifies both RigidBody3D state and state in PhysicsDirectBodyState3D (state) what should the behavior be?

@mihe
Copy link
Contributor

mihe commented Nov 12, 2023

I threw together a fix at #84799.

I have one concern though. If the user modifies both RigidBody3D state and state in PhysicsDirectBodyState3D (state) what should the behavior be?

Modifying both the node transform and the physics server transform during the same _integrate_forces call strikes me as a questionable thing to do in the first place. If someone desperately needs the physics server transform to reflect some changes they've made to the node transform then they can always call force_update_transform after doing so. The other way around is of course trickier, but I'm not sure it's a use-case worth catering to anyway.

@QbieShay
Copy link
Contributor

From the doc:

Allows you to read and safely modify the simulation state for the object. Use this instead of Node._physics_process if you need to directly change the body's position or other physics properties. By default, it works in addition to the usual physics behavior, but custom_integrator allows you to disable the default behavior and write custom force integration for a body.

If having a custom integrator is supposed to allow you to override position, I'm not sure why it shouldn't be possible to override the whole transform. At least from my understanding.

@mihe
Copy link
Contributor

mihe commented Nov 13, 2023

If having a custom integrator is supposed to allow you to override position, I'm not sure why it shouldn't be possible to override the whole transform.

I didn't mean to imply any sort of distinction between positions and transforms, if that's how it came across. Setting position/rotation/basis and their global equivalents is essentially just shorthand for setting the transform anyway.

What I was referring to (and what I understood @Owl-A to mean) was more the act of modifying the node transform through properties like position or transform while also modifying the physics server transform through the transform property of PhysicsDirectBodyState*D, all within the same _integrate_forces call, meaning something like this:

extends RigidBody3D

func _integrate_forces(state):
	# Read/write to the node transform
	transform.origin.x += 2.0 * state.step

	# Read/write to the physics server transform
	state.transform.origin.x += 10.0 * state.step

Having either one of those will work just fine (as of #84799) but when you have both of them then the outcome of that script will (as of #84799) be that the body moves at 2 u/s, due to the node transform ending up overwriting the physics server transform.

But as mentioned before, you can always call force_update_transform() yourself inbetween those two modifications and the body will move at 12 u/s, which is a fairly common workaround in cases where you potentially have stale physics state, from what I understand.

In either case, I struggle to see this being a common use-case. I suspect it's much more common to be dealing entirely with one transform or the other and not both.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

7 participants