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

OpenNI apps: Improve handling of command line arguments #5494

Merged
merged 36 commits into from
Jan 31, 2023
Merged

OpenNI apps: Improve handling of command line arguments #5494

merged 36 commits into from
Jan 31, 2023

Conversation

filbert14
Copy link
Contributor

@filbert14 filbert14 commented Oct 27, 2022

Edit: Done! Sorry, I didn't know drafts were a thing. .

Side notes:

  • I modified usage() in openni_color_filter.cpp as well as openni_tracking.cpp to fit the rest of the files (default values as well as structure)
  • I think there is a typo in parse.h at line 212
  • Now that device id is optional (or as it turns out, a lot of cmd line args seem to be optional), do you think it would be a good idea to update the syntax for usage()? (Like in the previous version of openni_color_filter.cpp - [] for optional params, < and > for required params, etc.)

General changes are (just a checklist to ease the review):

  • being more flexible with argument count (tolerance when nothing is being passed since everything would default anyway, so no more if (argc < 2)
  • let device_id be an optional parameter initialized with an empty string, and update args to device_ids
  • and update usage() to fit the actual usage

@filbert14 filbert14 marked this pull request as draft October 28, 2022 17:21
@filbert14 filbert14 marked this pull request as ready for review October 28, 2022 18:15
@filbert14 filbert14 marked this pull request as draft November 1, 2022 09:33
@filbert14 filbert14 marked this pull request as ready for review November 2, 2022 07:43
@larshg
Copy link
Contributor

larshg commented Nov 3, 2022

klt and grab_images feel like they would be a bit challenging to refactor, please inform me if that's on the table ;)

Then perhaps we should do that in a new PR 👍

@filbert14
Copy link
Contributor Author

You are right - sorry, I must have been extremely sleepy

in any case, deal! I will work on it after this is merged, thanks for the review :)

@larshg
Copy link
Contributor

larshg commented Jan 7, 2023

@sfilbertf could you please rebase and then I think its 🚀

@larshg larshg added this to the pcl-1.13.1 milestone Jan 7, 2023
@larshg larshg added changelog: enhancement Meta-information for changelog generation module: apps labels Jan 7, 2023
@filbert14 filbert14 requested a review from larshg January 11, 2023 05:35
@filbert14
Copy link
Contributor Author

Hey, I think it should be done :)

larshg
larshg previously approved these changes Jan 11, 2023
@larshg larshg requested a review from mvieth January 25, 2023 20:37
@mvieth
Copy link
Member

mvieth commented Jan 26, 2023

I think it's a bit problematic that, if users update PCL but try to use the old syntax app_name <device_id> instead of the new app_name -device_id <device_id>, the device id given as the first command line argument will not be used and the user will not even know about it. How about the following: call parse_argument for -device_id, if the argument is not found and argc > 1 and argv[1] does not start with a -, then use argv[1] as the device id. So perhaps like this:
if(pcl::console::parse_argument(argc, argv, "-device_id", device_id) == -1 && argc > 1 && argv[1][0] != '-') device_id = argv[1];

Copy link
Member

@mvieth mvieth left a comment

Choose a reason for hiding this comment

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

Thank you!

@mvieth mvieth changed the title #5491 "Inconsistencies in the command line args" OpenNI apps: Improve handling of command line arguments Jan 31, 2023
@mvieth mvieth merged commit 7b7dabe into PointCloudLibrary:master Jan 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: enhancement Meta-information for changelog generation module: apps
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants