haosulab / ManiSkill

SAPIEN Manipulation Skill Framework, a GPU parallelized robotics simulator and benchmark
https://maniskill.ai/
Apache License 2.0
930 stars 168 forks source link

Implicit copy in Pose.create #657

Closed BlGene closed 2 weeks ago

BlGene commented 4 weeks ago

Hi Team,

this just caused a bug for me and feels a bit inconsistent. My expectation would be that Pose.create creates a new instance, ChatGPT thinks so too ;)

(Pdb) new_pose = Pose.create(start_pose)
(Pdb) id(new_pose)
132378980628320
(Pdb) id(start_pose)
132378980628320
(Pdb) new_pose = Pose.create_from_pq(p=start_pose.get_p(), q=start_pose.get_q())
(Pdb) id(new_pose)
132378980629664

(Pdb) mani_skill.__version__
'3.0.0b10'
StoneT2000 commented 4 weeks ago

I would think so as well lol. Thanks for bringing this up. https://github.com/haosulab/ManiSkill/blob/3086a0fc8fa5523b10bb95e8fc3b5336593c54dc/mani_skill/utils/structs/pose.py#L125

Is the culprit probably. Would also need to clone the original raw pose data. I think the one where you do new_pose = Pose.create_from_pq(p=start_pose.get_p(), q=start_pose.get_q()) might also be problematic since if start_pose changes new_pose might also change.

BlGene commented 4 weeks ago

Yes, I ended up making it do what I want with pos_new = start_pose.get_p().clone().detach() ... new_pose.set_p(pos_new)

I don't know your API well, but it looks to me like these might be reasonable options:

  1. new_pose = start_pose.clone().detach() (the torch way)
  2. new_pose = Pose.create(start_pose) (the numpy way, also works with torch, but not recommended)
StoneT2000 commented 2 weeks ago

closed by #688

Opted to not clone directly since there could be useful optimizations when storing a very large matrix of poses that you might not want to clone and use up memory / time. If you really want to clone it then just clone the raw pose yourself.