Skip to content

Change to std::get_if for gz::rendering::Variant for throwless behaviour #1124

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

Merged
merged 4 commits into from
May 2, 2025

Conversation

dheerubhai-101
Copy link
Contributor

🦟 Bug fix

Fixes #516

Summary

Use of std::get for gz::rendering::Variant throws exceptions whenever it is interpreted as the wrong type. Non-critical exceptions when working Gazebo can cause lead to cumbersome debugging. So the code has been changed to use std::get_if for a softer error handling when encountering wrong Variant type.

Two things have been changed

  • Code using try catch with std::get changed to std::get_if in ogre/src/OgreThermalCamera.cc, ogre2/src/Ogre2BoundingBoxMaterialSwitcher.cc and ogre2/src/Ogre2SegmentationMaterialSwitcher.cc
  • std::holds_alternative nested with std::get changed to use only std::get_if for simpler value access in ogre2/src/Ogre2GpuRays.cc and ogre2/src/Ogre2ThermalCamera.cc

The following tests FAILED:

         23 - INTEGRATION_gpu_rays_ogre_gl3plus (Failed)
         88 - REGRESSION_reload_engine_ogre2_gl3plus (Failed)
        216 - UNIT_RenderingIface_TEST_ogre2_gl3plus (Failed)

due to the bug pointed out by @j-rivero.
Reason: Error while loading the library [/gz_ws/install/lib/gz-rendering-9/engine-plugins/libgz-rendering-ogre2.so]: /gz_ws/install/lib/gz-rendering-9/engine-plugins/libgz-rendering-ogre2.so: undefined symbol: _ZThn944_N4Ogre7HlmsPbs19_changeRenderSystemEPNS_12RenderSystemE

Will need your help to test it properly.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • 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-project-automation github-project-automation bot moved this from Inbox to In review in Core development Apr 28, 2025
Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

looks good, just minor style comments

@iche033
Copy link
Contributor

iche033 commented Apr 28, 2025

DCO failed. Can you sign your commits? Click on the button to the right of DCO check and select View details to see how to sign the commits

@dheerubhai-101
Copy link
Contributor Author

hey Ian, I think while I was signing off the DCO, the commits of other contributors have also been included. Is there any way to resolve that? This is my first PR so I am quite unsure and I believe I messed up.

@github-project-automation github-project-automation bot moved this from In review to Done in Core development May 1, 2025
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.

@dheerubhai-101 you closed the PR, are you planning to reopen it again ?

@dheerubhai-101
Copy link
Contributor Author

dheerubhai-101 commented May 1, 2025

@dheerubhai-101 you closed the PR, are you planning to reopen it again ?

Hi @ahcorde yes, I will reopen the PR today. The PR included the commits you made when I pulled and rebased, with my sign off on other contributers commits. I unexpectedly closed the PR while fixing it. I will resubmit the changes soon

@iche033
Copy link
Contributor

iche033 commented May 1, 2025

you changes look good,

Like you mentioned, you signed off commits from other authors. I still see this. Are you able to fix them? You can do a rebase and take only your commits and rebase them on the gz-rendering9 branch

@dheerubhai-101
Copy link
Contributor Author

hi Ian, rebased it as you said. It only includes my commits now. Thanks for the help man

@dheerubhai-101 dheerubhai-101 requested a review from ahcorde May 2, 2025 18:23
@iche033
Copy link
Contributor

iche033 commented May 2, 2025

great, thanks for the contribution!

@iche033 iche033 dismissed ahcorde’s stale review May 2, 2025 21:37

comments addressed

@iche033 iche033 merged commit 99031b7 into gazebosim:gz-rendering9 May 2, 2025
10 of 12 checks passed
@github-project-automation github-project-automation bot moved this from Inbox to Done in Core development May 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏛️ ionic Gazebo Ionic 🪵 jetty Gazebo Jetty
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Throwless Variant?
3 participants