search

ros-drivers / freenect_stack

libfreenect based ROS driver
http://ros.org/wiki/freenect_stack
35 stars 52 forks source link

fix tf_prefix leading slash issue #15

Closed jihoonl closed 9 years ago

jihoonl commented 9 years ago

13

jack-oquin commented 9 years ago

I see how this gets rid of the initial '/', but what happens if tf_prefix is actually defined?

piyushk commented 9 years ago

I agree with @joq. The tf_prefix parameter should be removed from the file that doesn''t support it. In case someone is already using the feature in Indigo, I want that launch file to do a hard fail instead of failing without an error. We've already established that we need to fix this issue in Indigo.

@jihoonl Could you remove the tf_prefix param from the file that doesn't support it?

jihoonl commented 9 years ago

Removed.

jack-oquin commented 9 years ago

It should still be possible to make tf_prefix work, by testing for a definition in the code. That seems better.

jihoonl commented 9 years ago

I have merged tf_prefix version and original version. It seems to work for all purpose.

@jack-oquin What do you mean by by testing for a definition in the code?

jack-oquin commented 9 years ago

The driver would searchParam(), and if it finds tf_prefix defined anywhere in or above the driver's namespace, it would build the Header.frame_id using the prefix, a '/' character, followed by the rgb_frame_id or depth_frame_id.

piyushk commented 9 years ago

@jack-oquin tf_prefix in code was deprecated in Hydro through most of ROS, and based on past experience adding that parameter in each individual piece of code also introduces problems since it may change the behavior of other legacy code in unexpected ways. Since there is no accepted convention, I prefer the current solution since it matches openni2_launch as well.

piyushk commented 9 years ago

@jihoonl Somethings off in this PR. Given your two latest commits there were no changes. Could you take a look and get back to me?

jihoonl commented 9 years ago

The last two commits were to merge freenect.launch and freenect_tf_prefix.launch

It seemed like freenect_tf_prefix.launch can be used for all purposes(with or without tf_prefix). So I removed no tf_prefix version and renamed freenect_tf_prefix.launch to freenect.launch

piyushk commented 9 years ago

The files changed tab only shows a whitespace change: https://github.com/ros-drivers/freenect_stack/pull/15/files

Since freenect_tf_prefix launch file was the original freenect launch file anyway, it doesn't surprise me that there were no changes. However, it's not clear to me if your problem got solved or if there is anything new to release.

jihoonl commented 9 years ago

Ah ha...it is weird. :( I will get back to the issue after testing.

jihoonl commented 9 years ago

Ah...my bad. the current release version works fine...

I was getting the leading slash warning when I subscribed to depth_registered/points with depth_registration flag off..