Resolve auto inertia based on input mass #1513

iche033 · 2024-12-09T17:10:34Z

🎉 New feature

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.

scpeters · 2024-12-09T21:11:32Z

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 · 2024-12-09T22:08:52Z

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 · 2024-12-09T22:39:49Z

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.

scpeters · 2024-12-09T23:12:35Z

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

src/Link_TEST.cc

include/sdf/Link.hh

python/test/pyLink_TEST.py

iche033 · 2024-12-10T18:46:20Z

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 · 2024-12-11T19:37:57Z

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 · 2024-12-11T22:46:38Z

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

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 
        
           );

src/Link_TEST.cc

scpeters · 2024-12-11T23:25:34Z

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 · 2024-12-11T23:57:06Z

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 · 2024-12-11T23:59:11Z

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 · 2024-12-12T02:31:53Z

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 · 2024-12-12T18:12:13Z

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 · 2024-12-12T19:13:04Z

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 · 2024-12-13T19:17:41Z

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

Signed-off-by: Ian Chen <[email protected]> update python test Signed-off-by: Ian Chen <[email protected]> udate api doc for resolving auto inertia Signed-off-by: Ian Chen <[email protected]> Add tests for auto-inertial with explicit mass (#1514) * link_dom test: Fix typo Signed-off-by: Steve Peters <[email protected]> * Test case for auto-inertials with explicit mass Signed-off-by: Steve Peters <[email protected]> --------- Signed-off-by: Steve Peters <[email protected]> update doc add comments Signed-off-by: Ian Chen <[email protected]> add one more test with multiple collisions Signed-off-by: Ian Chen <[email protected]> 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 <[email protected]> fix build Signed-off-by: Ian Chen <[email protected]> remove empty line Signed-off-by: Ian Chen <[email protected]> remove density warning, fix typo Signed-off-by: Ian Chen <[email protected]> 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 <[email protected]>

iche033 · 2024-12-13T21:03:13Z

rebased changes to main and forced pushed

scpeters · 2024-12-13T22:04:47Z

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

python/test/pyLink_TEST.py

Signed-off-by: Ian Chen <[email protected]>

iche033 · 2024-12-14T00:56:45Z

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.

Signed-off-by: Ian Chen <[email protected]>

iche033 · 2024-12-20T18:21:13Z

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 · 2024-12-20T19:12:39Z

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 · 2024-12-20T21:23:39Z

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

Signed-off-by: Steve Peters <[email protected]>

iche033 · 2024-12-20T21:50:40Z

see #1520

merged!

iche033 requested review from azeey and scpeters as code owners December 9, 2024 17:10

iche033 marked this pull request as draft December 9, 2024 17:10

github-actions bot added 🏛️ ionic Gazebo Ionic 🪵 jetty Gazebo Jetty labels Dec 9, 2024

scpeters mentioned this pull request Dec 9, 2024

Add tests for auto-inertial with explicit mass #1514

Merged

9 tasks

iche033 marked this pull request as ready for review December 9, 2024 22:40

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

scpeters approved these changes Dec 11, 2024

View reviewed changes

azeey reviewed Dec 11, 2024

View reviewed changes

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 mentioned this pull request Dec 13, 2024

Update multiple-collision test with justification #1515

Merged

scpeters changed the base branch from sdf15 to main December 13, 2024 19:30

iche033 force-pushed the auto_inertia_mass branch from d0226c6 to 2d466af Compare December 13, 2024 21:01

scpeters reviewed Dec 13, 2024

View reviewed changes

python/test/pyLink_TEST.py Show resolved Hide resolved

iche033 mentioned this pull request Dec 14, 2024

Add test for auto-inertia with mass and default density #1517

Merged

warn default density if mass is not specified

2509d55

Signed-off-by: Ian Chen <[email protected]>

iche033 added 2 commits December 16, 2024 22:39

Add test for auto-inertia with mass and default density (#1517)

c1dc394

Signed-off-by: Ian Chen <[email protected]>

Merge branch 'main' into auto_inertia_mass

b1b22f7

scpeters mentioned this pull request Dec 20, 2024

Add expectations about auto-inertial warnings #1520

Merged

iche033 force-pushed the auto_inertia_mass branch from d9e897b to b1b22f7 Compare December 20, 2024 21:48

Add expectations about auto-inertial warnings (#1520)

fa5d367

Signed-off-by: Steve Peters <[email protected]>

scpeters approved these changes Dec 20, 2024

View reviewed changes

iche033 merged commit aaadeea into main Dec 21, 2024
15 checks passed

iche033 deleted the auto_inertia_mass branch December 21, 2024 01:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve auto inertia based on input mass #1513

Resolve auto inertia based on input mass #1513

iche033 commented Dec 9, 2024 •

edited

Loading

scpeters commented Dec 9, 2024

iche033 commented Dec 9, 2024

iche033 commented Dec 9, 2024 •

edited

Loading

scpeters commented Dec 9, 2024

iche033 commented Dec 10, 2024 •

edited

Loading

scpeters commented Dec 11, 2024

iche033 commented Dec 11, 2024

azeey left a comment

scpeters commented Dec 11, 2024

iche033 commented Dec 11, 2024

iche033 commented Dec 11, 2024

azeey commented Dec 12, 2024

iche033 commented Dec 12, 2024 •

edited

Loading

scpeters commented Dec 12, 2024

scpeters commented Dec 13, 2024

iche033 commented Dec 13, 2024

scpeters commented Dec 13, 2024

iche033 commented Dec 14, 2024

iche033 commented Dec 20, 2024

scpeters commented Dec 20, 2024

scpeters commented Dec 20, 2024

iche033 commented Dec 20, 2024

	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
	);

Resolve auto inertia based on input mass #1513

Resolve auto inertia based on input mass #1513

Conversation

iche033 commented Dec 9, 2024 • edited Loading

🎉 New feature

Summary

Checklist

scpeters commented Dec 9, 2024

iche033 commented Dec 9, 2024

iche033 commented Dec 9, 2024 • edited Loading

scpeters commented Dec 9, 2024

iche033 commented Dec 10, 2024 • edited Loading

scpeters commented Dec 11, 2024

iche033 commented Dec 11, 2024

azeey left a comment

Choose a reason for hiding this comment

scpeters commented Dec 11, 2024

iche033 commented Dec 11, 2024

iche033 commented Dec 11, 2024

azeey commented Dec 12, 2024

iche033 commented Dec 12, 2024 • edited Loading

scpeters commented Dec 12, 2024

scpeters commented Dec 13, 2024

iche033 commented Dec 13, 2024

scpeters commented Dec 13, 2024

iche033 commented Dec 14, 2024

iche033 commented Dec 20, 2024

scpeters commented Dec 20, 2024

scpeters commented Dec 20, 2024

iche033 commented Dec 20, 2024

iche033 commented Dec 9, 2024 •

edited

Loading

iche033 commented Dec 9, 2024 •

edited

Loading

iche033 commented Dec 10, 2024 •

edited

Loading

iche033 commented Dec 12, 2024 •

edited

Loading