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

Prevent Scene3D 💥 if another scene is already loaded #347

Merged
merged 5 commits into from
Jan 10, 2022

Conversation

chapulina
Copy link
Contributor

🦟 Bug fix

Summary

Current issue:

  1. Start ign gazebo
  2. From the top-right menu, select Scene 3D
  3. 💥

This PR prevents a crash and prints a message explaining to the user what happened.

image

The message will be printed if there's any other plugin already loading a render engine, so it's compatible with both GzScene3D and Scene3D.

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 and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

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

@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label Jan 8, 2022
@chapulina chapulina added OOBE 📦✨ Out-of-box experience bug Something isn't working labels Jan 8, 2022
Signed-off-by: Louise Poubel <[email protected]>
…ionrobotics/ign-gui into chapulina/3/graceful_scene3d

Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
@codecov
Copy link

codecov bot commented Jan 8, 2022

Codecov Report

Merging #347 (9153833) into ign-gui3 (9af7abf) will decrease coverage by 0.05%.
The diff coverage is 64.28%.

Impacted file tree graph

@@             Coverage Diff              @@
##           ign-gui3     #347      +/-   ##
============================================
- Coverage     66.83%   66.77%   -0.06%     
============================================
  Files            25       25              
  Lines          2964     2992      +28     
============================================
+ Hits           1981     1998      +17     
- Misses          983      994      +11     
Impacted Files Coverage Δ
src/plugins/scene3d/Scene3D.cc 43.06% <64.28%> (+0.67%) ⬆️

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 9af7abf...9153833. Read the comment docs.

Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

Changes in these two files are unrelated:

  • src/plugins/scene3d/Scene3D.qml
  • test/integration/scene3d.cc

@chapulina
Copy link
Contributor Author

Changes in these two files are unrelated:
src/plugins/scene3d/Scene3D.qml

This is necessary to display a message to the user about why the plugin failed loading.

test/integration/scene3d.cc

At first I thought that this PR had made the tests less precise, so I relaxed the tolerances. I don't understand how that could be the case though, I think that test may be flaky even without that PR. I could break that into another PR if it's clearer, but since I have the approval I'll go ahead and merge now.

@chapulina chapulina merged commit 419613d into ign-gui3 Jan 10, 2022
@chapulina chapulina deleted the chapulina/3/graceful_scene3d branch January 10, 2022 17:02
@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-01-24-citadel-edifice-fortress/1241/1

@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-03-01-citadel-edifice-fortress/1313/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 🏰 citadel Ignition Citadel OOBE 📦✨ Out-of-box experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants