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

models: De-duplicate and clean up #11009

Closed
EricCousineau-TRI opened this issue Mar 25, 2019 · 7 comments
Closed

models: De-duplicate and clean up #11009

EricCousineau-TRI opened this issue Mar 25, 2019 · 7 comments

Comments

@EricCousineau-TRI
Copy link
Contributor

EricCousineau-TRI commented Mar 25, 2019

Relates #10531

We have some redundant models. I want to remove redundancy, and make things clearer (and possibly rely on upstream models when possible).

For example, @robert-verkuil ran into an issue where there are two Acrobot model files in the Drake source tree that he can't use from CMake (in underactuated):
FindResourceOrThrow("drake/multibody/benchmarks/acrobot/acrobot.sdf") indicates that this file is not installed
FindResourceOrThrow("drake/examples/acrobot/Acrobot.sdf") has an invalid spatial inertia for MBP's.
EDIT: Gah! Acrobot.sdf isn't even used, aside from some random attic Python parsing tests!

(Acute solution for this is to install benchmarks/.../acrobot.sdf for now.)

@jwnimmer-tri
Copy link
Collaborator

See also #5408 about the acrobot model file vs its URDF counterpart.
See also #10294 for same disease in acrobot, but about the code not the model files.

@EricCousineau-TRI
Copy link
Contributor Author

EricCousineau-TRI commented Mar 25, 2019

Additionally: We have //manipulation/models and //multibody/models. One of these should disappear. (I think //multibody/models; that, or it gets to inherit the //multibody/benchmark models?)

(Also, potentially we just move any generic models into //models? ...)

@jwnimmer-tri
Copy link
Collaborator

Additionally: We have //manipulation/models and //multibody/models. One of these should disappear.

On principle, I think it's completely fine to have a distinct models folder in each of those packages. The two packages have different purposes and audiences, so could plausibly have different model sets.

However, in particular if you want to argue that box and box_small are stupid things to have in multibody/models, then that's a-ok by me to fix that acute problem.

(Also, potentially we just move any generic models into //models? ...)

This seems worse on all counts.

@EricCousineau-TRI
Copy link
Contributor Author

FYI @IanTheEngineer

@jwnimmer-tri
Copy link
Collaborator

I'm not clear on what's still to be done here. We already have more focused issues filed about Acrobot. Do we have other examples of duplication or required clean-ups? Or is the issue about seeking out any other similar problems, but we don't know of any yet?

@jwnimmer-tri
Copy link
Collaborator

Ping @EricCousineau-TRI can we close this now? Or else can you clarify what's needed?

@EricCousineau-TRI
Copy link
Contributor Author

Closing. #10531 is the other more general thing that needs fixing, but that's already tracked. The acrobot-specific bits are also tracked.

Thanks for the ping!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants