isaac-sim / IsaacLab

Unified framework for robot learning built on NVIDIA Isaac Sim
https://isaac-sim.github.io/IsaacLab
Other
2.3k stars 948 forks source link

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

Open jtigue-bdai opened 2 months ago

jtigue-bdai commented 2 months ago

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

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.

zoctipus commented 2 months ago

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 commented 2 months ago

@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 commented 2 months ago

@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.