-
Notifications
You must be signed in to change notification settings - Fork 276
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
Add CLI to switch to Vulkan & Metal backends #1735
Conversation
Needs version bump |
07700e9
to
85f70ef
Compare
Rebased! |
Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
85f70ef
to
017e577
Compare
rebased. |
{ | ||
serverConfig.SetRenderEngineServerApiBackend(_renderEngineServerApiBackend); | ||
} | ||
|
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.
there is a warning about unused _renderEngineGuiApiBackend
variable
add a check _renderEngineGuiApiBackend
?
+ if (_renderEngineGuiApiBackend != nullptr)
+ {
+ serverConfig.SetRenderEngineGuiApiBackend(_renderEngineGuiApiBackend);
+ }
+
seems to work fine without these changes though
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.
If this is what I remember, then what happened is that I got confused.
If I understood correctly it appears that Gazebo has two ways of setting data from Server to Gui:
- Via CLI when launching the gui (e.g.
gz sim -g --render-engine-gui-api-backend vulkan
) - Via GUI asking server what was the Server launched with (e.g.
gz sim -s --render-engine-gui-api-backend vulkan
and thengz sim -g
)- It doesn't look like this codepath works anymore
- I could be wrong
The reason being is that _renderEngineGui
is being asked for (I didn't write that code) and is later read via message passing by the GUI from server; but from the looks of it, this path is broken or deprecated.
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.
The reason being is that _renderEngineGui is being asked for (I didn't write that code) and is later read via message passing by the GUI from server; but from the looks of it, this path is broken or deprecated.
likely broken. I added the check for _renderEngineGuiApiBackend
in 8a19054 and will need to fix this issue later.
else if (this->dataPtr->apiBackend == "metal") | ||
{ | ||
params["metal"] = "1"; | ||
} |
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.
set params["metal"] = "1"
if this->dataPtr->apiBackend
is empty string on APPLE
?
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.
If I remember correctly, no.
Because the higher level code should insist on using "metal" (and by higher level, I mean MinimalScene).
If the string is empty that means something went wrong with MinimalScene in gz-gui; and gz-gui will initialize Qt with OpenGL instead of Metal. See GzRenderer::SetGraphicsAPI
& RenderThread::SetGraphicsAPI
.
Qt must be initialized with Metal for this to work.
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.
ok I see that the gui does the right thing and set the api backend to metal. I added a change in the sensors system to do the same thing in 5d9a5fc
Signed-off-by: Ian Chen <ichen@openrobotics.org>
Signed-off-by: Ian Chen <ichen@openrobotics.org>
@osrf-jenkins run tests please |
Codecov Report
@@ Coverage Diff @@
## main #1735 +/- ##
==========================================
- Coverage 64.49% 64.47% -0.02%
==========================================
Files 341 342 +1
Lines 27098 27141 +43
==========================================
+ Hits 17477 17500 +23
- Misses 9621 9641 +20
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
@osrf-jenkins run tests please |
🎉 New feature
Order in which PRs must be merged
Summary
This PR adds the following CLI options:
--render-engine-gui-api-backend [arg]
--render-engine-server-api-backend [arg]
--render-engine-api-backend [arg]
The default is
metal
in Apple systems, andopengl
in the rest; and the other option beingvulkan
.This setting gets forwarded to the other components so it actually gets applied.
Test it
After merging all dependent PRs, run:
gz help sim
For help with the new CLI options
To test everything:
gz sim segmentation_camera.sdf -v4 --render-engine-api-backend vulkan
Server only:
gz sim segmentation_camera.sdf -v4 -s -r --render-engine-server-api-backend vulkan
GUI only:
gz sim -v4 -g --render-engine-gui-api-backend vulkan
And see the Ogre.log to check Vulkan is being used.
I use segmentation_camera.sdf because it forces the server to render.
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.