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

Readd mode arguments to openni2.launch #102

Open
wants to merge 3 commits into
base: ros1
Choose a base branch
from

Conversation

v-lopez
Copy link

@v-lopez v-lopez commented Dec 1, 2020

These args were lost on 576b616

Afaik there's no other way to specify at startup the mode, and changing the mode after the node is running via dynamic reconfigure is something that we'd like to avoid.

@MatthijsBurgh
Copy link
Contributor

I think it is good to restore this. I suggest keep the documentation line as well as described in 576b616#diff-f75072e949914a0b3f6d5ac4c7307380d93396eadd46d0ddf4c9a98fae6f3e83L21-L24

I suggest to update the tf_prefix launch file as well to keep these in sync.

Might be best to rebase and merge into noetic-devel

@130s 130s changed the base branch from indigo-devel to ros1 February 17, 2023 07:40
@130s
Copy link
Member

130s commented Feb 17, 2023

Thank you for the contribution.
I took a liberty to change the target branch to ros1, which might have not existed when this PR was opened.

I won't have a room to take a closer look nor test myself. Nor have I time to address the suggestion #102 (comment). That said, with @MatthijsBurgh comment, I'm inclined to merging (and depend on the OSS community to test). In a few days I might merge unless we'll receive objections.

Comment on lines +1 to +2
<?xml version="1.0"?>
<package format="3">
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<?xml version="1.0"?>
<package format="3">
<?xml version="1.0"?>
<?xml-model
href="http://download.ros.org/schema/package_format3.xsd"
schematypens="http://www.w3.org/2001/XMLSchema"?>
<package format="3">

@@ -32,6 +32,9 @@
doc="This arg is not used. Preserved only for backward compatibility." />
<arg name="auto_white_balance" default="true"
doc="This arg is not used. Preserved only for backward compatibility." />
<arg name="color_mode" default="5" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<arg name="color_mode" default="5" />
<!-- Resolution and FPS setting for each channel -->
<arg name="color_mode" default="5" />

Copy link
Contributor

@MatthijsBurgh MatthijsBurgh left a comment

Choose a reason for hiding this comment

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

@130s As mentioned in my previous comment. We should also update openni2_tf_prefix.launch before merging this one.
When we have updated that file too. I will approve.
(I don't have write access in this organization, so I can't edit this PR.)

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

Successfully merging this pull request may close these issues.

4 participants