-
Notifications
You must be signed in to change notification settings - Fork 67
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
Replacing pykdl with custom transformations library #105
base: ambf-1.0
Are you sure you want to change the base?
Replacing pykdl with custom transformations library #105
Conversation
… for ambf-1.0 branch
return Frame() | ||
|
||
@staticmethod | ||
def HD(a, alpha, d, theta): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this called HD? Should it not be called DH?
|
||
def __mul__(self, f): | ||
|
||
self.mat_44 = Frame.make_HTM(self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem O.K. Is mat_44 an attribute of Frame class?
|
||
class Frame(object): | ||
|
||
M = Rotation() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason for these two attributes are declared as static? Wouldn't it be better to make them instance attributes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the effort into the PR. This is going to be really useful.
I went over the python files from the original fork and I believe we should make the following changes.
- the rotation.py can be improved. Not sure why the
__new__
operator is being overloaded rather than simply using the__init__
method. - Let's overload the
__repr__
and__str__
methods in all the kinematic classes. - Remove the CMakeLists.txt and package.xml file as this package does not contain any ROS specific code and thus should not include these files.
- The contents of the
__init__.py
files in this PR and the last one seems redundant. Not sure if this is the best way to include the contents of the package. - As you have already noted in your comments, it's better to test the kinematics before integration rather than afterward. So let's create a script where we can test for the most commonly used operations for vectors, rotation matrices, and transforms.
I can work alongside you on these changes but you may need to add me as a collaborator in your fork.
Yes! I have added you as a collaborator as well! |
I'm also bringing the parallel discussion we had over slack to this thread.
|
I was also investigating the actual kdl library. It seems that they have updated their python bindings and now use pybind11 instead of sip. I feel we can try building the library itself -> add it to external/kdl and build both c++ and python libraries in python2 and python3. 2 lib files will be generated for both versions of python, and based on the python interpreter version, the correct one can be sourced? I was simultaneously working on creating a parent CMakeLists file to initiate both repository builds at the same time. The only issue I was facing was finding a way to install files into the build folder itself, instead of a |
This PR is a replacement of pykdl geometric primitives - Vector, Frame, Rotation, Twist, and Wrench. The library needs extensive testing but for the short term, testing for matrix multiplication and inverse should be done.