Skip to content

Change output file of urdf_to_graphiz#144

Merged
scpeters merged 4 commits intoros:masterfrom
hwiedPro:hwiedDFKI-patch-2
Oct 30, 2020
Merged

Change output file of urdf_to_graphiz#144
scpeters merged 4 commits intoros:masterfrom
hwiedPro:hwiedDFKI-patch-2

Conversation

@hwiedPro
Copy link
Copy Markdown
Contributor

Currently urdf_to_graphiz writes the pdf to $ROBOT_NAME.gv/$ROBOT_NAME.pdf in the current working directory. This is problematic especially when using this in scripts, as one would either have to read the XML-file to know where this file was saved or to read the stdout of urdf_to_graphiz

Moreover, it seems more handy to put the pdf in the same directory as the urdf. This way you can find it really easier later on and you don't have to move it to the urdf. Also when there might exist multiple URDFs for the same robot (same robot name) this prevents overwriting of files.

This MR saves the files right beneath the urdf by just appending .gv or .pdf to the input path.

@clalancette
Copy link
Copy Markdown
Contributor

This MR saves the files right beneath the urdf by just appending .gv or .pdf to the input path.

I don't think this is a good idea; what if the URDF is stored in /opt/ros/foxy/robot.urdf ? You wouldn't be able to run this at all unless you were root.

However, I do agree that the current behavior isn't very nice either. Here's what I suggest here:

  1. Add a second, optional command-line argument which is the output file that you want to write to.
  2. If that second command-line argument is given, write out to that.
  3. If that second command-line argument is not given, maintain the previous behavior, which is to write to the current working directory.
  4. Print a warning message if the second command-line argument is not given, saying that this behavior is deprecated.

That way we maintain the current behavior (write to the current directory), but allow users to write it wherever they want.

@hwiedPro
Copy link
Copy Markdown
Contributor Author

hwiedPro commented Oct 17, 2020

@clalancette Done! ;)
Please consider also #143 for enhanced joint information in the generated graph. ;)

Copy link
Copy Markdown
Contributor

@clalancette clalancette 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 to me, thanks for iterating here. @scpeters OK to merge?

Copy link
Copy Markdown
Contributor

@scpeters scpeters 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 to me; I just have one nit about improving the deprecation console message

return -1;
}
if (argc != 3) {
std::cerr << "WARNING: OUTPUT not given. This type of usage is deprecated!" << std::endl;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: I think we should also print the Usage: ... string here in case people aren't sure what OUTPUT means

@hwiedPro
Copy link
Copy Markdown
Contributor Author

hwiedPro commented Oct 30, 2020

looks good to me; I just have one nit about improving the deprecation console message

Thanks for approval, @clalancette and @scpeters. I added the string.

@scpeters scpeters merged commit afd0ae2 into ros:master Oct 30, 2020
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