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

Re-add namespace for GUI render event #1826

Merged
merged 1 commit into from
Dec 6, 2022
Merged

Conversation

jennuine
Copy link
Contributor

@jennuine jennuine commented Dec 1, 2022

🦟 Bug fix

Summary

A required namespace was accidentally removed in #1635 , this caused the assumption that the render event was a ignition::gazebo::gui event when it needed to be an ignition::gui event.

To test:

ign gazebo shapes.sdf

then use the Screenshot plugin.

Before PR, nothing happened when clicking the screenshot icon (no popup stating where image was saved and no image saved to ~/.ignition/gui/pictures).

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.

Signed-off-by: Jenn Nguyen <[email protected]>
@jennuine jennuine requested a review from iche033 December 1, 2022 01:15
@jennuine jennuine self-assigned this Dec 1, 2022
@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label Dec 1, 2022
@mjcarroll mjcarroll merged commit 412bfa2 into ign-gazebo3 Dec 6, 2022
@mjcarroll mjcarroll deleted the jennuine/fix_gui_event branch December 6, 2022 15:35
@@ -778,7 +778,7 @@ void IgnRenderer::Render()

if (ignition::gui::App())
{
gui::events::Render event;
ignition::gui::events::Render event;
Copy link
Contributor

@iche033 iche033 Dec 6, 2022

Choose a reason for hiding this comment

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

there are other places in ign-gazebo3 that uses gui::events::Render. Do they need to be updated as well? e.g. https:/gazebosim/gz-sim/blob/ign-gazebo3/src/gui/plugins/grid_config/GridConfig.cc#L90

Copy link
Contributor Author

@jennuine jennuine Dec 6, 2022

Choose a reason for hiding this comment

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

Nope, the bug lived in the sender. Anything listening to the render event doesn't need updated since both a ignition::gui::events::Render and ignition::gazebo::gui::events::Render are being sent: https:/gazebosim/gz-sim/blob/ign-gazebo3/include/ignition/gazebo/gui/GuiEvents.hh

Since both events are being broadcasted, plugins listening to either render event will be caught now (doesn't matter which one but preferably ignition::gui::events::Render since ignition::gazebo::gui::events::Render was removed in fortress)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants