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.

Sign up for GitHub

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

Jump to bottom

Resolve auto inertia based on input mass #1513

Merged
merged 5 commits into from
Dec 21, 2024
Merged

Resolve auto inertia based on input mass #1513

merged 5 commits into from
Dec 21, 2024

Conversation

iche033
Copy link
Contributor

@iche033 iche033 commented Dec 9, 2024 •
edited
Loading

🎉 New feature

Closes #1482

Summary

Support auto-inertia computation using mass and density. Implemented based on the suggestions in #1482 (comment) and #1482 (comment)

inertia is first auto resolved from all collisions as usual. If mass is specified, we normalize the inertia to get unit inertia, then scaling is applied to match the desired mass.

Marked as draft to get feedback on implementation and correctness of math

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@iche033 iche033 marked this pull request as draft December 9, 2024 17:10
@github-actions github-actions bot added 🏛️ ionic Gazebo Ionic 🪵 jetty Gazebo Jetty labels Dec 9, 2024
@scpeters scpeters mentioned this pull request Dec 9, 2024
Merged
9 tasks
@scpeters
Copy link
Member

scpeters commented Dec 9, 2024

this looks good! I've added some tests in #1514 targeting this branch that you can merge into this branch if they seem reasonable

@iche033
Copy link
Contributor Author

iche033 commented Dec 9, 2024

this looks good! I've added some tests in #1514 targeting this branch that you can merge into this branch if they seem reasonable

nice, test looks good, thanks!

@iche033
Copy link
Contributor Author

iche033 commented Dec 9, 2024 •
edited
Loading

Merged tests from #1514 and updated Link's ResolveAutoInertials API doc on how auto inertia computation works if mass is specified (d529d12). This PR is now ready for review.

@iche033 iche033 marked this pull request as ready for review December 9, 2024 22:40
@scpeters
Copy link
Member

scpeters commented Dec 9, 2024

I think we should also update the auto_inertial_params_proposal proposal as well, and at some point write a tutorial / documentation since the proposal has now been implemented, but I won't block this PR on either of those tasks

scpeters
scpeters reviewed Dec 9, 2024
View reviewed changes
src/Link_TEST.cc Show resolved Hide resolved
include/sdf/Link.hh Outdated Show resolved Hide resolved
python/test/pyLink_TEST.py Outdated Show resolved Hide resolved
@iche033
Copy link
Contributor Author

iche033 commented Dec 10, 2024 •
edited
Loading

I think we should also update the auto_inertial_params_proposal proposal as well

added a note in the auto inertial proposal about mass in gazebosim/sdf_tutorials#100

@scpeters
Copy link
Member

I think we don't have a test involving multiple collisions with different density values attached to a link with auto-inertial and explicit mass. I think that would be ideal, but I won't block on it

@iche033
Copy link
Contributor Author

iche033 commented Dec 11, 2024

I think we don't have a test involving multiple collisions with different density values attached to a link with auto-inertial and explicit mass. I think that would be ideal, but I won't block on it

added tests for auto inertial with link mass and multiple collisions in c072a17 and 410d4e6

azeey
azeey reviewed Dec 11, 2024
View reviewed changes
Copy link
Collaborator

@azeey azeey left a comment

Choose a reason for hiding this comment

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

Looks great overall. One of the goals for calculating the inertia based on mass was to make it possible to calculate the inertia even when density values are not set. Do we know if that works? Maybe we can add a test?

This also means we can stop emitting a warning if a Collision is missing its <density> if mass is provided:

sdformat/src/Collision.cc

Lines 296 to 305 in 92da673

density = DensityDefault();
Error densityMissingErr(
ErrorCode::ELEMENT_MISSING,
"Collision is missing a <density> child element. "
"Using a default density value of " +
std::to_string(DensityDefault()) + " kg/m^3. "
);
enforceConfigurablePolicyCondition(
_config.WarningsPolicy(), densityMissingErr, _errors
);

scpeters
scpeters reviewed Dec 11, 2024
View reviewed changes
src/Link_TEST.cc Outdated Show resolved Hide resolved
src/Link_TEST.cc Show resolved Hide resolved
@scpeters
Copy link
Member

I think we don't have a test involving multiple collisions with different density values attached to a link with auto-inertial and explicit mass. I think that would be ideal, but I won't block on it

added tests for auto inertial with link mass and multiple collisions in c072a17 and 410d4e6

thanks; I'm did the math to verify the inertia values for that test case, and it looks right but a bit complex. I'd like to have our test expectations documented, so I'll propose a slight variation of the test geometry that places the lumped center of mass at the origin to simplify the math and then document that. I can do that in a follow-up PR if other concerns are resolved before I have a chance to write it up

@iche033
Copy link
Contributor Author

iche033 commented Dec 11, 2024

Looks great overall. One of the goals for calculating the inertia based on mass was to make it possible to calculate the inertia even when density values are not set. Do we know if that works? Maybe we can add a test?

If density is not explicitly set by the user, the default value should be used. Yes I can add or change a test in a follow-up PR after @scpeters updates the test for computing auto inertia involving multiple collisions

This also means we can stop emitting a warning if a Collision is missing its if mass is provided:

removed warning ca94a22

@iche033
Copy link
Contributor Author

iche033 commented Dec 11, 2024

I'd like to have our test expectations documented, so I'll propose a slight variation of the test geometry that places the lumped center of mass at the origin to simplify the math and then document that. I can do that in a follow-up PR if other concerns are resolved before I have a chance to write it up

thanks! I've addessed remaining comments except the one about adding a test with no <density> specified. Let's do that in follow-up PR(s).

@azeey
Copy link
Collaborator

azeey commented Dec 12, 2024

One last thing I thought of is whether this would break existing SDF files. If you have a link with the mass specified as well as densities in the collisions, previously, the mass was completely ignored. Now, the mass will override the densities which will likely result in different MOI values. Am I correct?

@iche033
Copy link
Contributor Author

iche033 commented Dec 12, 2024 •
edited
Loading

One last thing I thought of is whether this would break existing SDF files. If you have a link with the mass specified as well as densities in the collisions, previously, the mass was completely ignored. Now, the mass will override the densities which will likely result in different MOI values. Am I correct?

yes this will be a behavior change so we would likely need to retarget this to main. Alternatively we could add another attribute to toggle whether to use mass or density for auto-inertia, e.g. <inertial auto=true auto_use_mass=true>.

@scpeters
Copy link
Member

One last thing I thought of is whether this would break existing SDF files. If you have a link with the mass specified as well as densities in the collisions, previously, the mass was completely ignored. Now, the mass will override the densities which will likely result in different MOI values. Am I correct?

Currently there is a warning when specifying mass with //inertial@auto == true saying that the mass will be ignored. This PR definitely introduces a behavior change for files like that since the mass will rescale the MOI values (though with multiple collisions the relative density still has an effect on the MOI). As to whether this is a "breaking change", this fixes a warning and honors the intent of the mass value.

We can certainly merge this to main. If we want to backport it to Ionic or Harmonic, we would need to clearly communicate the behavior change. Since it fixes an existing warning, I think there's a case to be made that it could be backported.

@scpeters scpeters mentioned this pull request Dec 13, 2024
Merged
@scpeters
Copy link
Member

I'd like to have our test expectations documented, so I'll propose a slight variation of the test geometry that places the lumped center of mass at the origin to simplify the math and then document that. I can do that in a follow-up PR if other concerns are resolved before I have a chance to write it up

thanks! I've addessed remaining comments except the one about adding a test with no <density> specified. Let's do that in follow-up PR(s).

here's my update to the multi-collision / multi-density test: #1515

@scpeters scpeters changed the base branch from sdf15 to main December 13, 2024 19:30
@iche033
rescale inertia to match specified mass
2d466af 
Signed-off-by: Ian Chen <ichen@openrobotics.org>

update python test

Signed-off-by: Ian Chen <ichen@openrobotics.org>

udate api doc for resolving auto inertia

Signed-off-by: Ian Chen <ichen@openrobotics.org>

Add tests for auto-inertial with explicit mass (#1514)

* link_dom test: Fix typo

Signed-off-by: Steve Peters <scpeters@openrobotics.org>

* Test case for auto-inertials with explicit mass

Signed-off-by: Steve Peters <scpeters@openrobotics.org>

---------

Signed-off-by: Steve Peters <scpeters@openrobotics.org>

update doc add comments

Signed-off-by: Ian Chen <ichen@openrobotics.org>

add one more test with multiple collisions

Signed-off-by: Ian Chen <ichen@openrobotics.org>

Update mass expectation in auto-inertial tests

The mass is no longer explicitly set in these
Link tests, so replace the not-equals expectation
with an expectation of what the mass should be.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>

fix build

Signed-off-by: Ian Chen <ichen@openrobotics.org>

remove empty line

Signed-off-by: Ian Chen <ichen@openrobotics.org>

remove density warning, fix typo

Signed-off-by: Ian Chen <ichen@openrobotics.org>

Update multiple-collision test with justification (#1515)

Modifies the auto-inertial test with multiple collisions
with different densities so that the lumped center of
gravity is at the link origin and derives the expected
moment of inertia values.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@iche033
Copy link
Contributor Author

iche033 commented Dec 13, 2024

rebased changes to main and forced pushed

@scpeters
Copy link
Member

This also means we can stop emitting a warning if a Collision is missing its if mass is provided:

removed warning ca94a22

It looks like the warning is unconditionally removed, regardless of whether //inertial/mass is set or not. It's tricky to conditionally remove the warning since Collision::CalculateInertial doesn't know whether the Link inertial has an explicit mass value set. I think we would need to either add a _warnIfDensityIsUnset flag to Collision::CalculateInertial method, or we could try to delete the ELEMENT_MISSING errors in this loop in Link.cc

@iche033 iche033 mentioned this pull request Dec 14, 2024
Merged
@iche033
warn default density if mass is not specified
2509d55 
Signed-off-by: Ian Chen <ichen@openrobotics.org>
@iche033
Copy link
Contributor Author

iche033 commented Dec 14, 2024

I think we would need to either add a _warnIfDensityIsUnset flag to Collision::CalculateInertial method, or we could try to delete the ELEMENT_MISSING errors in this loop in Link.cc

I went with the option to add a _warnIfDensityIsUnset arg to Collision::CalculateInertial in 2509d55. I think it implements what you're suggestion, please take another look, thanks.

@iche033
Copy link
Contributor Author

iche033 commented Dec 20, 2024

I went with the option to add a _warnIfDensityIsUnset arg to Collision::CalculateInertial in 2509d55. I think it implements what you're suggestion, please take another look, thanks.

@scpeters do the changes in 2509d55 look good to you?

@scpeters
Copy link
Member

I went with the option to add a _warnIfDensityIsUnset arg to Collision::CalculateInertial in 2509d55. I think it implements what you're suggestion, please take another look, thanks.

@scpeters do the changes in 2509d55 look good to you?

Yes, that looks good; I want to see if we can add a test that verifies the presence / absence of the errors / warnings depending on that new flag

@scpeters scpeters mentioned this pull request Dec 20, 2024
Merged
@scpeters
Copy link
Member

I went with the option to add a _warnIfDensityIsUnset arg to Collision::CalculateInertial in 2509d55. I think it implements what you're suggestion, please take another look, thanks.

@scpeters do the changes in 2509d55 look good to you?

Yes, that looks good; I want to see if we can add a test that verifies the presence / absence of the errors / warnings depending on that new flag

see #1520

@scpeters
Add expectations about auto-inertial warnings (#1520)
fa5d367 
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@iche033
Copy link
Contributor Author

iche033 commented Dec 20, 2024

see #1520

merged!

@iche033 iche033 merged commit aaadeea into main Dec 21, 2024
15 checks passed
@iche033 iche033 deleted the auto_inertia_mass branch December 21, 2024 01:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@azeey azeey azeey left review comments

@scpeters scpeters scpeters approved these changes

Assignees
No one assigned
Labels
🏛️ ionic Gazebo Ionic 🪵 jetty Gazebo Jetty
Projects
Core development
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

//inertial/@auto: allow specifying mass instead of density
3 participants
@iche033 @scpeters @azeey