Add Render Engine Cmd Line #331

JShep1 · 2020-09-03T00:50:09Z

Signed-off-by: John Shepherd [email protected]

Signed-off-by: John Shepherd <[email protected]>

JShep1 · 2020-09-03T19:12:37Z

I believe there may be something going wrong with the gui and server side that I wasn't quite sure how to handle. I think it has to do with where the RenderEnginePlugin Components are created specifically, eg, if the server creates both components in LevelManager.cc, and the gui render engine is specified later, where do we want to create the gui render engine component?

iche033 · 2020-09-03T19:49:39Z

and the gui render engine is specified later

what is the use case you are thinking of?

One use case that came to mind is launching the server first and gui later, i.e. ign gazebo -g --render-engine-gui <engine_name> which I think it's not going to work because we currently can't pass additional args to the gui process through runGui

JShep1 · 2020-09-03T19:58:19Z

and the gui render engine is specified later

what is the use case you are thinking of?

One use case that came to mind is launching the server first and gui later, i.e. ign gazebo -g --render-engine-gui <engine_name> which I think it's not going to work because we currently can't pass additional args to the gui process through runGui

Yes, that was the use case I was thinking of, and I tested it out and it's not working.

iche033 · 2020-09-03T20:11:15Z

src/cmd/cmdgazebo.rb.in

  "\n"\
-  "  --version                   Print Gazebo version information.          \n"\
+  "  --render-engine [arg]        Ignition Rendering engine plugin to load.        \n"\
+  "                               Gazebo will use OGRE2 by default.                \n"\


provide the exact string value that's used? Gazebo will use OGRE2 by default. (ogre2). Mainly just to be consistent with the --physics-engine arg description

Just to confirm, I remember you mentioned we could support specifying the render engine using both the engine name and plugin name (which we could do later)? I like specifying by the engine name which is less error-prone but we should document the inconsistency if we don't plan to address it in the near future.

I agree that the engine name is more user friendly, but the plugin name is more general for when we support custom engines. So I think the end goal would be:

Consistency across physics and rendering

Support for custom engine plugins, referring to them by the shared library name:

--render-engine MyCustomEngine

--physics-engine MyCustomEngine

Support for engine plugins that are shipped with the core libraries either by their shared library name

--render-engine ignition-rendering-ogre2-plugin

--render-engine ignition-physics-dartsim-plugin
or their short name

--render-engine ogre

--render-engine tpesim
To achieve this, we can always try to wrap the passed string into ignition-[physics/rendering]-[name]-plugin.

I agree with all the points above, I was wondering if you have an idea as to how to support binary names? Currently, specifying the regular name of the rendering engine is supported given the call here https://github.com/ignitionrobotics/ign-gazebo/pull/331/files#diff-a15bf0b158d65eb38bd94081b23c4291R2218 which takes "ogre" or "ogre2" and translates that to the actual rendering engine here https://github.com/ignitionrobotics/ign-gazebo/blob/ign-gazebo3/src/rendering/RenderUtil.cc#L1020 there, we would likely need to modify RenderUtil to now accept binary names, right?

I was wondering if you have an idea as to how to support binary names?

I'll add more info on this issue: gazebosim/gz-rendering#100

To be clear, I'd be ok proceeding with this PR as is, and adding support for loading the shared libraries later. I'd just make the change that @iche033 suggested above to say "ogre2" instead of "OGRE2".

Sounds good, fixes made here 88cd727

chapulina

we currently can't pass additional args to the gui process through runGui

I think that might not be necessary. The GUI plugins should get the information from the components the same way as if they has been launched at the same time as the server.

I think what needs to be changed is for the engine to be initialized during the Scene3D::Update call instead of Scene3D::LoadConfig.

src/ign.hh

src/gui/plugins/scene3d/Scene3D.cc

Signed-off-by: John Shepherd <[email protected]>

codecov · 2020-09-08T23:53:03Z

Codecov Report

Merging #331 into ign-gazebo3 will increase coverage by 11.61%.
The diff coverage is 72.29%.

@@               Coverage Diff                @@
##           ign-gazebo3     #331       +/-   ##
================================================
+ Coverage        65.71%   77.32%   +11.61%     
================================================
  Files              127      199       +72     
  Lines             6238    10503     +4265     
================================================
+ Hits              4099     8121     +4022     
- Misses            2139     2382      +243

Impacted Files	Coverage Δ
include/ignition/gazebo/EntityComponentManager.hh	`100.00% <ø> (ø)`
include/ignition/gazebo/Server.hh	`100.00% <ø> (ø)`
include/ignition/gazebo/ServerConfig.hh	`100.00% <ø> (ø)`
include/ignition/gazebo/gui/GuiEvents.hh	`0.00% <0.00%> (ø)`
src/SimulationRunner.hh	`100.00% <ø> (ø)`
.../plugins/component_inspector/ComponentInspector.cc	`9.32% <0.00%> (ø)`
src/systems/breadcrumbs/Breadcrumbs.hh	`100.00% <ø> (ø)`
src/systems/detachable_joint/DetachableJoint.hh	`100.00% <ø> (ø)`
src/systems/diff_drive/DiffDrive.hh	`100.00% <ø> (ø)`
src/systems/joint_controller/JointController.hh	`100.00% <ø> (ø)`
... and 144 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f6b4b0a...e93f8f9. Read the comment docs.

Signed-off-by: John Shepherd <[email protected]>

src/cmd/cmdgazebo.rb.in

iche033 · 2020-09-17T19:01:58Z

I think that might not be necessary. The GUI plugins should get the information from the components the same way as if they has been launched at the same time as the server.

I think this means the --render-engine-gui arg needs to be set when the server is launched and all gui clients would get the same render engine string? I was thinking of a use case that if multiple gui client are launched separately (with -g), they would be able to view the scene using different render engines, e.g. by specifying --render-engine-gui to override what's set by the server. I think this is a nice-to-have feature and can be addressed later.

Signed-off-by: Ian Chen <[email protected]>

chapulina

I get a crash if I choose an unsupported rendering engine, I think this should be fixed before this PR can be merged.

$ ign gazebo shapes.sdf --render-engine ogre3                                                                                                                    
[GUI] [Err] [RenderEngineManager.cc:160] No render-engine registered with name: ogre3                                                       
[GUI] [Err] [RenderUtil.cc:1023] Engine [ogre3] is not supported    
...
/home/chapulina/citadel_ws/install/lib/ign-gazebo-3/plugins/gui/libGzScene3D.so(_ZN8ignition6gazebo2v311IgnRenderer10InitializeEv+0x9d) [0x7
f42e40ef62d] /home/chapulina/citadel_ws/src/ign-gazebo/src/gui/plugins/scene3d/Scene3D.cc:1424
/home/chapulina/citadel_ws/install/lib/ign-gazebo-3/plugins/gui/libGzScene3D.so(_ZN8ignition6gazebo2v312RenderThread10RenderNextEv+0x80) [0x
7f42e40f4280] /home/chapulina/citadel_ws/src/ign-gazebo/src/gui/plugins/scene3d/Scene3D.cc:1786

Also, a quality of life improvement suggestion is displaying the rendering engine components in the component inspector (related to #158). I just realized that the physics engine isn't displayed either.

src/cmd/cmdgazebo.rb.in

Signed-off-by: John Shepherd <[email protected]>

JShep1 · 2020-09-18T02:15:26Z

I get a crash if I choose an unsupported rendering engine, I think this should be fixed before this PR can be merged.

I submitted a fix here e93f8f9 , let me know if you think that's a valid fix, I tried to mimic the physics way of doing it.

Also, a quality of life improvement suggestion is displaying the rendering engine components in the component inspector
(related to #158). I just realized that the physics engine isn't displayed either.

Would you want this to go into this PR or another? [edit] I just assigned myself #158 and will add that suggestion in under that issue

chapulina

This makes it so much easier to mix up different engines 😄

Signed-off-by: John Shepherd <[email protected]> Signed-off-by: Ian Chen <[email protected]> Co-authored-by: Ian Chen <[email protected]> Signed-off-by: Guillaume Doisy <[email protected]>

add render engine cmd line capability

d55adb1

Signed-off-by: John Shepherd <[email protected]>

JShep1 requested review from chapulina and iche033 as code owners September 3, 2020 00:50

github-actions bot added the 🏰 citadel Ignition Citadel label Sep 3, 2020

JShep1 mentioned this pull request Sep 3, 2020

Choose Render Engine from Cmd Line #330

Closed

Please codecheck

f33ccc6

Signed-off-by: John Shepherd <[email protected]>

iche033 reviewed Sep 3, 2020

View reviewed changes

chapulina reviewed Sep 3, 2020

View reviewed changes

src/ign.hh Show resolved Hide resolved

src/gui/plugins/scene3d/Scene3D.cc Outdated Show resolved Hide resolved

John Shepherd added 2 commits September 8, 2020 14:58

Merge branch 'ign-gazebo3' into jshep1/cmd_line_render_engine

e4a8a04

suggested fixes

88cd727

Signed-off-by: John Shepherd <[email protected]>

chapulina linked an issue Sep 14, 2020 that may be closed by this pull request

Choose rendering engine from command line #227

Closed

John Shepherd added 2 commits September 15, 2020 14:52

Merge gazebo3

5f1818c

Signed-off-by: John Shepherd <[email protected]>

Resolve merge conflicts

5871788

Signed-off-by: John Shepherd <[email protected]>

JShep1 requested a review from iche033 September 17, 2020 18:06

iche033 reviewed Sep 17, 2020

View reviewed changes

src/cmd/cmdgazebo.rb.in Outdated Show resolved Hide resolved

update cmd line arg doc

6220ad2

Signed-off-by: Ian Chen <[email protected]>

iche033 approved these changes Sep 17, 2020

View reviewed changes

chapulina suggested changes Sep 17, 2020

View reviewed changes

src/cmd/cmdgazebo.rb.in Outdated Show resolved Hide resolved

John Shepherd added 2 commits September 17, 2020 18:24

Merge branch 'ign-gazebo3' into jshep1/cmd_line_render_engine

0b1ecd5

Format and non-valid render engine update

e93f8f9

Signed-off-by: John Shepherd <[email protected]>

JShep1 requested a review from chapulina September 18, 2020 19:41

chapulina approved these changes Sep 19, 2020

View reviewed changes

chapulina merged commit 55ee349 into ign-gazebo3 Sep 19, 2020

chapulina deleted the jshep1/cmd_line_render_engine branch September 19, 2020 00:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Render Engine Cmd Line #331

Add Render Engine Cmd Line #331

JShep1 commented Sep 3, 2020

JShep1 commented Sep 3, 2020

iche033 commented Sep 3, 2020

JShep1 commented Sep 3, 2020

iche033 Sep 3, 2020

chapulina Sep 3, 2020

JShep1 Sep 8, 2020 •

edited

Loading

chapulina Sep 8, 2020

JShep1 Sep 8, 2020

chapulina left a comment

codecov bot commented Sep 8, 2020 •

edited

Loading

iche033 commented Sep 17, 2020

chapulina left a comment

JShep1 commented Sep 18, 2020 •

edited

Loading

chapulina left a comment

Add Render Engine Cmd Line #331

Add Render Engine Cmd Line #331

Conversation

JShep1 commented Sep 3, 2020

JShep1 commented Sep 3, 2020

iche033 commented Sep 3, 2020

JShep1 commented Sep 3, 2020

iche033 Sep 3, 2020

Choose a reason for hiding this comment

chapulina Sep 3, 2020

Choose a reason for hiding this comment

JShep1 Sep 8, 2020 • edited Loading

Choose a reason for hiding this comment

chapulina Sep 8, 2020

Choose a reason for hiding this comment

JShep1 Sep 8, 2020

Choose a reason for hiding this comment

chapulina left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 8, 2020 • edited Loading

Codecov Report

iche033 commented Sep 17, 2020

chapulina left a comment

Choose a reason for hiding this comment

JShep1 commented Sep 18, 2020 • edited Loading

chapulina left a comment

Choose a reason for hiding this comment

JShep1 Sep 8, 2020 •

edited

Loading

codecov bot commented Sep 8, 2020 •

edited

Loading

JShep1 commented Sep 18, 2020 •

edited

Loading