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

Standalone gz sdf executable #1539

Open
wants to merge 23 commits into
base: sdf15
Choose a base branch
from
Open

Conversation

sauk2
Copy link

@sauk2 sauk2 commented Feb 7, 2025

🎉 New feature

Part of gazebosim/gz-tools#7 and a continuation of #1465

Summary

This PR introduces a standalone gz sdf executable following a similar approach to gz-transport. It is a continuation of #1465 with all the review comments addressed.

Test it

Check for UNIT_gz_TEST

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

scpeters and others added 8 commits June 9, 2024 00:00
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
* Fixed cmake to find source file for FrameSemantics
* Updated exe location in ruby script
* Add a dependency on gz-sdformat-sdf to the sdf_descriptions target
* Updated cmd line arguments in executable tests
* Added dummy flags to make tests pass

---------

Signed-off-by: Saurabh Kamat <kamatsaurabh01@gmail.com>
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Saurabh Kamat <kamatsaurabh01@gmail.com>
Signed-off-by: Saurabh Kamat <kamatsaurabh01@gmail.com>
Signed-off-by: Saurabh Kamat <kamatsaurabh01@gmail.com>
@sauk2 sauk2 requested review from azeey and scpeters as code owners February 7, 2025 08:25
@github-actions github-actions bot added 🏛️ ionic Gazebo Ionic 🪵 jetty Gazebo Jetty labels Feb 7, 2025
Signed-off-by: Saurabh Kamat <kamatsaurabh01@gmail.com>
@sauk2 sauk2 requested a review from ahcorde February 7, 2025 13:24
azeey
azeey previously requested changes Feb 10, 2025
src/gz.hh Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be great to use git mv on this file to preserve history. Currently it looks like it's being removed here and added in cmd/gz.hh.

Copy link
Author

Choose a reason for hiding this comment

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

I tried to do this but subsequent modifications made the file too different for git to recognize it

Signed-off-by: Saurabh Kamat <kamatsaurabh01@gmail.com>
@sauk2 sauk2 force-pushed the gz_sdf_executable branch from 289d27e to 7b27baa Compare February 11, 2025 10:13
Signed-off-by: Saurabh Kamat <kamatsaurabh01@gmail.com>
Signed-off-by: Saurabh Kamat <kamatsaurabh01@gmail.com>
Signed-off-by: Saurabh Kamat <kamatsaurabh01@gmail.com>
Signed-off-by: Saurabh Kamat <kamatsaurabh01@gmail.com>
Signed-off-by: Saurabh Kamat <kamatsaurabh01@gmail.com>
@sauk2 sauk2 requested review from azeey and ahcorde February 11, 2025 15:45
@@ -47,5 +47,7 @@ if (GZ_PROGRAM)
COMMENT "Generating full description for spec ${desc_ver}"
VERBATIM)
endforeach()
add_custom_target(sdf_descriptions DEPENDS ${description_targets} ${PROJECT_LIBRARY_TARGET_NAME})
Copy link
Contributor

Choose a reason for hiding this comment

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

Was the removal of PROJECT_LIBRARY_TARGET_NAME from DEPENDS intended?

Copy link
Contributor

Choose a reason for hiding this comment

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

@sauk2 this final question and we are good to go !

Copy link
Author

@sauk2 sauk2 Feb 25, 2025

Choose a reason for hiding this comment

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

Oh sorry, I forgot to reply. This change was made by @azeey as a part of this commit (615048c) of this PR (#1489). He'd be the best person to clarify so tagging him here!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we're adding a dependency on gz-sdformat-sdf below, which is ultimately what is used to generate these files, this target no longer needs to depend on PROJECT_LIBRARY_TARGET_NAME

Signed-off-by: Saurabh Kamat <kamatsaurabh01@gmail.com>
Copy link
Contributor

@j-rivero j-rivero left a comment

Choose a reason for hiding this comment

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

Final comments from the runtime check. Let me know @sauk2 if you need help implementing them.

->expected(0, 1)
->default_val(SDF_PROTOCOL_VERSION);

command->add_option_function<std::string>("-g,--graph",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can not make this work sorry

./bin/gz-sdformat-sdf --graph /usr/share/gz/gz-sim7/worlds/apply_joint_force.sdf 
--graph requires filepath
Run with --help for more information.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry for the confusion. This option needs a following pose or frame text as argument. I have changed the description to reflect this (b15e767).

I have also added a validation that enforces the argument to be either pose or frame.

Now it looks like this in the help output

-g [--graph] TEXT:{pose,frame}
                                Print the PoseRelativeTo or FrameAttachedTo graph by following
                                with either pose or frame as argument respectively.
                                (WARNING: This is for advanced use only and the output may change
                                without any promise of stability)
                                Requires: filepath

Copy link
Contributor

Choose a reason for hiding this comment

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

Much better thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Is still true the message that says "Requires: filepath" ?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it still shows a dependency on filepath because the --graph requires it as this function is called to process that flag.

}

//////////////////////////////////////////////////
void addSdfFlags(CLI::App &_app)
Copy link
Contributor

Choose a reason for hiding this comment

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

The current message is:

Usage: ./bin/gz-sdformat-sdf [OPTIONS] [filepath]

I think that it is not very usable since the expected schema is more something like:

gz-sdformat-sdf [command] [command-options] [filepath]

Somehow: specify that command is the main first argument and that options are really affecting the way that commands work and probably that they don't work without a command. Also great if command can appear before the options and the filepath.

Copy link
Author

Choose a reason for hiding this comment

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

The usage message seems to be generated by gz-utils. I could not find any setting I could change in the executable to reflect this change.

Do you think it would be a good idea to make changes in gz-utils side to bring about this change?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that the relevant changes needs to go into the GzFormatter in https://github.com/gazebosim/gz-utils/blob/fd0a40c09be4fd7b2a40d877bb56d2fa1c7607e3/cli/include/gz/utils/cli/GzFormatter.hpp#L42-L65

It would be great to have them in but it is out of the scope for this PR I would say. Feel free to create an issue or a PR for it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@j-rivero I'm not sure what you mean. The gz-sdformat-sdf is not intended to be used directly, but through the gz tool. When you run gz sdf, it'll be running gz-sdformat-sdf under the hood. So as long as the usage is gz sdf [OPTIONS] ..., which is what the current usage (before this PR) is, I think we're good.

Signed-off-by: Saurabh Kamat <kamatsaurabh01@gmail.com>
Copy link
Contributor

@j-rivero j-rivero left a comment

Choose a reason for hiding this comment

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

Another great contribution @sauk2 , thanks !

@j-rivero
Copy link
Contributor

ops, I think that we need your approval @azeey here.

src/gz.cc Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you use gz mv to move this file to cmd/gz.cc to preserve history? Right now, it looks like this is removed and a new file is being added in cmd/gz.cc.

Copy link
Author

Choose a reason for hiding this comment

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

I tried to do this, but git does not recognize the files as moved due to successive modifications. Locally, I am able to see 'renamed' when I drop the similarity index to 10% but I don't know how to enforce this on GitHub.
Same with gz.hh

src/gz.hh Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you use gz mv to move this file to cmd/gz.hh to preserve history? Right now, it looks like this is removed and a new file is being added in cmd/gz.hh.

auto filepathOpt =
_app.add_option("filepath", opt->filepath,
"Path to an SDFormat file.");
_app.add_flag("-i,--preserve-includes", opt->preserveIncludes,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Many of these options are relevant when the -p flag is active. If you see how the usage is printed in the original gz sdf version, you'll see the relationship:

  -g [ --graph ] <pose, frame> arg  Print the PoseRelativeTo or FrameAttachedTo graph. (WARNING: This is for advanced
                                    use only and the output may change without any promise of stability)
  -p [ --print ] arg                Print converted arg. Note the quaternion representation of the
                                    rotational part of poses and unit vectors will be normalized.
      -i [ --preserve-includes ]    Preserve included tags when printing converted arg (does not preserve merge-includes).
      --degrees                     Pose rotation angles are printed in degrees.
      --snap-to-degrees arg         Snap pose rotation angles to this specified interval in degrees. This value must be
                                    larger than 0, less than or equal to 360, and larger than the defined snap tolerance.
      --snap-tolerance arg          Used in conjunction with --snap-to-degrees, specifies the tolerance at which snapping
                                    occurs. This value must be larger than 0, less than 360, and less than the defined
                                    degrees value to snap to. If unspecified, its default value is 0.01.
      --inertial-stats  arg         Prints moment of inertia, centre of mass, and total mass from a model sdf file.
      --precision arg               Set the output stream precision for floating point numbers. The arg must be a positive integer.
  -h [ --help ]                     Print this help message.

Is there a way to add this type of dependency in CLI11?

Copy link
Author

Choose a reason for hiding this comment

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

Done! I added a dependence on --print.

The usage looks like this now.

Positionals:
  filepath TEXT               Path to an SDFormat file.
                              Required by: --print
                              Required by: --inertial-stats
                              Required by: --graph
                              Required by: --check
                              

Options:
  -h [--help]                 Print this help message and exit
                              
  -v [--version]              Print the current library version
                              
  --force-version             Use a specific library version.
                              
  --versions                  Show the available versions.
                              
  -i [--preserve-includes]    Preserve included tags when printing converted arg (does not preserve merge-includes).
                              Requires: --print
                              
  --degrees                   Printed pose rotations are will be in degrees.
                              Requires: --print
                              
  --expand-auto-inertials     Auto-computed inertial values will be printed.
                              Requires: --print
                              
  --precision INT             Set the output stream precision for floating point numbers.
                              Requires: --print
                              
  --snap-to-degrees INT       Printed rotations are snapped to specified degree intervals.
                              Requires: --print
                              
  --snap-tolerance FLOAT      Printed rotations are snapped if they are within this specified tolerance.
                              Requires: --print
                              
[Option Group: command]
  Command to be executed.
  Options:
    -k [--check]                Check if an SDFormat file is valid.
                                Requires: filepath
                                
    -d [--describe] TEXT=1.12   Print the aggregated SDFormat spec description. Latest version (1.12)
                                
    -g [--graph] TEXT:{pose,frame}
                                Print the PoseRelativeTo or FrameAttachedTo graph by following
                                with either pose or frame as argument respectively.
                                (WARNING: This is for advanced use only and the output may change
                                without any promise of stability)
                                Requires: filepath
                                
    --inertial-stats            Prints moment of inertia, centre of mass, and total mass from a model sdf file.
                                Requires: filepath
                                
    -p [--print]                Print converted arg. Note the quaternion representation of the
                                rotational part of poses and unit vectors will be normalized.
                                Requires: filepath
                                Required by: --snap-tolerance
                                Required by: --snap-to-degrees
                                Required by: --precision
                                Required by: --expand-auto-inertials
                                Required by: --degrees
                                Required by: --preserve-includes

@azeey azeey dismissed their stale review February 26, 2025 19:00

Blocking issues have been addressed

Signed-off-by: Saurabh Kamat <kamatsaurabh01@gmail.com>
Signed-off-by: Saurabh Kamat <kamatsaurabh01@gmail.com>
Signed-off-by: Saurabh Kamat <kamatsaurabh01@gmail.com>
Signed-off-by: Saurabh Kamat <kamatsaurabh01@gmail.com>
Signed-off-by: Saurabh Kamat <kamatsaurabh01@gmail.com>
@sauk2 sauk2 requested a review from azeey February 27, 2025 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏛️ ionic Gazebo Ionic 🪵 jetty Gazebo Jetty
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

5 participants