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

Remove PCL dependency (hopefully) #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bzcheeseman
Copy link

@bzcheeseman bzcheeseman commented Jun 18, 2018

This makes some assumptions (documented in pcl.h) that are not necessarily true, but I just don't know.

Happy to chat more about design decisions, etc - this was just a quick first attempt at removing the PCL dependency.

Also - I was having a lot of trouble setting up catkin on my machine so I added a docker build that will hopefully help with testing. Just build as normal then you have to run catkin_make within docker (as below)

docker run -it --rm <nanomap image> bash
catkin_make

@peteflorence
Copy link
Owner

Thank you for your PR! Looking forward to reviewing when I get a chance!

Best,
Pete

@peteflorence
Copy link
Owner

Hey @bzcheeseman, this is very nice. I appreciate the attention to detail in dealing with Eigen nicely, etc.

Also I am a big fan of docker, --every project I've done since this one lives in a docker container, so the docker stuff is great.

Can you let me know what configuration you used to test this? Do you know if standard ROS point cloud publishing satisfies the assumption stated on line 57 of pcl.h?

@bzcheeseman
Copy link
Author

@peteflorence I used the tests you provided - which is not that helpful because it doesn't fill the point cloud with anything. As far as I could tell the pcl::PointXYZ is contiguous in memory but I can't say for 100% certain. I was hoping you might have some ideas to test with non-empty point clouds (I don't have anything physical set up to test this on at the moment)

@peteflorence
Copy link
Owner

@bzcheeseman OK yes what we should do is test with a rosbag of data -- that will contain real ros PointCloud2s getting published and we can see how this branch deals with performing Knn queries on them. I can maybe also find a place to make a rosbag available for testing.

And yes, the unit tests are not currently very useful for testing point clouds!

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.

2 participants