Update model files: for iiwa.urdf, use cylinders instead of meshes (for speed). #3507

Lucy-tri · 2016-09-19T15:36:24Z

Use cylinders instead of meshes for some links, for processing speed.
Leave meshes for links at begin and end of arm, for model accuracy.
Changed colors to better match reality. In our arm, only a part of the link
is orange, and this colors the entire link orange, but its an improvement
to see each link.
Adding mesh files in dae format and model.config for future work in Gazebo.

Replace link 5’s mesh with a cylinder.

Update collision comments

This change is

liangfok · 2016-09-19T15:38:45Z

I'm very curious to have latency and memory benchmark numbers to see how big of different this makes!

Lucy-tri · 2016-09-19T16:01:54Z

Test results running a test that shows up in Drake Visualizer, and using the Linux “time” command’s “real time” measure.

Collision meshes	run_kuka_iiwa_arm_dynamics (min’sec”)
all 8	4’1”
none	0’31”
#7	1’59”
#6,7	2’46”
#5,6,7	2’52”
#0	1’40”

liangfok · 2016-09-19T16:07:49Z

Thanks. I'm trying to make sense of these numbers. Does "#X" in the first column mean "body X" has a collision mesh?

Lucy-tri · 2016-09-19T17:21:42Z

There are 8 main segments in the iiwa arm, representing the segments you can see in the arm. (Is "segment" a good term to use? Like the piece of arm between the wrist and elbow). Segments are links in urdf and are numbered 0-7. So when a segment is listed in my table above, it means only those links have meshes, and the remainder have cylinders.
Current config for this PR is meshes for only 6 and 7 (0-5 are cylinders).
So that only saves 25+% : down from 4" to 2’46”.
The next step would be to simplify the #7 mesh, I'd say, but that can be another PR. I feel this is a good step in the right direction.
I'm working on a test failure, so not ready for review yet.

Review status: 0 of 15 files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

liangfok · 2016-09-19T17:53:16Z

Thanks. There appears to be almost an order of magnitude difference! 😄

Lucy-tri · 2016-09-19T19:12:36Z

Liang and I have gone over these results, which show that xdot from both the rbs and rbp have changed from the original iiwa model, but they also don’t agree with each other.

rigid_body_plant_test results:
Original iiwa14.urdf:
rbs: 7 6 5 4 3 2 1 -42.1556 -8.05324 187.948 62.7761 -404.659 652.033 6782.82
rbp: 7 6 5 4 3 2 1 -42.1556 -8.05324 187.948 62.7761 -404.659 652.033 6782.82

urdf from this PR
rbs: 7 6 5 4 3 2 1 -42.157 -8.06073 187.948 62.7326 -404.587 651.282 6781.85
rbp: 7 6 5 4 3 2 1 -42.1556 -8.05301 187.948 62.7775 -404.659 652.058 6782.82

@amcastro-tri : Liang claims that the same urdf used in both a rbs and rbp should produce the same xdot values, and so there must be a bug in rbp. Can you please take a look?

Review status: 0 of 15 files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

liangfok · 2016-09-19T19:15:08Z

Could the problem be that RigidBodyPlant::penetration_stiffness_ and RigidBodyPlant::penetration_damping_ have different values relative to their RigidBodySystem counterparts (RigidBodySystem::penetration_stiffness and RigidBodySystem::penetration_damping, respectively)?

amcastro-tri · 2016-09-19T19:50:26Z

Yes, that could be a problem even if there are no collisions because a joint could reach a limit in which case penetration_stiffness_ is used to apply a reaction force. I will take a closer look

Review status: 0 of 15 files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

Lucy-tri · 2016-09-19T20:20:13Z

Change rbp damping to 15, to match rbs:
7 6 5 4 3 2 1 -42.157 -8.06073 187.948 62.7326 -404.587 651.282 6781.85
7 6 5 4 3 2 1 -42.1556 -8.05309 187.948 62.777 -404.659 652.049 6782.82

Review status: 0 of 15 files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

Lucy-tri · 2016-09-19T21:20:48Z

Had to match friction also, now results match and test passes.

Review status: 0 of 15 files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

Lucy-tri · 2016-09-20T16:17:19Z

Some build failures happened due to a dreal build failure, fixed in #3524.

@drake-jenkins-bot retest this please

Review status: 0 of 16 files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

Lucy-tri · 2016-09-20T16:21:31Z

+@liangfok for feature review, please.

Review status: 0 of 16 files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

liangfok · 2016-09-21T02:39:46Z

I see numerous .dae files were added but they are not being referenced by iiwa14.urdf. Is this because there is another URDF or SDF file that references these .dae files for use with Gazebo? If so, I recommend that this other URDF also be included.

Reviewed 15 of 15 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

drake/examples/kuka_iiwa_arm/urdf/iiwa14.urdf, line 3 at r2 (raw file):

<?xml version="1.0" ?>
<!-- ===================================================================== -->
<!-- Before this urdf file was submitted to Drake, it was auto generated by

BTW: nit: "urdf" should be "URDF" since it is an acronym.

(same comment below)

drake/examples/kuka_iiwa_arm/urdf/iiwa14.urdf, line 11 at r2 (raw file):

     Therefore some collision models are represented as simple cylinders instead
     of meshes. However it's important to have accurate collision models at
     either end of an arm, so as not to hinder moving to a target.

Is "hinder" the right word here? I believe the purpose for having more accurate collision models for the end of the arm is because we anticipate that the end of the arm is where it will most likely contact the environment.

Comments from Reviewable

Lucy-tri · 2016-09-21T14:56:13Z

The dae's are for use in Gazebo, which apparently doesn't accept obj files. To use the URFD in Gazebo I did search and replace in the URDF of obj with dea. I don't want to create another URDF that will get out of sync. I could possibly remove the dae files if it's weird to have them here, unused. Regenerating them requires a tool like Meshlab.

Review status: all files reviewed at latest revision, 2 unresolved discussions.

drake/examples/kuka_iiwa_arm/urdf/iiwa14.urdf, line 3 at r2 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW: nit: "urdf" should be "URDF" since it is an acronym.

(same comment below)

Good catch, I'll fix.

Comments from Reviewable

liangfok · 2016-09-21T17:07:29Z

I think being able to load and simulate the IIWA model in Gazebo is useful since it could be used as a comparison with Drake and to help debug and improve the model. I created #3543 to track this issue. The .dae files should probably be removed from this PR and be included in a future PR that resolves #3543. Also, I notice that the .dae files consume another 24MB:

$ cd drake-distro/drake/examples/kuka_iiwa_arm/urdf/meshes/visual
$ find . -iname '*.dae' -print0 | du --files0-from - -ch -s | tail -1
24M total

Thus, we may need to address #3257 first.

Review status: all files reviewed at latest revision, 2 unresolved discussions.

Comments from Reviewable

Lucy-tri · 2016-09-21T19:34:20Z

Removed all dae files in this example.

Review status: 2 of 4 files reviewed at latest revision, 2 unresolved discussions.

drake/examples/kuka_iiwa_arm/urdf/iiwa14.urdf, line 3 at r2 (raw file):

Previously, Lucy-tri (Lucy) wrote…

Good catch, I'll fix.

fixed

drake/examples/kuka_iiwa_arm/urdf/iiwa14.urdf, line 11 at r2 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Is "hinder" the right word here? I believe the purpose for having more accurate collision models for the end of the arm is because we anticipate that the end of the arm is where it will most likely contact the environment.

Rewrote

Comments from Reviewable

Leave meshes for links at end of arm, for model accuracy. Changed colors to better match reality. In our arm, only a part of the link is orange, and this colors the entire link orange, but its an improvement to see each link. Adding model.config for future work in Gazebo. Make friction coefficient and damping in rigid body plant match rbs.

Lucy-tri · 2016-09-21T20:13:07Z

Squashed commits so dae files do not appear in history. I think I did that right, anyway.

Review status: 2 of 4 files reviewed at latest revision, 2 unresolved discussions.

Comments from Reviewable

liangfok · 2016-09-22T17:31:42Z

Reviewed 2 of 14 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

Lucy-tri · 2016-09-22T18:57:01Z

+@jwnimmer-tri for platform review, please.

Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

jwnimmer-tri · 2016-09-22T19:04:17Z

Reviewed 2 of 15 files at r1, 2 of 14 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

drake/systems/plants/rigid_body_plant/rigid_body_plant.h, line 163 at r4 (raw file):

  // TODO(amcastro-tri): Implement contact materials for the RBT engine.
  T penetration_stiffness_{150.0};  // An arbitrarily large number.
  T penetration_damping_{penetration_stiffness_ / 10.0};

I am skeptical of changing magic numbers in a PR titled "update model files", with no apparent test coverage or justification. I see there were discussions of this in the comment threads, but is this change truly appropriate for this PR?

Comments from Reviewable

liangfok · 2016-09-22T19:14:40Z

I left some more comments.

Review status: all files reviewed at latest revision, 3 unresolved discussions.

drake/examples/kuka_iiwa_arm/urdf/iiwa14.urdf, line 3 at r4 (raw file):

<?xml version="1.0" ?>
<!-- ===================================================================== -->
<!-- Before this URDF file was submitted to Drake, it was auto generated by

BTW: nit: replace "submitted" with "added".

drake/examples/kuka_iiwa_arm/urdf/iiwa14.urdf, line 4 at r4 (raw file):

<!-- ===================================================================== -->
<!-- Before this URDF file was submitted to Drake, it was auto generated by
     xacro from a source file. While living in Drake, this URDF file underwent

BTW: nit: insert back-ticks around xacro so readers know it's a thing.

drake/systems/plants/rigid_body_plant/rigid_body_plant.h, line 163 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I am skeptical of changing magic numbers in a PR titled "update model files", with no apparent test coverage or justification. I see there were discussions of this in the comment threads, but is this change truly appropriate for this PR?

Yeah, I can see how this is confusing!

This code has test coverage in drake-distro/drake/systems/plants/rigid_body_plant/test/rigid_body_plant_test.cc, specifically in the test called "CompareWithRBS1Dynamic".

Unfortunately, I don't see any other way to do this since RigidBodyPlant does not provide modifier methods for these fields.

@amcastro-tri, what do you think? If modifer methods were provided, these changes could be moved into the aforementioned unit test.

Comments from Reviewable

jwnimmer-tri · 2016-09-22T19:16:25Z

drake/systems/plants/rigid_body_plant/rigid_body_plant.h, line 163 at r4 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Yeah, I can see how this is confusing!

This code has test coverage in drake-distro/drake/systems/plants/rigid_body_plant/test/rigid_body_plant_test.cc, specifically in the test called "CompareWithRBS1Dynamic".

Unfortunately, I don't see any other way to do this since RigidBodyPlant does not provide modifier methods for these fields.

@amcastro-tri, what do you think? If modifer methods were provided, these changes could be moved into the aforementioned unit test.

If these two line edits are not part of the PR, does any test fail?

Comments from Reviewable

Lucy-tri · 2016-09-22T19:16:54Z

Review status: all files reviewed at latest revision, 3 unresolved discussions.

drake/systems/plants/rigid_body_plant/rigid_body_plant.h, line 163 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

If these two line edits are not part of the PR, does any test fail?

The values have been changed to fix an admitted bug, that the values should match between rbs and rbp. Yes, it caused a test failure in this test.

Comments from Reviewable

liangfok · 2016-09-22T19:17:32Z

Review status: all files reviewed at latest revision, 3 unresolved discussions.

drake/systems/plants/rigid_body_plant/rigid_body_plant.h, line 163 at r4 (raw file):

Previously, Lucy-tri (Lucy) wrote…

The values have been changed to fix an admitted bug, that the values should match between rbs and rbp. Yes, it caused a test failure in this test.

I assume the test failure is how the need for this change was detected?

Comments from Reviewable

amcastro-tri · 2016-09-22T19:18:38Z

Review status: all files reviewed at latest revision, 3 unresolved discussions.

drake/systems/plants/rigid_body_plant/rigid_body_plant.h, line 163 at r4 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

I assume the test failure is how the need for this change was detected?

Yes, we could add those setters so that we can make the changes from the unit test. I am a bit against it because I didn't want to expose our hacky collision model to the world. Sighs.... But I guess that is ok for the sake of this PR if the above solution is not satisfactory.

Comments from Reviewable

Lucy-tri · 2016-09-22T19:21:03Z

Review status: all files reviewed at latest revision, 3 unresolved discussions.

drake/systems/plants/rigid_body_plant/rigid_body_plant.h, line 163 at r4 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

Yes, we could add those setters so that we can make the changes from the unit test. I am a bit against it because I didn't want to expose our hacky collision model to the world. Sighs.... But I guess that is ok for the sake of this PR if the above solution is not satisfactory.

It caused a failure in rigid_body_plant_test, which tests one of the files in this commit, iiwa14.urdf. Yes, the test failure occurred after making the changes to iiwa14.urdf. The values are constants - couldn't we define constants that both rbs and rbp use as default values?

Comments from Reviewable

liangfok · 2016-09-22T19:21:08Z

drake/systems/plants/rigid_body_plant/rigid_body_plant.h, line 163 at r4 (raw file):

Previously, Lucy-tri (Lucy) wrote…

It caused a failure in rigid_body_plant_test, which tests one of the files in this commit, iiwa14.urdf. Yes, the test failure occurred after making the changes to iiwa14.urdf.
The values are constants - couldn't we define constants that both rbs and rbp use as default values?

I'm OK just changing the values here. However, if we don't get a good collision model soon, I can imagine the need for applications to fine-tune the collision model gains based on what's being modeled.

Comments from Reviewable

liangfok · 2016-09-22T19:22:23Z

drake/systems/plants/rigid_body_plant/rigid_body_plant.h, line 163 at r4 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

I'm OK just changing the values here. However, if we don't get a good collision model soon, I can imagine the need for applications to fine-tune the collision model gains based on what's being modeled.

We can define static constants in `RigidBodyPlant` that are used by `RigidBodySystem`. Definitely not the other way around since we want to delete `RigidBodySystem` once we migrate to System 2.0.

Comments from Reviewable

amcastro-tri · 2016-09-22T19:26:24Z

Review status: all files reviewed at latest revision, 3 unresolved discussions.

drake/systems/plants/rigid_body_plant/rigid_body_plant.h, line 163 at r4 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

We can define static constants in RigidBodyPlant that are used by RigidBodySystem. Definitely not the other way around since we want to delete RigidBodySystem once we migrate to System 2.0.

FTR, I am ok with the changes to the values of the constants done in this PR.

Comments from Reviewable

jwnimmer-tri · 2016-09-22T19:31:33Z

Review status: all files reviewed at latest revision, 3 unresolved discussions.

drake/systems/plants/rigid_body_plant/rigid_body_plant.h, line 163 at r4 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

FTR, I am ok with the changes to the values of the constants done in this PR.

Ah, I thought it was the "scan all the urdfs and sanity check them" comparison/regression test that was failing. But ZOMG I just realized what you are saying! The _RBP unit test_ loads the _iiwa model data_ from _examples_, so anytime we change the iiwa model for simulation convenience, we might break that unit test? That is heinous. I give up. This can merge as-is. Marking resolved.

Comments from Reviewable

liangfok · 2016-09-22T19:43:03Z

drake/systems/plants/rigid_body_plant/rigid_body_plant.h, line 163 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Ah, I thought it was the "scan all the urdfs and sanity check them" comparison/regression test that was failing. But ZOMG I just realized what you are saying! The RBP unit test loads the iiwa model data from examples, so anytime we change the iiwa model for simulation convenience, we might break that unit test? That is heinous. I give up. This can merge as-is. Marking resolved.

We are looking forward to learning the art of unit test creation...https://github.com//issues/3492

Comments from Reviewable

Lucy-tri · 2016-09-22T19:48:18Z

Review status: 3 of 4 files reviewed at latest revision, 3 unresolved discussions.

drake/examples/kuka_iiwa_arm/urdf/iiwa14.urdf, line 3 at r4 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW: nit: replace "submitted" with "added".

Fixed.

drake/examples/kuka_iiwa_arm/urdf/iiwa14.urdf, line 4 at r4 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW: nit: insert back-ticks around xacro so readers know it's a thing.

Fixed.

Comments from Reviewable

liangfok · 2016-09-22T19:48:57Z

Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

jwnimmer-tri · 2016-09-22T19:51:42Z

Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

Lucy-tri · 2016-09-22T19:54:31Z

Review status: all files reviewed at latest revision, 1 unresolved discussion.

drake/systems/plants/rigid_body_plant/rigid_body_plant.h, line 163 at r4 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

We are looking forward to learning the art of unit test creation...#3492

OK, so I won't make more code changes at this time. Thanks.

Comments from Reviewable

Lucy-tri force-pushed the Models branch from 3253de1 to a82a6bd Compare September 20, 2016 14:42

Lucy-tri assigned liangfok Sep 20, 2016

Lucy-tri force-pushed the Models branch from a82a6bd to ee8aaf6 Compare September 20, 2016 16:28

liangfok mentioned this pull request Sep 21, 2016

Drake's IIWA Model Cannot be Loaded into Gazebo #3543

Closed

Lucy-tri force-pushed the Models branch from ee8aaf6 to fc6f644 Compare September 21, 2016 17:34

Lucy-tri force-pushed the Models branch from 15769ed to ba1c8f4 Compare September 21, 2016 20:09

Lucy-tri assigned jwnimmer-tri Sep 22, 2016

Review comments, for comments.

dbe82a8

Lucy-tri merged commit ddf9612 into RobotLocomotion:master Sep 22, 2016

Update model files: for iiwa.urdf, use cylinders instead of meshes (for speed). #3507

Update model files: for iiwa.urdf, use cylinders instead of meshes (for speed). #3507

Conversation

Lucy-tri commented Sep 19, 2016 • edited by RussTedrake Loading

liangfok commented Sep 19, 2016 • edited Loading

Lucy-tri commented Sep 19, 2016

liangfok commented Sep 19, 2016 • edited Loading

Lucy-tri commented Sep 19, 2016

liangfok commented Sep 19, 2016 • edited Loading

Lucy-tri commented Sep 19, 2016

liangfok commented Sep 19, 2016 • edited Loading

amcastro-tri commented Sep 19, 2016

Lucy-tri commented Sep 19, 2016

Lucy-tri commented Sep 19, 2016

Lucy-tri commented Sep 20, 2016

Lucy-tri commented Sep 20, 2016

liangfok commented Sep 21, 2016

Lucy-tri commented Sep 21, 2016

liangfok commented Sep 21, 2016

Lucy-tri commented Sep 21, 2016

Lucy-tri commented Sep 21, 2016

liangfok commented Sep 22, 2016

Lucy-tri commented Sep 22, 2016

jwnimmer-tri commented Sep 22, 2016

liangfok commented Sep 22, 2016

jwnimmer-tri commented Sep 22, 2016

Lucy-tri commented Sep 22, 2016

liangfok commented Sep 22, 2016

amcastro-tri commented Sep 22, 2016

Lucy-tri commented Sep 22, 2016

liangfok commented Sep 22, 2016

liangfok commented Sep 22, 2016

amcastro-tri commented Sep 22, 2016

jwnimmer-tri commented Sep 22, 2016

liangfok commented Sep 22, 2016

Lucy-tri commented Sep 22, 2016

liangfok commented Sep 22, 2016

jwnimmer-tri commented Sep 22, 2016

Lucy-tri commented Sep 22, 2016

Lucy-tri commented Sep 19, 2016 •

edited by RussTedrake

Loading

liangfok commented Sep 19, 2016 •

edited

Loading

liangfok commented Sep 19, 2016 •

edited

Loading

liangfok commented Sep 19, 2016 •

edited

Loading

liangfok commented Sep 19, 2016 •

edited

Loading