Add Gravity Feature, fix LinkFeatures_TEST #275

scpeters · 2021-07-09T07:45:13Z

🎉 New feature

This adds a feature for getting and setting the gravity vector in a World. This was motivated by a failing test in #261.

Summary

The LinkFeatures_TEST currently loads an sdf::World object then copies it in order to modify the <gravity/> SDFormat element, with a comment noting "// TODO Add a world property feature to set gravity instead of this hack". In ignition-physics3 and earlier, this was a useful hack, but it doesn't work in ignition-physics4 due to an internal change in libsdformat11. I've illustrated the problem by adding some failing test expectations in e270f65 that call sdf::World::ValidateGraphs. Loading the sdf::World from an sdf::ElementPtr does not work anymore; you have to load an sdf::Root in order for the FrameAttachedToGraph and PoseRelativeToGraph to be properly loaded. I believe this is also related to the test failures observed in #261.

To fix the issue, I've added World::GetGravity and World::SetGravity APIs in a new Gravity Feature in World.hh (
94048ea). The APIs use FrameSemantics.hh so you can specify the FrameID for the coordinates in which the gravity vector is specified. I then updated the LoadWorld function in LinkFeatures_TEST in ec232ac so it can load the world properly. I removed the LoadWorld function from ShapeFeatures_TEST in 573f001 since it has the same flaw but is unused.

Test it

Compile the code as of e270f65 and observe the failure of UNIT_LinkFeatures_TEST, and then observe that the test passes with the tip of the branch.

Checklist

Signed all commits for DCO
Added tests
Added example and/or tutorial
Updated documentation (as needed)
Updated migration guide (as needed)
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

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

codecov · 2021-07-09T08:11:04Z

Codecov Report

Merging #275 (af2023e) into ign-physics4 (5b5207d) will decrease coverage by 0.10%.
The diff coverage is 100.00%.

❗ Current head af2023e differs from pull request most recent head 9ce8fd5. Consider uploading reports for the commit 9ce8fd5 to get more accurate results

@@               Coverage Diff                @@
##           ign-physics4     #275      +/-   ##
================================================
- Coverage         75.40%   75.30%   -0.11%     
================================================
  Files               125      125              
  Lines              5433     5463      +30     
================================================
+ Hits               4097     4114      +17     
- Misses             1336     1349      +13

Impacted Files	Coverage Δ
dartsim/src/WorldFeatures.cc	`85.18% <100.00%> (+2.20%)`	⬆️
include/ignition/physics/detail/FrameSemantics.hh	`100.00% <100.00%> (ø)`
include/ignition/physics/detail/World.hh	`100.00% <100.00%> (ø)`
dartsim/src/SDFFeatures.cc	`63.72% <0.00%> (-2.67%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b5207d...9ce8fd5. Read the comment docs.

chapulina

How about adding unit tests to WorldFeatures_TEST?

dartsim/src/WorldFeatures.cc

azeey

LGTM! Thanks for adding this feature.

azeey · 2021-07-09T22:05:24Z

include/ignition/physics/World.hh

+        /// \brief Implementation API for setting the gravity vector, which is
+        /// expressed in the World frame..
+        /// \param[in] _id Identity of the world.
+        /// \param[in] _gravity Name of gravity.


nit: "Name"->"Value"?

scpeters · 2021-07-10T07:18:40Z

include/ignition/physics/World.hh

+        /// (a quantity that contains information about the coordinates
+        /// in which it is expressed).
+        /// \return Relative Gravity vector.
+        public: RelativeGravityType GetGravity() const;


it now occurs to me that the two GetGravity functions will be confusing for compilers since they can both be called with no arguments and have different return types. I'm inclined to remove this one that returns RelativeGravityType

It's weird that the compiler doesn't catch this. I'd also vote for removing the function that returns RelativeGravityType.

removed in 7fa6ab2

azeey · 2021-07-12T17:40:51Z

dartsim/src/LinkFeatures_TEST.cc

+
+  TestWorldPtr world = _engine->ConstructWorld(*sdfWorld);
+  EXPECT_NE(nullptr, world);
+  world->SetGravity(_gravity);


Can you add an expectation on GetGravity just to exercise the API?

I had intended to do that, but then I realized that I would need to move the AssertVectorApprox class definition up above this function in order to use it in the expectation. So I stalled out on doing it. I'll do it now though

done in ef1012d

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

scpeters · 2021-07-14T05:15:18Z

How about adding unit tests to WorldFeatures_TEST?

done in 315424c

Requires moving definition of AssertVectorApprox Signed-off-by: Steven Peters <[email protected]>

scpeters · 2021-07-14T05:36:13Z

dartsim/src/WorldFeatures_TEST.cc

+  // Get default gravity in link frame, which is pitched by pi/4
+  EXPECT_PRED_FORMAT2(vectorPredicate10,
+                      Eigen::Vector3d(6.92964645563, 0, -6.92964645563),
+                      world->GetGravity(linkFrameID));


somehow this call to GetGravity is causing an assertion in KinematicsFeatures.cc

https://github.com/ignitionrobotics/ign-physics/blob/e21c8c9f94d364a10f0671ec1eb347f2c32c3348/dartsim/src/KinematicsFeatures.cc#L33-L40

related to call to detail::Resolve in detail/World.hh:

https://github.com/ignitionrobotics/ign-physics/blob/ef1012d9170f962756d8dfa7668d6a43a099eea9/include/ignition/physics/detail/World.hh#L82-L100

I think we should relax the assertion, unless I'm going something wrong here

I've disabled the assertion in 37d09ae

I'm not sure if it hurts to ask the world frame what it's frame data is. Could be a problem for some physics engines? Looking at the code https://github.com/ignitionrobotics/ign-physics/blob/37d09ae717292623e6c1345efb26f66e458b0275/include/ignition/physics/detail/FrameSemantics.hh#L58-L62, maybe we should set currentCoordinates = RotationType::Identity(); if the _relativeTo is the world frame, as is done a couple of lines below.

ooh, that's smart. I could leave the assert unchanged that way

I've reverted the removal of the assert and followed your suggestion in 12b4cd4

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

This reverts commit 37d09ae. Signed-off-by: Steven Peters <[email protected]>

Avoid calling FrameDataRelativeToWorld for quantities relative to the world frame but expressed in different coordinates. This prevents an assertion in KinematicsFeatures.cc Signed-off-by: Steven Peters <[email protected]>

scpeters added 7 commits July 8, 2021 21:34

LinkFeatures_TEST: ValidateGraphs in LoadWorld

e270f65

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

Fix doxygen parameter name in World.hh

5f41fcc

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

Add Feature to Get/Set Gravity vector in a World

94048ea

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

dartsim: implement Gravity Feature

356d191

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

Fix LinkFeatures_TEST by using Gravity Feature

ec232ac

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

ShapeFeatures_TEST: remove unused function

573f001

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

Add coverage for World::GetGravity

d593676

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

scpeters requested review from azeey and chapulina July 9, 2021 07:45

scpeters requested a review from mxgrey as a code owner July 9, 2021 07:45

github-actions bot added the 🏢 edifice Ignition Edifice label Jul 9, 2021

scpeters mentioned this pull request Jul 9, 2021

Clean up functions that trigger GCC9 warnings #261

Merged

chapulina reviewed Jul 9, 2021

View reviewed changes

dartsim/src/WorldFeatures.cc Show resolved Hide resolved

azeey approved these changes Jul 9, 2021

View reviewed changes

scpeters commented Jul 10, 2021

View reviewed changes

azeey reviewed Jul 12, 2021

View reviewed changes

scpeters added 4 commits July 13, 2021 22:09

Fix comment

5324195

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

Remove RelativeGravityType GetGravity API

7fa6ab2

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

Use RelativeForceType instead of RelativeGravity

8cbc89f

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

Test Gravity APIs in WorldFeatures_TEST

315424c

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

LinkFeatures_TEST: check GetGravity in LoadWorld

ef1012d

Requires moving definition of AssertVectorApprox Signed-off-by: Steven Peters <[email protected]>

scpeters commented Jul 14, 2021

View reviewed changes

Don't assert when getting World frame info

37d09ae

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

scpeters mentioned this pull request Jul 15, 2021

🎈 4.2.0~pre1 Prerelease #277

Merged

7 tasks

scpeters added 2 commits July 15, 2021 12:24

Revert "Don't assert when getting World frame info"

5261eee

This reverts commit 37d09ae. Signed-off-by: Steven Peters <[email protected]>

FrameSemantics: improve handling of World frame

12b4cd4

Avoid calling FrameDataRelativeToWorld for quantities relative to the world frame but expressed in different coordinates. This prevents an assertion in KinematicsFeatures.cc Signed-off-by: Steven Peters <[email protected]>

Merge branch 'ign-physics4' into link_features_validate_graphs_4

9ce8fd5

azeey approved these changes Jul 16, 2021

View reviewed changes

scpeters merged commit 164f088 into gazebosim:ign-physics4 Jul 16, 2021

scpeters deleted the link_features_validate_graphs_4 branch July 16, 2021 03:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Gravity Feature, fix LinkFeatures_TEST #275

Add Gravity Feature, fix LinkFeatures_TEST #275

scpeters commented Jul 9, 2021

codecov bot commented Jul 9, 2021 •

edited

Loading

chapulina left a comment

azeey left a comment

azeey Jul 9, 2021

scpeters Jul 14, 2021

scpeters Jul 10, 2021

azeey Jul 12, 2021

scpeters Jul 14, 2021

azeey Jul 12, 2021

scpeters Jul 14, 2021

scpeters Jul 14, 2021

scpeters commented Jul 14, 2021

scpeters Jul 14, 2021

scpeters Jul 15, 2021

azeey Jul 15, 2021

scpeters Jul 15, 2021

scpeters Jul 16, 2021

Add Gravity Feature, fix LinkFeatures_TEST #275

Add Gravity Feature, fix LinkFeatures_TEST #275

Conversation

scpeters commented Jul 9, 2021

🎉 New feature

Summary

Test it

Checklist

codecov bot commented Jul 9, 2021 • edited Loading

Codecov Report

chapulina left a comment

Choose a reason for hiding this comment

azeey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scpeters commented Jul 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 9, 2021 •

edited

Loading