-
Notifications
You must be signed in to change notification settings - Fork 93
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
kRawDepthColorSpace for depth image #74
Conversation
please take rviz images without camera lens covers too. ◉ Kei Okada On Wed, Sep 14, 2016 at 10:36 AM, Kanae Kochigami notifications@github.com
|
+1 it is known that this color space is better for any Pepper with and without lenses (tested on many Peppers), let's merge it |
I updated pictures. (The place Pepper is located is different in the condition with black lenses and without them though.) Thank you very much for your comments. |
BTW, Is there any difference other than color space and dpeth quality about the publishing frequency?◉ Kei Okada On Wed, Sep 14, 2016 at 5:22 PM, Kanae Kochigami
|
FYI: jsk-ros-pkg/jsk_robot#619 (comment) has better images for depth quality between kDepthColorSpace vs kRawDepthColorSpace |
@k-okada @kochigami As seen in current implementation, IMO it is "best effort". Currently all data are enqueued to one global queue and published as ROS messages in one while loop. So the change from |
agreed with @furushchev comment, it would make a lot of sense to separate lightweight reliable ROS messages from the heavy best effort ones. |
Hi, I also checked other topics. I don't think the frequency of all topics changes significantly between (depth topics)
(other topics) middile frequency: low frequency: (reference)
kRawDepthColorSpace (try-depth-raw branch)
|
ok, so shell we merge this PR ? or need to publish both kRawDepthColorSpace and kDepthColorSpace ? or allow to switch this with json.conf ? |
lgtm the impact on performance doesnt seem to be too significant. What do the maintainers think about this @Karsten1987 @vrabaud @suryaambrose ? |
👍 lgtm |
Seems good to me as well |
Hi, |
kRawDepthColorSpace for depth image
This pull request is to use
kRawDepthColorSpace
instead ofkDepthColorSpace
for depth image.The point cloud from
![pepper_with_lens_kdepthimagecolorspace](https://cloud.githubusercontent.com/assets/7259671/18504857/5dbf8970-7a9f-11e6-9800-c2e7ed5347a7.png)
kRawDepthColorSpace
is more clear.kDepthColorSpace + with black lenses:
kRawDepthColorSpace + with black lenses:
![pepper_with_lens_krawdepthimagecolorspace](https://cloud.githubusercontent.com/assets/7259671/18504868/676780cc-7a9f-11e6-8de3-11c4f7c88f7d.png)
kDepthColorSpace + without black lenses:
![pepper_without_lens_kdepthimagecolorspace](https://cloud.githubusercontent.com/assets/7259671/18504874/6cf78852-7a9f-11e6-9cf2-daab9f575af1.png)
kRawDepthColorSpace + with black lenses:
![pepper_without_lens_krawdepthimagecolorspace](https://cloud.githubusercontent.com/assets/7259671/18504883/77576a60-7a9f-11e6-9c59-56386f8cf873.png)
reference
jsk-ros-pkg/jsk_robot#619
(The last comment says that
kRawDepthColorSpace
has a strong effect when eye covers are removed.)ros-naoqi/pepper_robot#27
(The last comment says that
kRawDepthColorSpace
will be required for SLAM.)