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

Issue2806 #2828

Open
wants to merge 6 commits into
base: gz-sim9
Choose a base branch
from
Open

Conversation

GauravKumar9920
Copy link
Contributor

🦟 Bug fix and New feature

Fixes #2806

Summary

Previously, when launching multiple GZ Sim servers, for instance, using the commands gz sim -s shapes.sdf and gz sim -s default.sdf,

If we had used the command gz sim -g ‘fileName’,
It would not have considered the fileName parameter and would have launched any of the servers at random.

This pr introduces a new feature that allows you to -

  • Launch a file by passing the file name as an argument. For instance, you can utilize the command gz sim -g ‘fileName’ to launch the GUI, provided that the server(s) are operational.
  • Warn the user if there are multiple servers running in case where we don't pass the fileName as an argument. eg - gz sim -g. In case there's just one server running it will simply launch the gui of the same.

@github-actions github-actions bot added the 🏛️ ionic Gazebo Ionic label Mar 18, 2025
@jennuine jennuine requested a review from iche033 March 19, 2025 01:37
Copy link
Contributor

@arjo129 arjo129 left a comment

Choose a reason for hiding this comment

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

This diff is absolutely massive for such a small change. I've left some architectural feedback here: #2806 (comment)

Also when committing please sign off with git commit -s.

I think we can break these down into small PRs.

@iche033
Copy link
Contributor

iche033 commented Mar 19, 2025

I think the big diff is also partly due to indentation changes, e.g. https://github.com/gazebosim/gz-sim/pull/2828/files?w=1 shows diff without whitespace changes. Can you undo indentation changes?

Copy link

codecov bot commented Mar 20, 2025

Codecov Report

Attention: Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 68.94%. Comparing base (1509b0f) to head (b29df6a).
Report is 5 commits behind head on gz-sim9.

Files with missing lines Patch % Lines
src/SimulationRunner.cc 66.66% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           gz-sim9    #2828      +/-   ##
===========================================
- Coverage    68.95%   68.94%   -0.01%     
===========================================
  Files          345      345              
  Lines        33332    33354      +22     
===========================================
+ Hits         22983    22997      +14     
- Misses       10349    10357       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: GauravKumar9920 <[email protected]>
@GauravKumar9920 GauravKumar9920 requested a review from arjo129 March 20, 2025 11:52
Copy link
Contributor

@arjo129 arjo129 left a comment

Choose a reason for hiding this comment

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

This is looking a lot better. Most of my comments are more to do with syntax and style. I have one tiny knit regarding returning an incompletely constructed object. For now we should just continue with construction as is. In Jetty we can consider throwing an error.

GauravKumar9920 and others added 3 commits March 21, 2025 16:31
Co-authored-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Gaurav Kumar <[email protected]>
Co-authored-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Gaurav Kumar <[email protected]>
Signed-off-by: GauravKumar9920 <[email protected]>
Signed-off-by: GauravKumar9920 <[email protected]>
@GauravKumar9920 GauravKumar9920 requested a review from arjo129 March 25, 2025 17:12
Copy link
Contributor

@arjo129 arjo129 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for iterating.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏛️ ionic Gazebo Ionic
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

Consider warning users if there is already a Gazebo server running in the background
3 participants