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

[Proposal] Change asset.data.root_state_w to output velocities at the root link frame rather than COM for all asset types #942

Open
1 of 3 tasks
jtigue-bdai opened this issue Sep 5, 2024 · 3 comments · May be fixed by #966
Assignees
Labels
dev team Issue or pull request created by the dev team

Comments

@jtigue-bdai
Copy link
Collaborator

jtigue-bdai commented Sep 5, 2024

Proposal

. I propose we add processing to the data.root_state_w to transform the linear velocity to the root link frame.

It would also be beneficial to

Motivation

Currently the root_state_w of an asset provides linear velocities of the COM of the root body. Many users assume data is coming in at root frame rather than COM

Alternatives

An alternative is to create a separate property from data.root_state_w that does this. However I think if we have separate properties for at the com and at the body frame. We should make it consistent for all values in root sate (i.e. pose would also need to be consistent with COM).

Checklist

  • I have checked that there is no similar issue in the repo (required)

Acceptance Criteria

Add the criteria for which this task is considered done. If not known at issue creation time, you can add this once the issue is assigned.

  • data.root_state_w provides linear velocity of the link frame instead of the com
    or
  • a separate properties data.root_state_link_w and data.root_state_com_w
@jtigue-bdai jtigue-bdai self-assigned this Sep 5, 2024
@jtigue-bdai jtigue-bdai added the dev team Issue or pull request created by the dev team label Sep 5, 2024
@zoctipus
Copy link
Contributor

zoctipus commented Sep 5, 2024

I like it! I was assuming data.root_state_w is base link rather than COM, and used it to calculate distance between robot and treat. Then I found out A1 is trying to fetch the treat by stretching out it hand forward and realized root_state_w is COM, hahah.

begger

@Mayankm96
Copy link
Contributor

Mayankm96 commented Sep 6, 2024

@zoctipus Root state returns a mix as mentioned here: https://isaac-sim.github.io/IsaacLab/source/api/lab/omni.isaac.lab.assets.html#omni.isaac.lab.assets.ArticulationData.root_state_w

The position and quaternion are of the articulation root’s actor frame. Meanwhile, the linear and angular velocities are of the articulation root’s center of mass frame.

So I don't think for your problem this is correct?

@jtigue-bdai I like your second suggestion more. We should see how much overhead it is for us to do the COM -> link transformation manually. But if it is a lazy buffer, it should be okay 👀

@zoctipus
Copy link
Contributor

zoctipus commented Sep 6, 2024

@Mayankm96 Oh I am sorry, I check code again, I meant to say root_pos_w and confused it with root_state_w(the position part of root_state_w). I was using distance calculated as norm(robot.data.root_pos_w, treat.data.root_pos_w) to calculate the reward for fetching cube. Then I got this stretching behavior and made me think maybe root_pos_w is center of mass position? I remember after I switch to body_state_w, then this stretching hand behavior disappear.

@jtigue-bdai jtigue-bdai linked a pull request Sep 9, 2024 that will close this issue
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev team Issue or pull request created by the dev team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants