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

Fix render engine GUI name argument to be setted in the minimal scene #286

Merged
merged 9 commits into from Sep 21, 2021

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Sep 17, 2021

Signed-off-by: ahcorde ahcorde@gmail.com

🦟 Bug fix

Summary

This PR will set the render engine GUI name as a property of the MainWindow. Then this could be consume by the plugins. Right now the minimal_scene only works with "ogre2" (as a default render engine) this PR fix this and allow to set the render engine not only with the sdf but with the command line argument --render-engine-gui

Checklist

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

@ahcorde ahcorde self-assigned this Sep 17, 2021
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Sep 17, 2021
@osrf-triage osrf-triage added this to Inbox in Core development Sep 17, 2021
Signed-off-by: ahcorde <ahcorde@gmail.com>
@codecov
Copy link

codecov bot commented Sep 17, 2021

Codecov Report

Merging #286 (d7e612c) into main (ddf6aee) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #286      +/-   ##
==========================================
- Coverage   65.37%   65.35%   -0.03%     
==========================================
  Files          32       32              
  Lines        4604     4618      +14     
==========================================
+ Hits         3010     3018       +8     
- Misses       1594     1600       +6     
Impacted Files Coverage Δ
include/ignition/gui/Helpers.hh 100.00% <ø> (ø)
src/Helpers.cc 93.33% <100.00%> (+0.65%) ⬆️
src/MainWindow.cc 96.95% <100.00%> (+0.02%) ⬆️
src/plugins/minimal_scene/MinimalScene.cc 59.83% <100.00%> (-1.01%) ⬇️

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 ddf6aee...d7e612c. Read the comment docs.

@chapulina chapulina added beta Targeting beta release of upcoming collection bug Something isn't working labels Sep 18, 2021
@chapulina chapulina moved this from Inbox to In review in Core development Sep 18, 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.

The approach looks good to me. Can we add tests for the new functions?

include/ignition/gui/Helpers.hh Outdated Show resolved Hide resolved
src/MainWindow.cc Outdated Show resolved Hide resolved
src/MainWindow.cc Outdated Show resolved Hide resolved
src/plugins/minimal_scene/MinimalScene.cc Outdated Show resolved Hide resolved
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
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 pushed some tweaks in d057f65, LGTM. Feel free to merge if you and CI are happy with the changes :)

include/ignition/gui/MainWindow.hh Outdated Show resolved Hide resolved
@ahcorde ahcorde merged commit 72e1066 into main Sep 21, 2021
@ahcorde ahcorde deleted the ahcorde/minimal_scene/render_engine_gui_argument branch September 21, 2021 13:36
Core development automation moved this from In review to Done Sep 21, 2021
@j-rivero j-rivero removed this from Done in Core development May 6, 2022
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 bug Something isn't working 🏯 fortress Ignition Fortress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants