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

Collada cleanup dependencies #26

Merged
merged 16 commits into from Apr 17, 2018

Conversation

Projects
None yet
2 participants
@clalancette
Copy link
Collaborator

commented Apr 12, 2018

This PR cleans up collada to setup the dependencies more properly, and do some other cleanup. More specifically:

  1. It switches both packages in the repository to package format 2.
  2. It cleans out unnecessary includes from the header files.
  3. It changes from including class_loader.h to class_loader.hpp, as the former is deprecated.
  4. It removes all dependencies from this package to roscpp; only rosconsole is really needed.
  5. It fixes up the dependencies in the package.xml and CMakeLists.txt to properly export dependencies to downstream users, and bring in the correct dependencies everywhere.

clalancette added some commits Apr 12, 2018

Minor cleanups in the CMakeLists.txt.
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Cleanup collada_parser.h to only include what it uses.
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Switch to package format 2 for collada_parser.
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Switch to class_loader.hpp.
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Alphabetize include order.
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Remove collada_parser dependency on roscpp.
In point of fact, it really only depends on rosconsole, so
switch to that as a dependency.  We also have to add in
some additional includes that we were inheriting from ros.h.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Cleanup an obvious merge error.
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Switch collada_urdf to package.xml format 2.
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Add a dependency from collada_urdf to rosconsole.
It actually does require it, so make sure to express that.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Cleanup includes in collada_urdf.h
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Make sure to include what is used in collada_to_urdf.cpp
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Make sure to include what is used in collada_urdf.cpp
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Remove all use of ROS from urdf_to_collada.cpp.
It is really a standalone program, so doesn't need to call
ros::init at all.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Remove unnecessary includes in test_collada_urdf.cpp
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
More cleanup.
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

@clalancette clalancette requested a review from sloretz Apr 12, 2018

@sloretz
Copy link
Collaborator

left a comment

LGTM save one minor comment on urdf_to_collada.cpp

#include <string>
#include <sstream>
#include <vector>

#include <stdint.h>

This comment has been minimized.

Copy link
@sloretz

sloretz Apr 13, 2018

Collaborator

<cstdint>?

This comment has been minimized.

Copy link
@clalancette

clalancette Apr 16, 2018

Author Collaborator

Good call, fixed.

std::string input_filename(argv[1]);
std::string output_filename(argv[2]);

urdf::Model robot_model;
if( !robot_model.initFile(input_filename) ) {
if (!robot_model.initFile(input_filename)) {
ROS_ERROR("failed to open urdf file %s", input_filename.c_str());

This comment has been minimized.

Copy link
@sloretz

sloretz Apr 13, 2018

Collaborator

Might need to replace with std::cerr or #include <ros/rosconsole.h>.

This comment has been minimized.

Copy link
@clalancette

clalancette Apr 16, 2018

Author Collaborator

Yeah, good point. It compiled before, but I'm not sure why. I've changed this to a std::cerr now, since this is a standalone program.

@@ -38,16 +38,13 @@
#define COLLADA_PARSER_COLLADA_PARSER_H

#include <string>
#include <map>
#include <boost/function.hpp>

This comment has been minimized.

Copy link
@sloretz

sloretz Apr 13, 2018

Collaborator

Pre-release show no issues? Removing includes from public headers could break downstream code that is missing includes, but I'm fine with that if a pre-release build succeeds.

This comment has been minimized.

Copy link
@clalancette

clalancette Apr 16, 2018

Author Collaborator

Yeah, the prerelease was fine. That doesn't guarantee that out-of-tree users aren't depending on it, but I think the somewhat small chance of breakage is worth it to cleanup the exported headers.

Fixes from review.
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 16, 2018

Thanks for the review so far! Ready for another pass.

@clalancette clalancette merged commit ac13da6 into kinetic-devel Apr 17, 2018

2 checks passed

Kpr__collada_urdf__ubuntu_xenial_amd64 Build finished.
Details
Lpr__collada_urdf__ubuntu_xenial_amd64 Build finished.
Details

@clalancette clalancette deleted the cleanup branch Apr 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.