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

fixing observation bug after resetting envs #53

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

liangpan99
Copy link

@liangpan99 liangpan99 commented Aug 9, 2023

Now we can get correct observations for both policy and discriminator after resetting environments!
All reset methods (default, reference, and fall) are fixed.

@liangpan99 liangpan99 changed the title fix obs bug after resetting envs fixing observation bug after resetting envs Aug 9, 2023
@xbpeng
Copy link
Collaborator

xbpeng commented Sep 3, 2023

Thanks a lot for submitting this fix! I think your method is reasonable, but it might not give the right positions and velocities for the rigid bodies. It looks like you are computing the positions and velocities at each of the keyframes in the motion data, and then directly interpolating those in order in order to get the pos and vel at an intermediate time. But linear interpolation will not necessarily give you the right values, because the relationship between joint rotations and rigid body positions/vel is nonlinear. I think the correct way to calculate the pos and vel at an intermediate timestep is to first compute the interpolated reduced coordinate pose + vel of the character, and then calculate the maximal coordinate positions and velocities from the interpolated pose + vel. But this can get pretty expensive.

Though what you have is certainly between than what's currently in the code. But I'm not sure if we should merge it now, since it is still not quite correct.

@liangpan99
Copy link
Author

liangpan99 commented Sep 3, 2023

Thank you for pointing out this mistake! The correct way to calculate the pos and vel at an intermediate time is indeed to first interpolate them in reduced coordinate space and then transform to maximal coordinate space. Using current codebase, it is easy to do this for computing right global 3D positions for the rigid bodies. I have implemented it in the latest commit. A simple example is shown in the figure below. For computing right global 3D linear velocities, I have not yet found a proper way. Although there are still some problems with the linear velocity calculation, we have been able to obtain better observations than before. I think the current method is already able to meet the needs of some users. It works well in my project.

image

  • I think we needn't to merge this pull request, just keep it open. The current code has very good readability. Merging this code will make it a little more complicated, which is unfriendly to most users. People who find the observation issue Initial state distribution bug when resetting the environment #20 will find this solution themselves.

  • There are two bugs that can be fixed easily and neatly at this moment. Maybe I can submit a new pull request.

  1. The way we compute postions for key rigid bodies is still incorrect because of interpolation in maximal coordinate space. This affects the process of populating the buffer of AMP observations.
class MotionLib():
   def get_motion_state(self, motion_ids, motion_times):
         ...
         key_pos0 = self.gts[f0l.unsqueeze(-1), self._key_body_ids.unsqueeze(0)]
         key_pos1 = self.gts[f1l.unsqueeze(-1), self._key_body_ids.unsqueeze(0)]
         ...
         key_pos = (1.0 - blend_exp) * key_pos0 + blend_exp * key_pos1
         ...

https://github.com/nv-tlabs/ASE/blob/21257078f0c6bf75ee4f02626260d7cf2c48fee0/ase/utils/motion_lib.py#L195C62-L195C62

  1. The localRootObs in config file has the opposite effect because of a small bug in function compute_humanoid_observations_max.
# missing a "not", correct way: "if (not local_root_obs)"
if (local_root_obs):
   root_rot_obs = torch_utils.quat_to_tan_norm(root_rot)
   local_body_rot_obs[..., 0:6] = root_rot_obs

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

Successfully merging this pull request may close these issues.

2 participants