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

Patchworkpp as Library to easily include in other packages #8

Closed
wants to merge 1 commit into from

Conversation

wienans
Copy link

@wienans wienans commented Jan 9, 2023

As Patchworkpp in ROS was only accessible in its own package via a header file i splitted the header file in cpp and hpp file and created a library which gets installed and can be used in other packages.
Additionally i mad some conveniance changes for using it from other packages such that

  • changed output frame id of ground / no ground topic equals input cloud frame id instead of fixed "map" frame id
  • added namespace for class
  • moved parameters in local namespace

The demo.launch worked with my bag files but didn't verify if video and offline_kitti was working. Only checked that they are building and starting.

@seungjae24
Copy link
Member

seungjae24 commented Jan 18, 2023

Hi @wienans. Thank you for having interest on our work!

Your requested codes are great in some aspects, but I should let you know it would be hard to merge your pull request for some reasons. First, you modified some default parameters which shouldn't be changed for some first-visited users. For example, if I run the demo.launch file in your pull request following the original instruction written in the README file, RViz does not visualize the ground and non-ground point cloud because you changed the input point cloud topic in the demo.launch file. I think it can be very confusing to first time users. Second, I don't have any plan to separate source codes from the header file. I intentionally put all definitions and functions in the one file for a convenient usage and easy understanding. However, if I think it is necessary to be separated in source and header file for some reasons, I will update the code. Third, I suggest you to not change the default rviz configuration file(e.g. .rviz files) because there is nothing necessary to be changed. The changes of rviz configuration file generate too much difference in the file changes.

Thank you.

@wienans
Copy link
Author

wienans commented Jan 18, 2023

Hi
Understandable, no Problem. Yeah surely the visualisation is not working and could be easily fixed which I could do but if you not considering to generate external lib from the code it is also not really needed.

@wienans wienans closed this Jan 18, 2023
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.

None yet

2 participants