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

Improve default names for automatic floating base joints #19164

Closed
RussTedrake opened this issue Apr 10, 2023 · 6 comments · Fixed by #20253
Closed

Improve default names for automatic floating base joints #19164

RussTedrake opened this issue Apr 10, 2023 · 6 comments · Fixed by #20253
Assignees
Labels
component: multibody plant MultibodyPlant and supporting code priority: medium type: usability When a feature is awkward, confusing, or undiscoverable

Comments

@RussTedrake
Copy link
Contributor

See extended discussion in slack.

The choice to use e.g. $world_body1 was made during code review, but this choice did not consider all use cases. People writing optimization problems who reference the joint positions by names in constraints don't want something that verbose. Python users using namedview can't eat the $ in the name.

Request: the default names should be something more like {"_"*N}{body_name} for the smallest N that works.

Although we don't have proper depreciation, the slack message includes some relevant ideas from @jwnimmer-tri .

Related to #19162.

@sherm1
Copy link
Member

sherm1 commented Apr 10, 2023

I will use the new scheme in the new MbP graph-building code I'm working on for #18442

@sherm1
Copy link
Member

sherm1 commented Apr 10, 2023

Q: if the system consists of four model instances of the same humanoid (say), we'll add floating joints to the torsos. The proposed scheme makes those joints torso, _torso, __torso, and ___torso with no clear mapping between name and model instance. Is that OK? We could instead guarantee to add underscores in order of model instance index -- is that worthwhile?

Edit: no need to disambiguate across model instances; OK to have the same joint names in different instances. See below.

@jwnimmer-tri
Copy link
Collaborator

jwnimmer-tri commented Apr 10, 2023

The proposed scheme makes those joints torso, _torso, __torso, and ___torso ...

That's not the proposal (as I understand it). The proposal is to use the model instance name as the basis for the floating joint name. Each humanoid has a unique model instance name. The only de-confliction we need to do is between the implicit floating joint's name, and any other pre-existing joint names within the model. The chance of a joint named humanoid_3 within the humanoid xml seems low.

Edit: Ah, the text above did say body_name, didn't it? That seems wrong. I think it should be model name, for the reasons above.

@sherm1
Copy link
Member

sherm1 commented Apr 10, 2023

A single model instance can include multiple floating bodies -- naming the joints for the bodies as in Russ's proposal seems like the natural approach and least likely to require disambiguation.

@sherm1
Copy link
Member

sherm1 commented Apr 10, 2023

Ah, sorry. I forgot that we can have the same names in multiple model instances. So the only disambiguation we need is if the model contains both a floating body named "xyz" and an unrelated joint named "xyz" in the same model instance (very unlikely!). In that case we would need to make body xyz's joint be "_xyz" to avoid a joint name conflict.

@jwnimmer-tri
Copy link
Collaborator

Fair enough. I'm happy to allow whoever implements this to think carefully about the naming choices and back it up with unit tests.

@sherm1 sherm1 self-assigned this Apr 10, 2023
RussTedrake added a commit to RussTedrake/drake that referenced this issue Apr 13, 2023
Related to RobotLocomotion#19164 -- MbP uses names which are not immediately useable as namedview fields.

After this PR (and now that RobotLocomotion#19174 has landed), we can do e.g.
```
PositionView = namedview("PositionView", plant.GetPositionNames())
StateView = namedview("StateView", plant.GetStateNames())
```
RussTedrake added a commit to RussTedrake/drake that referenced this issue Apr 13, 2023
Related to RobotLocomotion#19164 -- MbP uses names which are not immediately useable as `namedview` fields.

After this PR (and now that RobotLocomotion#19174 has landed), we can do e.g.
```
PositionView = namedview("PositionView", plant.GetPositionNames())
StateView = namedview("StateView", plant.GetStateNames())
```
and all of the MbP introspection variants.
RussTedrake added a commit to RussTedrake/drake that referenced this issue Apr 13, 2023
Related to RobotLocomotion#19164 -- MbP uses names which are not immediately useable as `namedview` fields.

After this PR (and now that RobotLocomotion#19174 has landed), we can do e.g.
```
PositionView = namedview("PositionView", plant.GetPositionNames())
StateView = namedview("StateView", plant.GetStateNames())
```
and all of the MbP introspection variants.
RussTedrake added a commit to RussTedrake/drake that referenced this issue Apr 13, 2023
Related to RobotLocomotion#19164 -- MbP uses names which are not immediately useable as `namedview` fields.

After this PR (and now that RobotLocomotion#19174 has landed), we can do e.g.
```
PositionView = namedview("PositionView", plant.GetPositionNames())
StateView = namedview("StateView", plant.GetStateNames())
```
and all of the MbP introspection variants.
RussTedrake added a commit to RussTedrake/drake that referenced this issue Apr 13, 2023
Related to RobotLocomotion#19164 -- MbP uses names which are not immediately useable as `namedview` fields.

After this PR (and now that RobotLocomotion#19174 has landed), we can do e.g.
```
PositionView = namedview("PositionView", plant.GetPositionNames())
StateView = namedview("StateView", plant.GetStateNames())
```
and all of the MbP introspection variants.
sammy-tri pushed a commit that referenced this issue Apr 13, 2023
Related to #19164 -- MbP uses names which are not immediately useable as `namedview` fields.

After this PR (and now that #19174 has landed), we can do e.g.
```
PositionView = namedview("PositionView", plant.GetPositionNames())
StateView = namedview("StateView", plant.GetStateNames())
```
and all of the MbP introspection variants.

Co-Authored-By: Jeremy Nimmer <jeremy.nimmer@tri.global>
@jwnimmer-tri jwnimmer-tri added type: cleanup type: usability When a feature is awkward, confusing, or undiscoverable and removed type: cleanup labels May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: multibody plant MultibodyPlant and supporting code priority: medium type: usability When a feature is awkward, confusing, or undiscoverable
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants