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

Add a flag to force headless rendering mode #701

Merged
merged 17 commits into from
Sep 14, 2021
Merged

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Mar 23, 2021

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

🎉 New feature

Summary

This PR depends on gazebosim/gz-rendering#272. This will allow to use the flag --headless to force ignition-rendering to use the headless mode if it's available.

Test it

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • 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

@github-actions github-actions bot added the 🏢 edifice Ignition Edifice label Mar 23, 2021
Signed-off-by: ahcorde <[email protected]>
@chapulina chapulina added enhancement New feature or request rendering Involves Ignition Rendering labels Mar 23, 2021
@codecov
Copy link

codecov bot commented Mar 23, 2021

Codecov Report

Merging #701 (bea1038) into main (f0e235b) will decrease coverage by 0.01%.
The diff coverage is 91.30%.

❗ Current head bea1038 differs from pull request most recent head 8b96f3e. Consider uploading reports for the commit 8b96f3e to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #701      +/-   ##
==========================================
- Coverage   63.28%   63.27%   -0.02%     
==========================================
  Files         238      239       +1     
  Lines       19423    19445      +22     
==========================================
+ Hits        12292    12303      +11     
- Misses       7131     7142      +11     
Impacted Files Coverage Δ
include/ignition/gazebo/ServerConfig.hh 100.00% <ø> (ø)
include/ignition/gazebo/rendering/RenderUtil.hh 100.00% <ø> (ø)
src/rendering/RenderUtil.cc 36.49% <71.42%> (+0.17%) ⬆️
...on/gazebo/components/RenderEngineServerHeadless.hh 100.00% <100.00%> (ø)
src/LevelManager.cc 89.03% <100.00%> (+0.08%) ⬆️
src/ServerConfig.cc 90.33% <100.00%> (+0.15%) ⬆️
src/ign.cc 72.36% <100.00%> (+0.18%) ⬆️
src/systems/sensors/Sensors.cc 71.74% <100.00%> (+0.51%) ⬆️
...int_position_controller/JointPositionController.cc 53.14% <0.00%> (-7.43%) ⬇️
... and 2 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 b766bb8...8b96f3e. Read the comment docs.

@chapulina chapulina added 🏯 fortress Ignition Fortress and removed 🏢 edifice Ignition Edifice labels Apr 1, 2021
@chapulina
Copy link
Contributor

I think the word "headless" can be confusing to Gazebo users. We often use that word to refer to a server-only simulation, without the GUI process.

Maybe headless-rendering?

@chapulina chapulina added the needs upstream release Blocked by a release of an upstream library label Aug 10, 2021
@ahcorde ahcorde marked this pull request as ready for review August 12, 2021 15:52
@ahcorde ahcorde self-assigned this Aug 12, 2021
@ahcorde
Copy link
Contributor Author

ahcorde commented Aug 12, 2021

@mjcarroll @chapulina open for review

@chapulina chapulina changed the title Add a flag to force headless mode Add a flag to force headless rendering mode Aug 12, 2021
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

I just have some minor comments, but the changes LGTM. I haven't tried actually running it headless with the upstream PR though.

I think it would be best to merge this only after gazebosim/gz-rendering#272 gets in, so the functionality is working.

include/ignition/gazebo/ServerConfig.hh Outdated Show resolved Hide resolved
src/cmd/cmdgazebo.rb.in Outdated Show resolved Hide resolved
src/ign.hh Show resolved Hide resolved
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
@chapulina chapulina added the beta Targeting beta release of upcoming collection label Sep 8, 2021
@chapulina chapulina removed the needs upstream release Blocked by a release of an upstream library label Sep 14, 2021
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

I just tried the flag and it works for me, but I haven't tried it in a container without access to the display.

@chapulina chapulina merged commit 05b7bf0 into main Sep 14, 2021
@chapulina chapulina deleted the ahcorde/feature/headless branch September 14, 2021 21:34
WilliamLewww pushed a commit to WilliamLewww/ign-gazebo that referenced this pull request Dec 7, 2021
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>

Co-authored-by: Ian Chen <[email protected]>
Co-authored-by: Louise Poubel <[email protected]>
Signed-off-by: William Lew <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection enhancement New feature or request 🏯 fortress Ignition Fortress rendering Involves Ignition Rendering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants