uwreact / uwreact_robot

Software behind our fully autonomous FIRST robots

BSD 3-Clause "New" or "Revised" License

8 stars 6 forks source link

Zed depth image to laser scan #73

Closed wraftus closed 5 years ago

wraftus commented 5 years ago

Pull Request

The AMCL node we want to use needs a laser scan as it's input, so we use depth_image_to_laserscan to turn the zed node's depth image to a laser scan. Also, since this is the first real perception pr, we created the perception launch file as well.

Closes #68

Contribution Checklist

[x] I have read the contributing guide.
[x] I have referenced relevant issues.
[x] I have labeled accordingly.
[x] I have detailed my changes as much as possible.

Change Checklist

[ ] I have added tests.
[ ] I have added necessary documentation.
[ ] I have auto formatted using clang-format or yapf.
[x] I have tested my changes on real hardware.

wraftus commented 5 years ago

Wasn't sure if there was a launch file code styler or whatever, so haven't done anything like that. Also unsure where to put documentation, should we make READMEs for each subteam?

wraftus commented 5 years ago

PS, new to ROS, pls be gentle : )

matthew-reynolds commented 5 years ago

Bump @wraftus

Don't want this to go stale.

matthew-reynolds commented 5 years ago

I think that really all of these parameters for the zed_wrapper nodes should live in a yaml config file, but let's push that back to a follow-up PR - I want to get this landed.

Edit: Tracked by #78

matthew-reynolds commented 5 years ago

@wmmc88 Please merge and create follow-up issue to track those changes. Is reconfigurability via the yaml enough (#78), or do you want command-line arguments to change those parameters?

wmmc88 commented 5 years ago

I personally think the launch file should pull defaults from the yaml and override the defaults with cl args

matthew-reynolds commented 5 years ago

My only concern is that as we add cameras, if we want independent control of parameters on each camera, the cl args will be very confusing and bloated.

I suppose my bigger question is "Is this really a feature we want/need?" What do we gain by adding command-line arguments compared to just the yaml? If there is an advantage, then I definitely like defaults from yaml, override with command line, but I'm just not sure we really need that functionality and the complications it adds.

wmmc88 commented 5 years ago

i think that the args would be the same for every type of camera(ex. all zeds run at same resolution and framerate). benefit would be the ability to quickly test different params to see its effects without needed to open a file and edit each time :stuck_out_tongue: I would think this wouldnt be its own feature and is just a small tack-on to #78

matthew-reynolds commented 5 years ago

I don't really think that's a valid benefit IMO, but if all the cameras will have the same parameters then there is very little cost to adding the functionality. Let's add this as a comment on #78?

(Shame the frame rate and resolution aren't dynamically reconfigurable... :( The zed wrapper seems to be somewhat lacking)