ros / roslisp_common

Clone of the old roslisp_common SVN repo from code.ros.org.
8 stars 14 forks source link

Various improvements for TF #23

Closed gaya- closed 9 years ago

gaya- commented 9 years ago

See the commit messages. The second commit solves https://github.com/ros/roslisp_common/issues/19 Fifth commit relates to the corresponding comment discussion.

airballking commented 9 years ago

@gaya- Thanks for the pull request with various fixes! I'd like to merge some of the commits but not all of them.

Unfortunately, https://github.com/gaya-/roslisp_common/commit/cfca1bd46d3ca2d112cbf1fb4430eeff4c0496c2 does not solve https://github.com/ros/roslisp_common/issues/19 The essence of https://github.com/ros/roslisp_common/issues/19 is about cl-tf2 depending on cl-tf-datatypes, which in turn depends on cl-tf. Jan and me already agreed offline that the best solution would be revert to the merged pull request which introduced that dependency and work out a joined cl-interface. Sorry for not documenting this in https://github.com/ros/roslisp_common/issues/19

I'll come back to you to cherry-pick the commits that fit to our plans.

gaya- commented 9 years ago

cl_tf_datatypes does not depend on cl_tf but only on cl_transforms.

If there is something else you don't like please let me know and I'll try to fix it as soon as possible. I have a feeling that PRs are cleaner than cherry picking.

airballking commented 9 years ago

Sorry, I was vague in my statements:

gaya- commented 9 years ago

work out a joined cl-interface

A joined cl-tf-interface could indeed be nice, I'd be into contributing to that if you keep me posted. However, if TF will be deprecated in favour of TF2 soon that might end up being a waste of time.

gaya- commented 9 years ago

Any comments on this? It is essential to have this merged in order to proceed with the other PRs hanging on the other repos. @airballking @fairlight1337 If you prefer an in person meeting for resolving the conflicts to a Github discussion, please let me know.

airballking commented 9 years ago

Tomorrow I'll have some coding time off the robot. I'll look into cleaning this up then.

airballking commented 9 years ago

I do not see how to salvage this pull request for two reasons:

I intend to do the following:

https://github.com/ros/geometry_experimental/blob/indigo-devel/tf2_ros%2Finclude%2Ftf2_ros%2Fbuffer_interface.h

https://github.com/ros/geometry_experimental/blob/indigo-devel/tf2%2Finclude%2Ftf2%2Fconvert.h

That will cause braking changes for many users. But so did all PRs, so far. Again, I do not see how to salvage this situation without being radical.

Plus, the biggest changes are caused by moving from cl-tf to cl-tf2. And that is sadly a different story. I hope that a generic interface with support for various datatypes will helps us ease that pain. But switching libraries has costs.

fairlight1337 commented 9 years ago

Breaking changes are fine for me. All this is in need of refactoring anyway.

gaya- commented 9 years ago

Cool features and unwanted changes are intertwined within single commits.

For me all the features were very useful, so I'd appreciate it if I would be given the opportunity to have a discussion on them.

It spreads the usage of cl-tf-datatypes within cl-tf2, while I actually want to remove cl-tf-datatypes entirely from this repository. I intend to do the following: I hope that a generic interface with support for various datatypes will helps us ease that pain.

cl-tf-datatypes is a collection of generic datatypes that reflect the ROS datatypes and can be used by both CL-TF and CL-TF2, just as it is done in ROS (although the names of messages are different in TF and TF2, they are exactly the same thing, as we all know). As I mentioned before, cl-tf-datatypes is completely independent of cl-tf. In order to have a generic interface for both libraries one needs to have common datatypes that both would support and be able to use. In that sense, getting rid of cl-tf-datatypes and introducing a generic interface are contradicting each other. If you don't like something specific in cl-tf-datatypes that is a different story that can be discussed.

airballking commented 9 years ago

Cool features and unwanted changes are intertwined within single commits.

For me all the features were very useful, so I'd appreciate it if I would be given the opportunity to have a discussion on them.

I never meant to say that some features which you provided were not useful. What I meant was: Several commits combine a set of refactorings, new features, or hot-fixes in one. That makes it very hard to pinpoint single commits to merge.

Regarding the representations used in cl-tf, cl-tf2, and cl-tf-datatypes: They are not the same thing. They are compatible in the sense that you can translate them back and forth. They are incompatible in the sense that one set of representations is built with inheritance and the other one through composition.

In order to have a generic interface for both libraries one needs to have common datatypes that both would support and be able to use. In that sense, getting rid of cl-tf-datatypes and introducing a generic interface are contradicting each other.

I do not think so. I work on porting the design of the tf2 C++ library which supports various different data representations to cl-tf2. That design allows users to expose their own datatypes to tf2 through a data interface. Like that, tf2 does not program against a specific representation but an interface. I think this to be a promising avenue. (Especially, with all the new representations for which users want to use tf.) Again, I point to the C++ sources:

https://github.com/ros/geometry_experimental/blob/indigo-devel/tf2_ros%2Finclude%2Ftf2_ros%2Fbuffer_interface.h

https://github.com/ros/geometry_experimental/blob/indigo-devel/tf2%2Finclude%2Ftf2%2Fconvert.h

This is how my current version of the buffer interface looks (simple and advanced): https://github.com/airballking/roslisp_common/blob/tf2-refactoring/cl_tf2/src/buffer-interface.lisp

Any user-defined data representation on which users would like to call transform(...), e.g. transform, point, pose, or mesh, would have to implement this interface: https://github.com/airballking/roslisp_common/blob/tf2-refactoring/cl_tf2/src/data-interface.lisp

Finally, this is the sketeched broadcaster interface: https://github.com/airballking/roslisp_common/blob/tf2-refactoring/cl_tf2/src/broadcaster-interface.lisp

gaya- commented 9 years ago

As long as the datatypes created through composition as you call it are still supported I don't mind if we switch from common datatypes to common interfaces. FYI, if the datatypes will change and will stop being extensions of cl-transforms datatypes the whole CRAM will have to be refactored, excepted the core part. And it's not just the calls related to TF but much-much more, including anything that deals with cl-transforms.

gaya- commented 9 years ago

This PR is dead, so I better close it.