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 Instance() method of Singleton classes #3269

Merged
merged 2 commits into from
Feb 19, 2023

Conversation

traversaro
Copy link
Collaborator

@traversaro traversaro commented Oct 13, 2022

In particular, make sure that all the Instance() methods of the same class always use the same instantiation.

🦟 Bug fix

Fixes conda-forge/gazebo-feedstock#148 .

Summary

This should fix all the cases in which multiple instances of a Singleton classes were observed, as for example in conda-forge/macOS build of Classic Gazebo (see conda-forge/gazebo-feedstock#148 (comment)). The fix was inspired from gazebosim/gz-sim#1725 and from the related PRs:

Thanks a lot to @azeey for suggesting the correct fix in conda-forge/gazebo-feedstock#152 (comment) .

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.

In particular, make sure that all the Instance() methods
of the same class always use the same instantiation.
Copy link
Collaborator

@azeey azeey left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in reviewing this. I have one comment regarding deprecation, otherwise, LGTM!

gazebo/common/SingletonT.hh Show resolved Hide resolved
@traversaro
Copy link
Collaborator Author

What is your opinion regarding merging this @scpeters @azeey ? We just hit a regression involving this patch in conda-forge (see conda-forge/gazebo-feedstock#161), to avoid this in the future it would be great if we could get this eventually in an official release.

Copy link
Collaborator

@azeey azeey left a comment

Choose a reason for hiding this comment

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

+1 for merging.

Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

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

thank you, this looks like a good set of changes

I'm rerunning Ubuntu CI and then it will be ready for merge, though I may need to do some testing with ROS before releasing this

gazebo/common/SingletonT.hh Show resolved Hide resolved
@traversaro
Copy link
Collaborator Author

I'm rerunning Ubuntu CI and then it will be ready for merge, though I may need to do some testing with ROS before releasing this

Sure, we can also just wait for the ROS tests before even merging this PR, I am not particulary in a hurry on this one. I am just afraid that at some point the problem that affected conda-forge macOS builds also started to affect other builds (for example homebrew builds as soon as a new version of Apple Clang is released).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MacOS - Gazebo: Cannot Add Model to World because simulation is not running
3 participants