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

Zed depth image to laser scan #73

Merged
merged 10 commits into from
Mar 24, 2019
Merged

Zed depth image to laser scan #73

merged 10 commits into from
Mar 24, 2019

Conversation

wraftus
Copy link
Contributor

@wraftus wraftus commented Mar 12, 2019

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

  • I have read the contributing guide.
  • I have referenced relevant issues.
  • I have labeled accordingly.
  • 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.
  • I have tested my changes on real hardware.

@wraftus
Copy link
Contributor Author

wraftus commented Mar 12, 2019

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
Copy link
Contributor Author

wraftus commented Mar 12, 2019

PS, new to ROS, pls be gentle : )

@wraftus wraftus self-assigned this Mar 12, 2019
Copy link
Member

@matthew-reynolds matthew-reynolds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - I'm being nitpicky just because, as you said, this is the first launch file commit, so I want to try to set a bit of a precedent for ones to come.

uwreact_bringup/package.xml Show resolved Hide resolved
uwreact_bringup/launch/zeds.launch Outdated Show resolved Hide resolved
uwreact_bringup/launch/zeds.launch Outdated Show resolved Hide resolved
uwreact_bringup/launch/zeds.launch Outdated Show resolved Hide resolved
uwreact_bringup/launch/zeds.launch Outdated Show resolved Hide resolved
uwreact_bringup/launch/zeds.launch Outdated Show resolved Hide resolved
uwreact_bringup/launch/zeds.launch Show resolved Hide resolved
uwreact_bringup/launch/zeds.launch Outdated Show resolved Hide resolved
uwreact_bringup/launch/zeds.launch Outdated Show resolved Hide resolved
Copy link
Member

@wmmc88 wmmc88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in general i think we could strip out a lot of the topic naming params and have those be set in stone

uwreact_bringup/launch/zeds.launch Outdated Show resolved Hide resolved
uwreact_bringup/launch/zeds.launch Outdated Show resolved Hide resolved
uwreact_bringup/launch/zeds.launch Outdated Show resolved Hide resolved
uwreact_bringup/launch/zeds.launch Outdated Show resolved Hide resolved
@matthew-reynolds
Copy link
Member

Bump @wraftus

Don't want this to go stale.

@matthew-reynolds
Copy link
Member

matthew-reynolds commented Mar 24, 2019

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

Copy link
Member

@wmmc88 wmmc88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good to merge, but def need the zed resolution, framerate etc, to be configurable at launchtime

@matthew-reynolds
Copy link
Member

matthew-reynolds commented Mar 24, 2019

@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
Copy link
Member

wmmc88 commented Mar 24, 2019

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

@wmmc88 wmmc88 merged commit 1ece345 into master Mar 24, 2019
@wmmc88 wmmc88 deleted the zed_depth_to_laser branch March 24, 2019 19:53
@matthew-reynolds
Copy link
Member

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
Copy link
Member

wmmc88 commented Mar 24, 2019

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 😛 I would think this wouldnt be its own feature and is just a small tack-on to #78

@matthew-reynolds
Copy link
Member

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Convert ZED Depth Image to Laser Scan
3 participants