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.

Sign up for GitHub

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

Jump to bottom

Print an error message when trying to load SDF files that don't contain a <world> #1998

Merged
merged 4 commits into from
Jun 12, 2023
Merged

Print an error message when trying to load SDF files that don't contain a <world> #1998

merged 4 commits into from
Jun 12, 2023

Conversation

Henrique-BO
Copy link
Contributor

@Henrique-BO Henrique-BO commented May 28, 2023 •
edited
Loading

🦟 Bug fix

Fixes #1941

Summary

Added an error message when attempting to load an SDF file that doesn't contain a world.

Checklist

  • Signed all commits for DCO
  • Added tests
  • 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.

@osrf-triage osrf-triage added this to Inbox in Core development May 28, 2023
@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label May 28, 2023
@Henrique-BO
Add error message when trying to load model as world
5a888c9 
Signed-off-by: Henrique-BO <henrique.barrosoliveira@usp.br>
@Henrique-BO
Add SdfWithoutWorld test to Server
611932c 
Signed-off-by: Henrique-BO <henrique.barrosoliveira@usp.br>
ahcorde
ahcorde previously requested changes May 29, 2023
View reviewed changes
src/Server_TEST.cc Outdated
Comment on lines 1102 to 1103
serverConfig.SetSdfFile(std::string(PROJECT_SOURCE_PATH) +
"/test/worlds/models/sphere/model.sdf");
Copy link
Contributor

Choose a reason for hiding this comment

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

use common::joinPaths

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 9059a2b.

src/Server_TEST.cc Outdated
Comment on lines 1123 to 1126
}


// Run multiple times. We want to make sure that static globals don't cause
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
}
// Run multiple times. We want to make sure that static globals don't cause
}
// Run multiple times. We want to make sure that static globals don't cause

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 9059a2b.

Core development automation moved this from Inbox to In review May 29, 2023
@Henrique-BO
Use joinPaths and fix codecheck
9059a2b 
Signed-off-by: Henrique-BO <henrique.barrosoliveira@usp.br>
@codecov
Copy link

codecov bot commented May 30, 2023 •
edited
Loading

Codecov Report

Merging #1998 (15f98e0) into ign-gazebo3 (3dd77a2) will increase coverage by 0.04%.
The diff coverage is 100.00%.

❗ Current head 15f98e0 differs from pull request most recent head bee3130. Consider uploading reports for the commit bee3130 to get more accurate results

@@               Coverage Diff               @@
##           ign-gazebo3    #1998      +/-   ##
===============================================
+ Coverage        70.71%   70.75%   +0.04%     
===============================================
  Files              262      262              
  Lines            16759    16763       +4     
===============================================
+ Hits             11851    11861      +10     
+ Misses            4908     4902       -6
Impacted Files Coverage Δ
src/Server.cc 84.70% <100.00%> (+0.36%) ⬆️

... and 1 file with indirect coverage changes

@azeey azeey changed the title Throw an error when trying to load sdf files that don't contain a <world> Print an error message when trying to load SDF files that don't contain a <world> Jun 1, 2023
azeey
azeey reviewed Jun 7, 2023
View reviewed changes
src/Server.cc Outdated
@@ -182,6 +182,12 @@ Server::Server(const ServerConfig &_config)
return;
}

if (this->dataPtr->sdfRoot.WorldCount() == 0)
{
ignerr << "SDF file doesn't contain a world.\n";
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be helpful to add that if the user wants to spawn a model, they could use the ResourceSpawner GUI plugin or the world/<world name/create service.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in bee3130.

@Henrique-BO
Update error message
bee3130 
Signed-off-by: Henrique-BO <henrique.barrosoliveira@usp.br>
@azeey
Copy link
Contributor

azeey commented Jun 7, 2023

@osrf-jenkins run tests please

@azeey azeey requested a review from ahcorde June 7, 2023 22:14
@azeey
Copy link
Contributor

azeey commented Jun 12, 2023

CI is broken on macOS, so I'll go ahead and merge this.

@azeey azeey dismissed ahcorde’s stale review June 12, 2023 20:11

Feedback has been addressed

@azeey azeey merged commit 1038a35 into gazebosim:ign-gazebo3 Jun 12, 2023
6 of 7 checks passed
Core development automation moved this from In review to Done Jun 12, 2023
@azeey
Copy link
Contributor

azeey commented Jun 12, 2023

Note, this should not be forward ported to gz-sim7 because loading model files directly is supported in that branch.

@quarkytale quarkytale linked an issue Jun 13, 2023 that may be closed by this pull request
[BUG] gazebo citadel fails to load sdf files that aren't a part of a <world> but doesn't throw an error. #1941
Closed
@azeey azeey mentioned this pull request Jun 14, 2023
Merged
@Henrique-BO Henrique-BO deleted the load_model_error branch July 17, 2023 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@azeey azeey azeey approved these changes

@mjcarroll mjcarroll Awaiting requested review from mjcarroll mjcarroll is a code owner

@ahcorde ahcorde Awaiting requested review from ahcorde

Assignees
No one assigned
Labels
🏰 citadel Ignition Citadel
Projects
Core development
Archived in project
Core development
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] gazebo citadel fails to load sdf files that aren't a part of a <world> but doesn't throw an error.
3 participants
@Henrique-BO @azeey @ahcorde