-
Notifications
You must be signed in to change notification settings - Fork 340
Standalone gz sim
executable
#2809
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
Conversation
Signed-off-by: Saurabh Kamat <[email protected]>
Signed-off-by: Saurabh Kamat <[email protected]>
Signed-off-by: Saurabh Kamat <[email protected]>
Signed-off-by: Saurabh Kamat <[email protected]>
Signed-off-by: Saurabh Kamat <[email protected]>
Signed-off-by: Saurabh Kamat <[email protected]>
2eda54f
to
87f72ba
Compare
Signed-off-by: Saurabh Kamat <[email protected]>
Signed-off-by: Saurabh Kamat <[email protected]>
…d tests Signed-off-by: Saurabh Kamat <[email protected]>
Signed-off-by: Saurabh Kamat <[email protected]>
Signed-off-by: Saurabh Kamat <[email protected]>
gz sim
executablegz sim
executable
Before looking into the code: Given the client / server architecture I would expect to find something like: |
Thanks a lot @sauk2 ! I put together an hack (or let's call it a "proof of concept", to be more fancy) to understand if this new standalone executable could be used to be able to get a working Once a consensus is reached on this PR, I can try to cleanup the code in https://github.com/traversaro/ign-gazebo/tree/sim-standalone-exe-win to have it in PR shape. |
For clarity, there I used |
That is a good point. Ideally we should go with one code path for all unless we find a good reason or a problem in a particular arch. Maybe wrong but I have always assumed that the ruby code currently spawns two different processes via |
Signed-off-by: Saurabh Kamat <[email protected]>
Signed-off-by: Saurabh Kamat <[email protected]>
@j-rivero and @traversaro Thanks a lot for all your feedback! Sorry for the delay in making changes and responding. I've moved this PR back to draft since I believe further discussion is needed before it's ready for merge. Based on your comments, it made sense to handle the server and GUI as separate processes rather than separate threads. To that end, I've split them into two distinct CLI executables - Handling the Another issue is that logs are not streamed to the terminal in the current implementation. The Additionally, the server and GUI flags are currently defined within their respective executables. I’m exploring ways to extract these flags so they can be included in the main help message - one idea is to invoke each executable with I’d appreciate any suggestions or feedback on these points. Thanks again! |
Thanks a lot for the great work!
We used a similar solution in Gazebo Classic on Windows, and it worked fine.
Interesting. I do not have experience with subprocess.h, but with reproc and tiny-process-lib the subprocess output is automatically printed in the terminal that launched the main process. Perhaps there is some way to achieve this, without the need to manually process the stdout and stderr from the child processes?
Good point. Could it make sense to move the logic of those flags to a simple header-only library, and call it from both the main executable and server/client ones? |
Sounds like a bug/defect in subprocess that would need to be fixed. I think that we can go with the workaround by now if it is producing the expect result and ticket the bug in gz-utils in order not to go too far with this PR.
+1. |
Signed-off-by: Saurabh Kamat <[email protected]>
Signed-off-by: Saurabh Kamat <[email protected]>
@traversaro and @j-rivero, Sorry for the delay in getting back to you, and thanks for the feedback! I initially tried using I've made a few changes that seem to work well. Instead of maintaining separate executables for Now, the server process runs directly from the main executable and the GUI is launched as a separate process using I still need to figure out how to make this work on Windows, so it would be great to get some advice on this! I’d be happy to get your feedback and do let me know if this approach works for you! |
Sorry, I arrived late and I see that you already worked on this. To be honest I always used tiny-process-lib and reproc, so even without using them directly you can check for their implementations to understand more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going slowly through the changes:
Blackbox testing a little detected some differences with current gz behaviour:
- Running launching a simulation and running
gz-gui-main
after it displays the screen to select default worlds. If I run currentgz sim -g
this does not happen and the gui appears with the world that it is being run by the server. - If I use
gz-gui-main
from the command line, select a world, and close the GUI window (using the window manager X) , the command line is still running, not finished.
We can address this problem later no need to do it in this PR but: the PR is an step in the right direction but the current approach does not isolate the gui/server in a way that we can package them separately.
src/cmd/sim_main.cc
Outdated
app.add_flag("--force-version", "Use a particular library version."); | ||
app.add_flag("--versions", "Show the available versions."); | ||
|
||
app.add_flag("-g", opt->launchGui, "Run and manage Gazebo GUI"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
app.add_flag("-g", opt->launchGui, "Run and manage Gazebo GUI"); | |
app.add_flag("-g", opt->launchGui, "Run and manage only the Gazebo GUI"); |
Signed-off-by: Saurabh Kamat <[email protected]>
@traversaro thank you for pointing me in the right direction! I have moved to using the
@j-rivero Thanks for your feedback and for trying out the functionality!
I made some changes to try to fix this issue by running a non-blocking server and manually controlling its lifecycle. Now, when the GUI is closed, the server is shut down manually without having to worry about handling signals. Do give it a look and let me know if you are still getting that problem.
I have started working on this now by wrapping the GUI components within conditional statements. Hoping to push some commits on this soon! |
Signed-off-by: Saurabh Kamat <[email protected]>
Signed-off-by: Saurabh Kamat <[email protected]>
Signed-off-by: Saurabh Kamat <[email protected]>
That is great, thanks! |
Thanks ! Let me clarify one detail: we do want to have the Gazebo server independent of using (and/or linking) any GUI component. This can be achieve by building Gazebo without the GUI (like the option you are adding) but we also should be able to get it done without disabling the GUI and providing independent binaries that we can package in different packages (i.e: gz-sim-server.deb / gz-sim-gui.deb) from a single build. Installing the gz-sim-server should bring a server only experience without all the GUI dependencies.
Yes ! testing now works as expected on Linux. The only caveat is that invoking the GUI display the initial world selection window that I think make no sense to have it there. |
After speaking with the PMC yesterday we agree that the change in this PR is a bit risky to deploy it directly on a released distribution like Ionic, so we probably want it to go into a development version so it gets well testing during the release cycle before we ship it to the users. @sauk2sorry for the late decision, I can take care of migrating it the Jetty branch. The other point to move this forward is to list what is still missing? From my personal testing:
|
Thank you for the feedback! I agree that it would be best to include this in the next version. I appreciate your offering to change the target branch to |
Follow up in #2849 |
🎉 New feature
Related to gazebosim/gz-tools#7 and #2737
Summary
This PR introduces a standalone executable for
gz sim
by following a similar approach togz model
. The new executable is integrated intocmdsim.rb.in
, replacing the existing functionality. It is also a step in the direction of decoupling the server from GUI components, allowing us to buildgz-sim
library without GUI support.Two executables are created -
gz-sim-main
andgz-sim-gui
. Thegz-sim-main
executable contains the code to launch the server in the same process and invokegz-sim-gui
in a separate process. For, Unix-based systems,std::system()
is used, and for Windows,CreateProcess()
fromwindows.h
is used.The majority of the behavior remains consistent, including:
-s
-g
Changes to the launch behaviour
-s
runs the server in the main thread.-g
runs the GUI as a separate process.-s
or-g
launches both the Server and GUI in separate threads. The Server instance is intentionally created non-blocking so its lifecycle can be controlled. ASIGINT
closes both the server and GUI but in case the GUI is closed from the screen (through the windowx
), the server is shutdown manually.Enabling/disabling GUI support
An option
ENABLE_GUI
is provided to enable/disable building with GUI. It isON
by default, and turning itOFF
buildslibgz-sim<version>.so
without any link togz-gui
by not buildinglibgz-sim<version>-gui.so
. The executablegz-sim-main
is also built without any link tolibgz-sim<version>-gui.so
, andgz-sim-gui
executable is not built at all.Additionally, a new job has been added CI to build
gz-sim
without GUI enabled.Test it
To test the executables, check for
UNIT_gz_TEST
andINTEGRATION_log_system
.To build
gz-sim
without GUI support, the library must be built with-DENABLE_GUI=OFF
as a CMake argument. Check linkages usingldd
forlibgz-sim<version>.so
andgz-sim-main
to find no linkages to any GUI related libraries.Checklist
codecheck
passed (See contributing)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.