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

Add units to VehicleWheel3D suspension stiffness and damping #94862

Merged
merged 1 commit into from
Sep 17, 2024

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Jul 28, 2024

Minor PR. In master VehicleWheel3D's suspension stiffness does not have its units documented, this PR fixes that.

The metric/SI unit for suspension stiffness is kg/s² or N/m. However, the values for typical cars are usually between 20,000 and 200,000, so it's common to use Mg/s² or N/mm instead to bring the range of values down to between 20 and 200. This is what Godot uses. The documentation already recommends values around 50, 100, and 200 depending on the type of car, and these are the values that produce sane results in testing.

Similarly, the metric/SI unit for suspension damping is kg/s or N⋅s/m. Typical values are in the hundreds, but Godot works with values between 0 and 1, so it is using Mg/s or N⋅s/mm instead.

Here is what it looks like in the inspector:

Screenshot 2024-07-28 at 1 05 09 AM

EDIT: I temporarily marked this PR as draft because 1) I looked at the code and did not see where any conversion factor of 1000 was taking place and 2) I realized that the cars in Truck Town may be misleading me due to their low mass (40kg vs a real car's 1000 to 2000 kg). However, I did some more testing, even with a car of higher mass more like a realistic car, values in N/mm produced much better results than trying to put in numbers in the tens of thousands. So I'm highly confident that N/mm is correct, even though I can't find where in the code this is determined.

@aaronfranke aaronfranke added this to the 4.4 milestone Jul 28, 2024
@aaronfranke aaronfranke requested review from a team as code owners July 28, 2024 07:26
@aaronfranke aaronfranke marked this pull request as draft July 28, 2024 07:33
@aaronfranke aaronfranke marked this pull request as ready for review July 28, 2024 07:50
@aaronfranke aaronfranke changed the title Add units to VehicleWheel3D suspension stiffness Add units to VehicleWheel3D suspension stiffness and damping Jul 28, 2024
@aaronfranke aaronfranke force-pushed the unit-suspension-stiffness branch from 152cd25 to cb44114 Compare July 28, 2024 08:07
Copy link

@vlandemart vlandemart left a comment

Choose a reason for hiding this comment

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

Tiny changes, seem correct

Copy link
Member

@Calinou Calinou 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 to me.

@akien-mga
Copy link
Member

akien-mga commented Sep 17, 2024

It seems way overkill to me to list units both in N and in kg·m/s² or combination thereof.

I think we should pick one standard and stick with it. AFAIK N is a SI unit, and from what I remember of my university time, this would typically be the only one people reason about, unless doing dimensional analysis - and I don't think it's our role to invite people to dimensional analysis in the unit suffix of the inspector.

So I would stick to only the N derivatives and not specify 2 sets of units for those already convoluted dimensions.

Unless it is commonly established to use Mg over N in those specific dimensions in car engineering, in which case I would suggest using only that and dropping the N equivalents.

@aaronfranke aaronfranke force-pushed the unit-suspension-stiffness branch from c2152a5 to e3895e0 Compare September 17, 2024 10:32
@akien-mga akien-mga merged commit 750ffa5 into godotengine:master Sep 17, 2024
20 checks passed
@akien-mga
Copy link
Member

Thanks!

@aaronfranke aaronfranke deleted the unit-suspension-stiffness branch September 17, 2024 21:40
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.

4 participants