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

Correctly set mass for a rigid body with custom inertia and center of mass #78757

Merged
merged 1 commit into from Sep 26, 2023

Conversation

Crimzoth
Copy link
Contributor

@Crimzoth Crimzoth commented Jun 27, 2023

See #78750 for issue description.

Removed erroneous check, which caused _inv_mass not to be calculated when RigidBody2D or RigidBody3D used both custom center of mass and custom inertia.

The only purpose of the check for (calculate_inertia || calculate_center_of_mass) appears to be to prevent adding bodies to the godot_space's mass_properties_update_list . On elements of this list GodotBody3D::update_mass_properties() is then executed.
At the first glance it appears that if calculate_inertia == false and calculate_center_of_mass == false function GodotBody3D::update_mass_properties() would do nothing and this check in GodotBody3D::_mass_properties_changed() is valid.
This is not entirely true. GodotBody3D::update_mass_properties() is also used to calculate _inv_mass, which would be left at default value if the body was omitted from mass property update and cause incorrect physics behavior as described in #78750.

By removing the check for (calculate_inertia || calculate_center_of_mass) it can be ensured that _inv_mass is always calculated. At the same time GodotBody3D::update_mass_properties() already contains checks for the above parameters, meaning that if calculate_inertia == false or calculate_center_of_mass == false no unnecessary calculations will be executed. Checking for these parameters twice is not needed.

@AThousandShips
Copy link
Member

Please update the title of the PR and the message of the commit to be more descriptive of what is actually being done

@Crimzoth Crimzoth changed the title Update godot_body_3d.cpp Fix: Mass not set properly for RigidBody3D with custom inertia and center of mass Jun 27, 2023
@AThousandShips AThousandShips modified the milestones: 4.x, 4.2 Jun 27, 2023
@AThousandShips
Copy link
Member

If this is the correct solution it should probably be done for 2D as well for completeness

@Crimzoth
Copy link
Contributor Author

Yes, I've just checked and it affects 2D as well. I've added another commit for RigidBody2D.

@YuriSizov YuriSizov changed the title Fix: Mass not set properly for RigidBody3D with custom inertia and center of mass Correctly set mass for a rigid body with custom inertia and center of mass Jun 27, 2023
@YuriSizov
Copy link
Contributor

Please note that PRs should only contain one commit as a rule, so your commits need to be squashed (with some common message for both changes).

Removed erroneous check, which caused _inv_mass not to be calculated when RigidBody2D or RigidBody3D used both custom center of mass and custom inertia.
@Crimzoth
Copy link
Contributor Author

Please note that PRs should only contain one commit as a rule, so your commits need to be squashed (with some common message for both changes).

Squashed into single commit

@fire
Copy link
Member

fire commented Jun 27, 2023

@aaronfranke You might be interested in center of mass prs.

@wmigor
Copy link

wmigor commented Sep 23, 2023

I am making an airplane simulator. I am waiting for this fix

Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

Not 100% my area of expertise but this looks good to me. It makes sense that this code should always run even when the user sets their own values. It also makes sense why this bug wasn't discovered earlier, most people do not set custom inertia so it's unlikely that this bug would be encountered often.

@akien-mga akien-mga merged commit 1e4165a into godotengine:master Sep 26, 2023
13 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

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.

Mass not set properly for RigidBody3D with custom inertia and center of mass
7 participants