1. OpenSourceRoboticsFoundation
  2. Simulation
  3. gazebo
  4. Pull requests

Pull requests

#1747 Merged at 2825730
Repository
gazebo-fork
Branch
issue_1278
Repository
gazebo
Branch
gazebo6

Support for fixed joints

Author
  1. Silvio Traversaro
Reviewers
Description

Implementation of the fixed joints, solves #1278 .

For more informations check https://bitbucket.org/osrf/gazebo/issue/1278/support-fixed-joint-types-in-gazebo .

  • Issues #1278: support fixed joint types in Gazebo closed

Comments (28)

  1. Steven Peters

    Thanks for the submission! I haven't read the code yet, but I pulled your commits into osrf/gazebo to simplify the jenkins testing. I'll post updates as the results come in

        1. Silvio Traversaro author

          Thanks! Merging from master just before opening the PR (without rebuilding the tests) was not a great idea.

          If you prefer for integration with CI we can move this PR to the osrf/gazebo branch, but clearly even that is not optimal because I can't push eventual fixes to the osrf branch.

          1. Steven Peters

            No, let's keep it here. I often pull commits from forks when they are in branches before merging. It makes our CI work a little better, though we're working to make it support forks better

  2. Steven Peters

    I'm looking at your test, and it looks like you based it off of the friction test, which I thought was a nice way to set up the test. I think there is an even better way to set up models now, introduced in pull request #1466. Instead of using string streams and xml, the model properties can be encoded in protobuf messages and then used to spawn models programmatically.

    We have migrated a few tests to this new way, but I hadn't gotten to the friction test yet, since I had been hoping to make some larger changes to the friction parameters.

    Long story short, do you mind trying to use the protobuf interface for that test? I can advise on how to make the modifications, though there is a decent example in pull request #1744 (joint_test.hh) and also the GetWorldInertia test.

        1. Steven Peters

          We can also add provide_feedback as a field in joint.proto

          How would you feel about submitting a smaller pull request with fixes for provide_feedback? That will make the rest of this easier to review.

  3. Louise Poubel

    I ran a coverage build for this branch last night.

    I noticed that the newly introduced functions have only about 28% function coverage, which sort of makes sense since most functions are meaningless for fixed joints. My question is, do we care about the coverage here (we're trying to improve the coverage on all new pull requests)? Should there be tests calling these functions at least to make sure they don't break anything? I think that would be quick to implement... Nathan Koenig ?

    1. Silvio Traversaro author

      I guess all that different dummy implementations could be avoided with a slightly different joint structure.. but this is out of the scope of this PR. Let me know what do you think it make sense to do in the short term.

  4. Steven Peters

    I think we could add the fixed joint type to the SpawnJointRotational* tests. These tests are applied to joints that have all three translational directions constrained (revolute, ball, universal). The SpawnJointRotational test creates a parent and child link connected by a joint, applies a linear velocity to the parent link, and verifies that the child follows. The SpawnJointRotationalWorld test creates a link and attaches it to the world through a joint, both as parent and child, and then verifies that it doesn't fall with gravity turned on. Here's a patch to enable it:

    diff -r bdc3eba2e76b test/integration/joint_spawn.cc
    --- a/test/integration/joint_spawn.cc   Mon Jun 29 10:25:45 2015 -0700
    +++ b/test/integration/joint_spawn.cc   Sun Jul 19 16:10:28 2015 -0700
    @@ -492,6 +492,7 @@
       ::testing::Combine(PHYSICS_ENGINE_VALUES,
       ::testing::Values("revolute"
                       , "universal"
    +                  , "fixed"
                       , "ball")));
    
     // Skip prismatic, screw, and revolute2 because they allow translation
    @@ -499,6 +500,7 @@
       ::testing::Combine(PHYSICS_ENGINE_VALUES,
       ::testing::Values("revolute"
                       , "universal"
    +                  , "fixed"
                       , "ball")));
    
     int main(int argc, char **argv)