Allow loading a model SDF file in the Server class #1775

JoanAguilar · 2022-11-01T21:21:47Z

🎉 New feature

Summary

The constructor of the Server class has been updated to handle SDF files that contain a model without a world. The model is loaded into the default world.

Test it

One should be able to create Server instances using model SDF files, for instance: test/worlds/models/sphere/model.sdf.

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.

codecov · 2022-11-01T22:40:43Z

Codecov Report

Merging #1775 (2131fc7) into gz-sim7 (6a8594b) will increase coverage by 0.05%.
The diff coverage is 68.64%.

@@             Coverage Diff             @@
##           gz-sim7    #1775      +/-   ##
===========================================
+ Coverage    64.16%   64.21%   +0.05%     
===========================================
  Files          335      336       +1     
  Lines        26508    26864     +356     
===========================================
+ Hits         17008    17251     +243     
- Misses        9500     9613     +113

Impacted Files	Coverage Δ
include/gz/sim/SystemLoader.hh	`100.00% <ø> (ø)`
include/gz/sim/detail/EntityComponentManager.hh	`93.86% <ø> (ø)`
include/gz/sim/rendering/RenderUtil.hh	`100.00% <ø> (ø)`
.../plugins/component_inspector/ComponentInspector.hh	`28.57% <ø> (ø)`
src/gui/plugins/scene_manager/GzSceneManager.cc	`17.58% <0.00%> (-0.60%)`	⬇️
...rc/systems/ackermann_steering/AckermannSteering.hh	`100.00% <ø> (ø)`
src/systems/track_controller/TrackController.cc	`46.80% <ø> (+0.19%)`	⬆️
.../systems/triggered_publisher/TriggeredPublisher.hh	`100.00% <ø> (ø)`
.../plugins/component_inspector/ComponentInspector.cc	`5.46% <2.77%> (-0.33%)`	⬇️
...s/multicopter_motor_model/MulticopterMotorModel.cc	`75.91% <33.33%> (-0.22%)`	⬇️
... and 16 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Joan Aguilar Mayans <joan@openrobotics.org>

mjcarroll · 2022-11-02T13:13:20Z

Since UNIT_Server_TEST is already pretty close to the timeout when run in Github Actions (at least, maybe other places as well), I'm a little concerned about adding tests that also depend on rendering as well.

Do you think that you could break this out into UNIT_Server_Rendering_TEST to make it a separate executable and also contain the rendering dep?

azeey

Looks great and works in debug builds. The only blocking comment is about the use of asserts. I also agree with @mjcarroll that we should put the test in a separate file.

src/Server.cc

azeey · 2022-11-02T16:37:35Z

src/Server.cc

+          // world and add the model to it.
+          const sdf::Model model = *(sdfRoot.Model());
+          errors = this->dataPtr->sdfRoot.LoadSdfString(DefaultWorld::World());
+          assert(this->dataPtr->sdfRoot.WorldCount() == 1);


I don't think we use assert in our code base very much, so this seems out of place, but I'm not opposed to using it.

Yeah, I was worndering about that. The other options that come to mind would be to catch this via if statement, or leave this uncaught. I have no strong preference, I'll go with whatever you-all think is best.

I think it would be sufficient to add an if statement and returning early if world is nullptr.

azeey · 2022-11-02T16:42:41Z

src/Server_TEST.cc

+      auto loadedEngNames = gz::rendering::loadedEngines();
+      if (loadedEngNames.empty())
+      {
+        gzdbg << "No rendering engine is loaded yet" << std::endl;


This works, but I think the recommended way to return a failure with a reason is for the function to return an AssertionResult (http://google.github.io/googletest/advanced.html#using-a-function-that-returns-an-assertionresult)

azeey · 2022-11-02T16:43:25Z

src/Server_TEST.cc

+      if (!engine)
+      {
+        gzerr << "Internal error: failed to load engine [" << engineName
+          << "]. Grid plugin won't work." << std::endl;


Is the grid plugin relevant here?

No. FWIW, I took this from RenderingGuiPlugin::FindScene. I'll update this.

src/Server.cc

JoanAguilar · 2022-11-02T17:58:21Z

Do you think that you could break this out into UNIT_Server_Rendering_TEST to make it a separate executable and also contain the rendering dep?

OK, sure. Note that (for now) there's only going to be one test in the new executable: LoadSdfModelRelativeUri.

mjcarroll · 2022-11-02T18:01:33Z

Note that (for now) there's only going to be one test in the new executable: LoadSdfModelRelativeUri.

Yes, I think that will be fine.

Signed-off-by: Joan Aguilar Mayans <joan@openrobotics.org>

* Remove unnecessary sdf::Root constructor call. * Avoid an unnecessary copy of an sdf::Model instance. * Remove asserts. Signed-off-by: Joan Aguilar Mayans <joan@openrobotics.org>

Signed-off-by: Joan Aguilar Mayans <joan@openrobotics.org>

JoanAguilar · 2022-11-02T21:33:40Z

Previous comments should be addressed now:

LoadSdfModelRelativeUri is now on its own file.
I have removed the asserts.
I have updated FindScene to use gtest macros to determine success (rather than returning a boolean).
Some other minor changes.

mjcarroll · 2022-11-03T01:35:09Z

Looking better, I think that Windows and macOS are having failures in the test that you added. The Ubuntu failure is known and can be ignored.

azeey · 2022-11-03T03:56:35Z

Looks like the windows tests are failing on the stable CI as well https://build.osrfoundation.org/job/ign_gazebo-gz-sim7-win/17/

mjcarroll · 2022-11-03T12:40:28Z

Oops, you are right, I misread that.

Also, I'm interested in why these compiler warnings have all appeared in the Windows build here, I thought that we just addressed that via the #1771

azeey · 2022-11-03T15:08:18Z

Also, I'm interested in why these compiler warnings have all appeared in the Windows build here, I thought that we just addressed that via the #1771

This branch needs to merge from gz-sim7. Unfortunately, when we enabled the ability to merge a PR without being up-to-date with the target branch, github removed the "Update branch" button.

Signed-off-by: Joan Aguilar Mayans <joan@openrobotics.org>

src/Server_Rendering_TEST.cc

azeey · 2022-11-04T16:58:15Z

src/Server_TEST.cc

@@ -519,6 +519,18 @@ TEST_P(ServerFixture, SdfStringServerConfig)
  EXPECT_EQ(2u, *server.SystemCount());
 }

+/////////////////////////////////////////////////
+TEST_P(ServerFixture, LoadSdfModel)


Can this be removed or be added to the other file? We need to reduce the number of tests in Server_TEST.cc.

Fine with me. This test will then be in Server_Rendering_TEST.cc and not require rendering, but I understand we are OK with that. (Or I guess I could just remove it.)

src/Server_Rendering_TEST.cc

src/CMakeLists.txt

Signed-off-by: Joan Aguilar Mayans <joan@openrobotics.org>

JoanAguilar · 2022-11-06T02:01:28Z

I pushed another batch of changes:

I added Server_Rendering_TEST.cc to the list of tests_needing_display.
Per comment, I disabled LoadSdfModelRelativeUri for macOS.
I moved LoadSdfModel to Server_Rendering_TEST.cc. I think there is some value in keeping it (for instance, LoadSdfModel will run on macOS, while LoadSdfModelRelativeUri won't); however, it does feel a bit misplaced.
I made tests on Server_Rendering_TEST.cc run only once.

CI looks a lot more green now, but that might be in part because LoadSdfModelRelativeUri does not seem to run in some of the builds.

edit: looking at the logs, it seems that LoadSdfModelRelativeUri only runs on the Ubuntu jenkins job, and that LoadSdfModel runs in the Ubuntu and macOS jenkins jobs.

As a side note: after I moved UNIT_Server_Rendering_TEST to the list of tests_needing_display it doesn't run anymore on my local build. I wonder if the fact that I'm using Docker + rocker is interfering with the logic in charge of detecting a valid display (display capabilities seem to work fine, as I'm able to launch a GUI windows). And talking about GUI windows, there also seems to be a set of GUI tests that are not in the tests_needing_display list, that run regardless (and spawn new windows while they run).

Signed-off-by: Joan Aguilar Mayans <joan@openrobotics.org>

mjcarroll · 2022-11-07T20:51:51Z

As a side note: after I moved UNIT_Server_Rendering_TEST to the list of tests_needing_display it doesn't run anymore on my local build

It's likely that the detection is broken because of missing application. I believe that you need to have env DISPLAY set, and then you need xwininfo from x11-utils and glxinfo from mesa-utils.

Currently, these things will come back as "STATUS" messages, but maybe we can elevate them to warning to make it more obvious.

azeey · 2022-11-08T15:53:22Z

src/Server.cc

+          // world and add the model to it.
+          const sdf::Model model = *(sdfRoot.Model());
+          errors = this->dataPtr->sdfRoot.LoadSdfString(DefaultWorld::World());
+          assert(this->dataPtr->sdfRoot.WorldCount() == 1);


I think it would be sufficient to add an if statement and returning early if world is nullptr.

Signed-off-by: Joan Aguilar Mayans <joan@openrobotics.org>

JoanAguilar requested a review from mjcarroll as a code owner November 1, 2022 21:21

github-actions bot added the 🌱 garden Ignition Garden label Nov 1, 2022

JoanAguilar requested a review from azeey November 1, 2022 21:25

JoanAguilar added 3 commits November 1, 2022 16:49

Handle loading a model SDF file in the Server class.

d2cd203

Signed-off-by: Joan Aguilar Mayans <joan@openrobotics.org>

Add unit test loading a model SDF file.

cdac879

Signed-off-by: Joan Aguilar Mayans <joan@openrobotics.org>

Add unit test that loads a model SDF file with relative URIs.

21d4161

Signed-off-by: Joan Aguilar Mayans <joan@openrobotics.org>

JoanAguilar force-pushed the load-model-sdf-3 branch from 94cc75a to 21d4161 Compare November 2, 2022 00:09

azeey requested changes Nov 2, 2022

View reviewed changes

JoanAguilar added 3 commits November 2, 2022 13:27

Move Server unit tests that require rendering to a separate file.

ebb1f4a

Signed-off-by: Joan Aguilar Mayans <joan@openrobotics.org>

Address review comments.

4500f7c

* Remove unnecessary sdf::Root constructor call. * Avoid an unnecessary copy of an sdf::Model instance. * Remove asserts. Signed-off-by: Joan Aguilar Mayans <joan@openrobotics.org>

Use gtest macros in FindScene.

73e0ff9

Signed-off-by: Joan Aguilar Mayans <joan@openrobotics.org>

JoanAguilar requested a review from azeey November 2, 2022 21:34

Merge remote-tracking branch 'origin/gz-sim7' into load-model-sdf-3

acd016a

Signed-off-by: Joan Aguilar Mayans <joan@openrobotics.org>

azeey reviewed Nov 4, 2022

View reviewed changes

JoanAguilar added 5 commits November 4, 2022 11:34

Merge remote-tracking branch 'origin' into load-model-sdf-3

9890bb8

Signed-off-by: Joan Aguilar Mayans <joan@openrobotics.org>

Add Server_Rendering_TEST.cc to tests_needing_display.

ca5acab

Signed-off-by: Joan Aguilar Mayans <joan@openrobotics.org>

Move LoadSdfModel to Server_Rendering_TEST.cc.

bf30236

Signed-off-by: Joan Aguilar Mayans <joan@openrobotics.org>

Disable LoadSdfModelRelativeUri on macOS.

f322809

Signed-off-by: Joan Aguilar Mayans <joan@openrobotics.org>

Run tests in Server_Rendering_TEST.cc only once.

90f96ad

Signed-off-by: Joan Aguilar Mayans <joan@openrobotics.org>

Remove Server_Rendering_TEST.cc from the initial gtest_sources.

8724f79

Signed-off-by: Joan Aguilar Mayans <joan@openrobotics.org>

JoanAguilar requested a review from azeey November 6, 2022 02:01

Merge remote-tracking branch 'origin/gz-sim7' into load-model-sdf-3

311ba69

Signed-off-by: Joan Aguilar Mayans <joan@openrobotics.org>

azeey approved these changes Nov 8, 2022

View reviewed changes

JoanAguilar added 2 commits November 8, 2022 14:59

Merge remote-tracking branch 'origin/gz-sim7' into load-model-sdf-3

0bfe6cf

Signed-off-by: Joan Aguilar Mayans <joan@openrobotics.org>

Return early if unable to load the default world.

2131fc7

Signed-off-by: Joan Aguilar Mayans <joan@openrobotics.org>

mjcarroll approved these changes Nov 9, 2022

View reviewed changes

mjcarroll merged commit 5a15834 into gazebosim:gz-sim7 Nov 9, 2022

JoanAguilar deleted the load-model-sdf-3 branch November 10, 2022 18:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow loading a model SDF file in the Server class #1775

Allow loading a model SDF file in the Server class #1775

JoanAguilar commented Nov 1, 2022

codecov bot commented Nov 1, 2022 •

edited

Loading

mjcarroll commented Nov 2, 2022

azeey left a comment

azeey Nov 2, 2022

JoanAguilar Nov 2, 2022

azeey Nov 8, 2022

azeey Nov 2, 2022

azeey Nov 2, 2022

JoanAguilar Nov 2, 2022

JoanAguilar commented Nov 2, 2022

mjcarroll commented Nov 2, 2022

JoanAguilar commented Nov 2, 2022

mjcarroll commented Nov 3, 2022

azeey commented Nov 3, 2022

mjcarroll commented Nov 3, 2022

azeey commented Nov 3, 2022

azeey Nov 4, 2022

JoanAguilar Nov 4, 2022 •

edited

Loading

JoanAguilar commented Nov 6, 2022 •

edited

Loading

mjcarroll commented Nov 7, 2022

azeey Nov 8, 2022

Allow loading a model SDF file in the Server class #1775

Allow loading a model SDF file in the Server class #1775

Conversation

JoanAguilar commented Nov 1, 2022

🎉 New feature

Summary

Test it

Checklist

codecov bot commented Nov 1, 2022 • edited Loading

Codecov Report

mjcarroll commented Nov 2, 2022

azeey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoanAguilar commented Nov 2, 2022

mjcarroll commented Nov 2, 2022

JoanAguilar commented Nov 2, 2022

mjcarroll commented Nov 3, 2022

azeey commented Nov 3, 2022

mjcarroll commented Nov 3, 2022

azeey commented Nov 3, 2022

Choose a reason for hiding this comment

JoanAguilar Nov 4, 2022 • edited Loading

Choose a reason for hiding this comment

JoanAguilar commented Nov 6, 2022 • edited Loading

mjcarroll commented Nov 7, 2022

Choose a reason for hiding this comment

codecov bot commented Nov 1, 2022 •

edited

Loading

JoanAguilar Nov 4, 2022 •

edited

Loading

JoanAguilar commented Nov 6, 2022 •

edited

Loading