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

[3.x] Fix KinematicBody axis lock #45176

Merged
merged 1 commit into from
Aug 30, 2021

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Jan 14, 2021

This fixes #25798 in 3.x.

@AndreaCatania
Copy link
Contributor

I'm not strongly against this, and for the presented reason it seems a good change. However, the physics server API are generally not so fast especially if compared to locked_axis & (1 << i). So have 3 PhysicsServer calls on a function that is called recursively each frame may be bad, especially when you have more than 1 character.

This is not a blocker if the physics team still think worth it.

@AndreaCatania
Copy link
Contributor

By the way, just noticed the type of locked_axis should not be uint16_t but uint32_t. All back-ends supports up to 32 flags.

@pouleyKetchoupp
Copy link
Contributor

@AndreaCatania The rest of the KinematicBody api is an expensive process anyway, I don't think these calls really make a difference. So I would rather keep things clean and simple.

For info, #25798 is only present on 3.2, since on master set_axis_lock is setting both members. It still makes sense to remove this duplicate on both branches anyway.

Copy link
Contributor

@pouleyKetchoupp pouleyKetchoupp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one thing I'd like to change. In order to clean things completely, let's add a helper method on KinematicBody3D instead of calling the physics server directly.

Something like:

void KinematicBody::_apply_axis_lock(Vector3 &p_motion) const {
	if (get_axis_lock(PhysicsServer::BODY_AXIS_LINEAR_X)) {
		p_motion.x = 0.0f;
	}
	if (get_axis_lock(PhysicsServer::BODY_AXIS_LINEAR_Y)) {
		p_motion.y = 0.0f;
	}
	if (get_axis_lock(PhysicsServer::BODY_AXIS_LINEAR_Z)) {
		p_motion.z = 0.0f;
	}
}

And then:

_apply_axis_lock(result.motion);

Instead of:

	for (int i = 0; i < 3; i++) { {
		if (PhysicsServer3D::get_singleton()->body_is_axis_locked(get_rid(), PhysicsServer3D::BodyAxis(1 << i))) {
			result.motion[i] = 0;
		}
	}

So the code in KinematicBody won't rely on actual values from the physics server to work properly.

@aaronfranke
Copy link
Member Author

@AndreaCatania I considered if we could replace PhysicsServer::get_singleton()->body_is_axis_locked with PhysicsServer::get_singleton()->body_get_axis_lock or similar and then keep the locked_axis & (1 << i), but it looks like this API used to exist and you removed it so I figured it was for a good reason.

@aaronfranke
Copy link
Member Author

aaronfranke commented Jan 14, 2021

@pouleyKetchoupp What if the helper method was located on PhysicsServer3D? This would avoid the repeated calls to body_is_axis_locked , assuming we don't implement body_get_axis_lock or similar, since we should be able to get locked_axis directly. It somewhat makes sense because locked_axis is located in PhysicsServer3D's bodies and PhysicsServer3D::BodyAxis is in PhysicsServer3D, though it's not as local of a solution as what you propose.

@pouleyKetchoupp
Copy link
Contributor

@aaronfranke It's not a bad idea, but currently I don't think it's worth the extra changes in the Physics Server API, and having to implement it in both Bullet and Godot Physics. I would rather do that later, only if for some reason we need it for optimization purpose.

@madmiraal
Copy link
Contributor

See #45175 (comment). #25798 was fixed with #38852, but never backported to 3.2. I think the better approach is a backport of #38852.

@aaronfranke aaronfranke requested a review from a team as a code owner March 12, 2021 12:26
Base automatically changed from 3.2 to 3.x March 16, 2021 11:11
@akien-mga akien-mga modified the milestones: 3.2, 3.3 Mar 17, 2021
@aaronfranke aaronfranke changed the title [3.2] Fix KinematicBody having a duplicate locked_axis variable [3.x] Fix KinematicBody having a duplicate locked_axis variable Mar 21, 2021
@akien-mga akien-mga modified the milestones: 3.3, 3.4 Mar 26, 2021
@aaronfranke aaronfranke force-pushed the 3.2-kine-dup-lock branch from 65f5ff4 to a73ecd8 Compare May 4, 2021 08:26
@aaronfranke aaronfranke force-pushed the 3.2-kine-dup-lock branch 2 times, most recently from 919e990 to a6bf1ae Compare June 18, 2021 15:03
@aaronfranke aaronfranke requested a review from a team as a code owner August 30, 2021 01:40
@aaronfranke aaronfranke changed the title [3.x] Fix KinematicBody having a duplicate locked_axis variable [3.x] Fix KinematicBody axis lock Aug 30, 2021
@aaronfranke
Copy link
Member Author

I updated this PR so that now it's a 3.x backport of #38852. This also includes backporting the renames, but the old names are kept as aliases for compatibility, and this PR also updates the documentation.

@aaronfranke aaronfranke requested review from AndreaCatania and a team August 30, 2021 01:43
Copy link
Contributor

@pouleyKetchoupp pouleyKetchoupp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@pouleyKetchoupp pouleyKetchoupp merged commit e443796 into godotengine:3.x Aug 30, 2021
@aaronfranke aaronfranke deleted the 3.2-kine-dup-lock branch August 30, 2021 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants