Closed corot closed 9 years ago
Hi Corot, I've tested your pull request and just have some comments:
PR #27: it works fine. I'd just added a PR corot/softkinetic#1 so that the point cloud in shown properly when using softkinetic_camera_ds311_demo.launch.
PR #25: I get wrong image_color and image_mono when color_compression is set to YUY2. I had also this problem in hydro_devel and I was trying to implement a proper conversion to cv::Mat. I don't know if anybody fixed this at some point of time.
Thanks for testing!!!
OK, both points should be fine now. In fact, color images have been always wrong with YUY2, but as (I suppose) most of as are using MJPEG.... passed unnoticed for long!
Could you retry the PR with recent changes?
Great! It works like charm! the color and mono images work well now. I've just added a PR corot/softkinetic#2 suggesting C++ pointer casting
Done! I didn't test it, so I trust you.... :see_no_evil:
Did you try other parameters combinations? Mostly the depth/color frame formats, as I always tried with the values you put as defaults. I wouldn't be very surprised if something crashes with other values....
@ipa-flg can you please review and comment?
Hi, I checked this PR with several parameters combinations and had 1 problem which is fixed in https://github.com/corot/softkinetic/pull/3 After that I have no problem with ROS hydro and DS325.
did this reach consensus to be OK for merge?
@ipa-fmw: I've not seen a response from @ipa-flg?
VR :ok:
That's why I pull it on indigo_dev (@ipa-fmw , by the way, should not we follow the indigo-devel convention?). So it fixes almost all open issues:
As l also ROS-formatted the code, the merge looks quite scary, sorry for not following the 1 change - 1 PR convention. But as it looks that people is using hydro branch, I don't see any problem on merging it on indigo, so people can compare.
I included recent PR #27, but didn't thoroughly tested, so please, @jordi-pages... give it a fast try.
Finally, I renamed parameter minNeighboursInRadius as min_neighbours to respect parameters naming conventions.
So when trying, don't be surprised if something doesn't work at the first try... But the global improve worth the migration.