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

Feat/controller dependencies #990

Open
wants to merge 14 commits into
base: og-develop
Choose a base branch
from

Conversation

cremebrule
Copy link
Contributor

No description provided.

@@ -117,7 +117,7 @@ def opacity(self, opacity):
if self.has_material():
self.material.opacity_constant = opacity
else:
self.set_attribute("primvars:displayOpacity", th.tensor([opacity]))
self.set_attribute("primvars:displayOpacity", np.array([opacity]))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no np here and it's not allowed either. Does set_attribute not unwrap the tensor?

Comment on lines +113 to +114
dict: Default base joint controller config to control this robot's base. Uses velocity
control by default.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to use position control by default

Comment on lines +285 to +296
cur_orn_mat = T.quat2mat(root_link_orn).T @ T.quat2mat(base_orn)
cur_pose = th.zeros((2, 4, 4))
cur_pose[:, :3, :3] = cur_orn_mat
cur_pose[:, 3, 3] = 1.0

local_pose = th.zeros((2, 4, 4))
local_pose[:] = th.eye(4)
local_pose[:, :3, 3] = u_vec[self.base_idx].view(2, 3)

# Rotate the linear and angular velocity to the desired frame
lin_vel_global, _ = T.pose_transform(th.zeros(3), cur_orn, u_vec[self.base_idx[:3]], th.tensor([0, 0, 0, 1]))
ang_vel_global, _ = T.pose_transform(th.zeros(3), cur_orn, u_vec[self.base_idx[3:]], th.tensor([0, 0, 0, 1]))
global_pose = cur_pose @ local_pose
lin_vel_global, ang_vel_global = global_pose[0, :3, 3], global_pose[1, :3, 3]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain what's going on here? What was the original thing doing, what's the new one doing? Very hard to follow!

@@ -115,7 +115,7 @@ def _create_discrete_action_space(self):
raise ValueError("VX300S does not support discrete actions!")

@property
def controller_order(self):
def _raw_controller_order(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General question: it seems to me that this could easily be implemented in the manipulation/articulatedtrunk/locomotion etc robot classes since it's always exactly the same thing for every robot? (e.g. each class calls super and appends its extra element).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for default_controllers.

Comment on lines +605 to +607
if name not in self._cache:
self._cache[name] = self._fcns[name]()
return self._cache[name]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this on purpose: the caching currently already implicitly happens in ControllableObjectViewAPI thanks to its batching mechanism so this actually doesn't do anything except make cache misses just a bit slower.

@@ -7,10 +7,11 @@
import math
from typing import List, Optional, Tuple

import torch
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets remove the as th import then

Comment on lines +62 to +63
trunk:
name: JointController
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we shouldn't need this right? Can we remove it? From all of these files?

arm_0:
name: InverseKinematicsController
dependencies: [trunk]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we change this to subsume_controllers: to make it very very explicit what's going on? Also in the code that parses these.

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