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

Fix GodotPhysicsDirectBodyState2D get_contact_impulse to return current physics step impulse #81654

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KlugeNico
Copy link

Issue:
GodotPhysicsDirectBodyState2D get_contact_impulse() function returns wrong value. Initial it returns just a Vector2.ZERO.
The problem is, that not the final impulse sum is used, but the value generated in pre_solve().

Solution:
The only solution I found was to add a function that's called after solve(). It's called post_solve and is optional for stuff that must be done in a GodotConstraint2D after being solved. Here we can add the collision contact info with all required information, instead of adding it in pre_solve(), where some information (the impulse sum) is missing.

Same issue for 3D:
#73569

@KlugeNico KlugeNico requested a review from a team as a code owner September 14, 2023 15:13
@Chaosus Chaosus added this to the 4.2 milestone Sep 18, 2023
@pcbeard
Copy link
Contributor

pcbeard commented Dec 10, 2023

FWIW, I've tested this by cherry picking onto my fork, and the impulse values look good to me.

}
if (B->can_report_contacts()) {
B->add_contact(global_B + offset_A, c.normal, depth, shape_B, c.initial_rB, global_A + offset_A, shape_A,
A->get_instance_id(), A->get_self(), c.initial_rA, c.acc_impulse); }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
A->get_instance_id(), A->get_self(), c.initial_rA, c.acc_impulse); }
A->get_instance_id(), A->get_self(), c.initial_rA, c.acc_impulse);
}

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.

None yet

6 participants