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

Should we make this header-only? #10

Open
bmagyar opened this issue Apr 20, 2018 · 6 comments
Open

Should we make this header-only? #10

bmagyar opened this issue Apr 20, 2018 · 6 comments

Comments

@bmagyar
Copy link
Member

bmagyar commented Apr 20, 2018

I think we could simplify a couple of controllers but I'd opt for not linking to a lib for it.
Opinions @ros-controls/ros_control ?

@bmagyar
Copy link
Member Author

bmagyar commented Apr 20, 2018

@artivis this could shorten the diff_drive_controller code too

@matthew-reynolds
Copy link
Member

Reviving this to point something small out:

Beyond the normal considerations of header-only (longer compiles, but more chance for the compiler to optimize, etc), tf2 is a build-only dependency (At least since #14), so if this were header-only that would add a little overhead. Realistically though, I would be surprised if there are many cases where you'd want urdf_geometry_parser without tf2.

@bmagyar
Copy link
Member Author

bmagyar commented Apr 5, 2020

After a quick glance at the code... it's not using roscpp anymore and even tf2 is a very very marginal, case, used internally so probably dependencies could be reduced to urdf. Much larger of a chance for a header-only now! ;)

@matthew-reynolds
Copy link
Member

We still need roscpp for our rosconsole outputs (ROS_ERROR, ROS_DEBUG, etc), and for fetching the robot description from the param server. But you're right, we definitely don't need tf2: See #15.

@bmagyar
Copy link
Member Author

bmagyar commented Apr 5, 2020

You are right... While the purist angel on one shoulder says "we could turn this into a C++ header-only library grabbing a string and returning error strings when failing" the little devil on the other one prefers convenience libraries to be...convenient. I side with the devil today

@artivis
Copy link

artivis commented Apr 6, 2020

Note that fetching the param from the param server (nh.getParam) could (should?) be move outside the constructor. After all, it is an UrdfGeometryParser class not an UrdfGeometryFetcherAndParser.

As for the logging, one could replace it with an output argument e.g. UrdfGeometryParser(const std::string& robot_model_str, const std::string& base_link, UrdfGeometryParserException& exception). Or simply throw UrdfGeometryParserException (with class UrdfGeometryParserException : std::runtime_error).

I guess implementing the above two propositions implies important changes, Noetic?

@bmagyar looks like I'm playing on the angel of template side ;)

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

No branches or pull requests

3 participants