Auto generated model.

[arjo129]

This PR adds a script which creates an automatically generated model and some tests. The script parses the XML and calculates the appropriate center of mass and buoyancy volumes based on the constraints given.

How the script works

The script reads an SDF-like file with the poses of individual links and computes their moments. It then calculates the correct position of the center of mass of the vehicle and applies this to the link whose inertial pose is marked with @calculated along with the appropriate mass.

Finally for any link whose collision volume that has been marked as @neutral_buoyancy the system will write the link to have neutral buoyancy.

Requires upstream: gazebosim/gz-sim#1211

Things to look out for in this PR.

Without #96 I expect most of the tests to fail except for the newly added tests which test the hydro-static stability of the vehicle. The newly FlatWorld tests assert that the vehicle is neutrally buoyant so it doesn't move any where to start of. There are no inherent oscillations. The TiltedWorld test asserts that the vehicle when placed in a certain pitch will oscillate without hydrodynamics to damp it. The maximum of this oscillation should be its starting pitch. Both these tests only check the buoyancy of the vehicle and no hydrodynamics.

[chapulina]

The smaller collisions look really strange

I know they're calculated in a way to achieve stability, but shouldn't they at least be reasonably placed and scaled? If we're going with whatever brings stability, I'd recommend we just get rid of these small collisions and rely on just one or two large collisions for buoyancy.

I also noticed that the battery's CoM seems to be way forward now, outside the battery itself:

[chapulina]

Could you add a comment explaining why we're loading only these plugins instead of loading tethys_equipped?

We should also consider putting these into a new model. Right now the plugins are duplicated on the 2 world files and on tethys_equipped - it will be hard to keep them all in sync.

[arjo129]

This is a unit test for the vehicle's hydrostatics I am not interested in the hydrodynamics. We could alternativelyu create a tethys_hydrostatic model which the tethys_equipped model will inherit in the future.

[chapulina]

Can we document that on the file? It's not immediately obvious to a reader why DetachableJoint and JointPositionController are needed for hydrostatics.

We could alternativelyu create a tethys_hydrostatic model which the tethys_equipped model will inherit in the future.

+1, I like the idea of incrementally building the model, ticketed #131.

[chapulina]

Should we check minPitch too?

It would also be interesting to check that the vehicle is actually bouncing up and down, because the current test would also pass if the amplitude is decreasing (which shouldn't happen because we don't have hydrodynamics loaded), or even if the vehicle were stopped in place. I'm not sure what the quickest way of testing this would be, maybe checking that the pitch rate keeps going up and down?

[arjo129]

This is a good point. I think we should ticket this so it can be revisited. I'm not sure if minPitch is the best way to go or something else. We should check for oscillations and amplitude of the oscillations.

[arjo129]

The battery visual is in the wrong place. I guess we can move it?

…

On Wed, Dec 22, 2021, 12:12 PM Louise Poubel ***@***.***> wrote: ***@***.**** requested changes on this pull request. The smaller collisions look really strange [image: PR89_coll] <https://user-images.githubusercontent.com/5751272/147033962-fea95e09-e3f9-449f-bce4-96b2b391fb86.png> I know they're calculated in a way to achieve stability, but shouldn't they at least be reasonably placed and scaled? If we're going with whatever brings stability, I'd recommend we just get rid of these small collisions and rely on just one or two large collisions for buoyancy. I also noticed that the battery's CoM seems to be way forward now, outside the battery itself: [image: PR89_com] <https://user-images.githubusercontent.com/5751272/147034184-9a7ccedc-19b0-41e2-b629-3c5bb21c6dd8.png> ------------------------------ In lrauv_description/CMakeLists.txt <#89 (comment)>: > configure_file( "hooks/hook.dsv.in" "${CMAKE_CURRENT_BINARY_DIR}/hooks/hook.dsv" @only ) +#============================================================================ +# Model Generation +add_custom_command( + OUTPUT modelsdf + COMMAND python3 ${CMAKE_CURRENT_SOURCE_DIR}/scripts/description_generator.py ${CMAKE_CURRENT_SOURCE_DIR}/models/tethys/model.sdf.in ${CMAKE_CURRENT_BINARY_DIR}/models/tethys/model.sdf Can we break this line so it's not necessary to scroll horizontally? ------------------------------ In lrauv_description/test/test_hydrostatic_stability.cc <#89 (comment)>: > + } + ); + + fixture->Finalize(); + + fixture->Server()->Run(true, 10000, false); + EXPECT_EQ(10000, tethysPoses.size()); + + bool first = true; + ignition::math::Pose3d prev_pose; + for (const auto &pose: tethysPoses) + { + if (first) + { + prev_pose = pose; + first = true; Oops ⬇️ Suggested change - first = true; + first = false; ------------------------------ In lrauv_description/test/worlds/flat_world.sdf <#89 (comment)>: > + <uri>tethys</uri> + <plugin + filename="ignition-gazebo-detachable-joint-system" + name="ignition::gazebo::systems::DetachableJoint"> + <parent_link>base_link</parent_link> + <child_model>__model__</child_model> + <child_link>drop_weight</child_link> + <topic>/model/tethys/drop_weight</topic> + </plugin> + <plugin + filename="ignition-gazebo-joint-position-controller-system" + name="ignition::gazebo::systems::JointPositionController"> + <joint_name>battery_joint</joint_name> + <use_velocity_commands>true</use_velocity_commands> + <cmd_max>0.0007</cmd_max> + </plugin> Could you add a comment explaining why we're loading only these plugins instead of loading tethys_equipped? We should also consider putting these into a new model. Right now the plugins are duplicated on the 2 world files and on tethys_equipped - it will be hard to keep them all in sync. ------------------------------ In lrauv_ignition_plugins/worlds/buoyant_tethys.sdf <#89 (comment)>: > @@ -5,6 +5,7 @@ --> <sdf version="1.6"> <world name="buoyant_tethys"> + <gravity>0 0 -9.8</gravity> If this is needed, it would be good to add a comment, otherwise I can see us removing it by mistake. Is it a matter of the precision matching other calculations? ------------------------------ In lrauv_description/test/test_hydrostatic_stability.cc <#89 (comment)>: > + fixture->Finalize(); + + fixture->Server()->Run(true, 100000, false); + EXPECT_EQ(100000, tethysPoses.size()); + + bool first = true; + + double maxPitch {std::numeric_limits<double>::min()}, + minPitch{std::numeric_limits<double>::max()}; + ignition::math::Pose3d prev_pose; + for (const auto &pose: tethysPoses) + { + if (first) + { + prev_pose = pose; + first = true; ⬇️ Suggested change - first = true; + first = false; ------------------------------ In lrauv_description/test/test_hydrostatic_stability.cc <#89 (comment)>: > + { + auto worldEntity = ignition::gazebo::worldEntity(_ecm); + ignition::gazebo::World world(worldEntity); + + auto modelEntity = world.ModelByName(_ecm, "tethys"); + EXPECT_NE(ignition::gazebo::kNullEntity, modelEntity); + + tethysPoses.push_back( + ignition::gazebo::worldPose(modelEntity, _ecm)); + } + ); + + fixture->Finalize(); + + fixture->Server()->Run(true, 10000, false); + EXPECT_EQ(10000, tethysPoses.size()); Does this one have one less zero than the tilted test on purpose? ------------------------------ In lrauv_description/test/test_hydrostatic_stability.cc <#89 (comment)>: > + EXPECT_NEAR(prev_pose.Pos().Z(), pose.Pos().Z(), 1e-2); + auto pitch = pose.Rot().Euler().Y(); + if (pitch > maxPitch) + { + maxPitch = pitch; + } + + if (pitch < minPitch) + { + minPitch = pitch; + } + + prev_pose = pose; + } + + // Since we start the system at 0.08 pitch, we should not exceed this. Should we check minPitch too? It would also be interesting to check that the vehicle is actually bouncing up and down, because the current test would also pass if the amplitude is decreasing (which shouldn't happen because we don't have hydrodynamics loaded), or even if the vehicle were stopped in place. I'm not sure what the quickest way of testing this would be, maybe checking that the pitch rate keeps going up and down? ------------------------------ In lrauv_description/test/test_hydrostatic_stability.cc <#89 (comment)>: > + + double maxPitch {std::numeric_limits<double>::min()}, + minPitch{std::numeric_limits<double>::max()}; + ignition::math::Pose3d prev_pose; + for (const auto &pose: tethysPoses) + { + if (first) + { + prev_pose = pose; + first = true; + continue; + } + EXPECT_NEAR(prev_pose.Pos().X(), pose.Pos().X(), 1e-2); + EXPECT_NEAR(prev_pose.Pos().Y(), pose.Pos().Y(), 1e-2); + EXPECT_NEAR(prev_pose.Pos().Z(), pose.Pos().Z(), 1e-2); + auto pitch = pose.Rot().Euler().Y(); Can we also add expectations for roll and yaw? ------------------------------ In lrauv_description/test/test_hydrostatic_stability.cc <#89 (comment)>: > + for (const auto &pose: tethysPoses) + { + if (first) + { + prev_pose = pose; + first = true; + continue; + } + EXPECT_EQ(prev_pose, pose); + prev_pose = pose; + } + EXPECT_EQ(prev_pose.Pos(), ignition::math::Vector3d(0,0,0)); + EXPECT_EQ(prev_pose.Rot(), ignition::math::Quaterniond(1,0,0,0)); +} + +TEST(Stability, TiltedWorld) It would be good to explain here what we're trying to test. — Reply to this email directly, view it on GitHub <#89 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEEMQBND7MEOT7HGLMV3QTUSFFZHANCNFSM5IQAJQ2Q> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were assigned.Message ID: ***@***.***>

[chapulina]

I'm close to approving this. Just checking if you missed these 2 comments, @arjo129:

• Auto generated model. #89 (comment)
• Auto generated model. #89 (comment)

[arjo129]

Thanks a lot I've addressed the feedback from those two.

[chapulina]

Let's get it in, thanks for the patience! 🚀