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 adding system to non-existent entity #2516

Merged
merged 3 commits into from
Aug 7, 2024
Merged

Conversation

iche033
Copy link
Contributor

@iche033 iche033 commented Aug 6, 2024

🦟 Bug fix

Fixes #2508, #2511, and #2513

Summary

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.

@iche033 iche033 requested a review from mjcarroll as a code owner August 6, 2024 00:05
@github-actions github-actions bot added the 🎵 harmonic Gazebo Harmonic label Aug 6, 2024
Copy link
Contributor

@arjo129 arjo129 left a comment

Choose a reason for hiding this comment

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

LGTM. I've only got a minor issue over the logging.

// otherwise check if entity exists before attempting to load the plugin.
else if (!this->entityCompMgr->HasEntity(entity))
{
gzwarn << "Unable to add plugins to Entity: '" << entity
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth setting to gzerr as by default end users don't see any indication of failure unless the enable logging at higher verbosities. In addition the add service should probable return false if we fail to add a object, but by default it always returns true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated this msg and the one above to gzerr. 510a5dd

In addition the add service should probable return false if we fail to add a object, but by default it always returns true.

I noticed we do that in several places in gz-sim so that the requests can be handled asynchronously. The response data true indicates that the request was handled but not necessarily successful. I added a todo note on this. Not sure if there's a way to set response data in an asynchronous way, maybe @caguero knows if it's currently possible?

@azeey azeey added the beta Targeting beta release of upcoming collection label Aug 6, 2024
@arjo129 arjo129 enabled auto-merge (squash) August 7, 2024 02:52
@arjo129 arjo129 merged commit c3479e2 into gz-sim8 Aug 7, 2024
9 checks passed
@arjo129 arjo129 deleted the add_system_check_entity branch August 7, 2024 03:15
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 🎵 harmonic Gazebo Harmonic
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Crash after dynamically adding plugin LiftDrag to non-existing entity
3 participants