Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds documentation to Pendulum.urdf. #5156

Closed

Conversation

liangfok
Copy link
Contributor

@liangfok liangfok commented Feb 13, 2017

This change is Reviewable

@liangfok
Copy link
Contributor Author

+@amcastro-tri for feature review please. Thanks!


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


Comments from Reviewable

@RussTedrake
Copy link
Contributor

i don't think that the first part of the documentation adds useful information, and it adds redundancy. i worry that it is doomed to get out of sync with the source below. for example, we typically don't document the value of constants.

@liangfok
Copy link
Contributor Author

I see. Here is the thread that served as the original motivation for creating this PR:

https://reviewable.io/reviews/robotlocomotion/drake/4958#-KcruAXuE_LqGfb50JVE

Maybe now that @amcastro-tri agrees that the COM is at Z=-0.5, we don't necessarily need to document it?


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


Comments from Reviewable

@amcastro-tri
Copy link
Contributor

I don't think that the first part of the documentation adds useful information, and it adds redundancy.

I need to agree here. The model is so simple that a quick look tells you what's in there. The confusion was because I apparently didn't care to read the URDF specification carefully. Now that I know exactly what the <origin> tag within a <joint> actually specifies it is very easy for me to tell what this urdf describes.

The second part describing a limitation of this urdf when computing the center of mass with RigidBodyTree::centerOfMass() will undoubtedly go out of sync.

Up to you @liangfok. If you really feel strong about documenting this urdf I would recommend:

  1. Remove this "problem" with "centerOfMass()". It will go out of sync.
  2. Make the description more accurate. With that alone I cannot reproduce in my head what the setup is about. For instance, in what plane is the pendulum swinging? , where is it located?

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


Comments from Reviewable

@liangfok
Copy link
Contributor Author

Alright, since the the confusion about what the URDF describes is resolved and questions were raised about the usefulness of both the first and second half of the proposed documentation, I will close this PR.

@liangfok liangfok closed this Feb 14, 2017
@liangfok liangfok deleted the feature/update_pendulum_urdf branch February 14, 2017 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants