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

SceneBroadcaster: Use double for state publish frequency instead of int #1417

Merged
merged 4 commits into from May 4, 2022

Conversation

Thodoris1999
Copy link
Contributor

🦟 Bug fix

Fixes #1414

Summary

Changed the state_hertz publishing frequency data type from int to double. Previously, worlds with those params set between 0 and 1 would crash due to being rounded to 0 and state_hertz being inverted. Note that dynamic_pose_hertz still gets rounded down to 0 as it is represented as int in AdvertiseOptions, but does not cause a crash.

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

…ency instead of int

Signed-off-by: Theodoros Tyrovouzis <teotyrov@gmail.com>
Signed-off-by: Theodoros Tyrovouzis <teotyrov@gmail.com>
@chapulina chapulina moved this from Inbox to In review in Core development Mar 28, 2022
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, the fix works for me!

Would you be able to add a test to test/integration/scene_broadcaster_system.cc? It will be tricky to check for precise publish times since the plugin uses the system clock, so I think the test could at least check that there's no crash.

src/systems/scene_broadcaster/SceneBroadcaster.cc Outdated Show resolved Hide resolved
@Thodoris1999
Copy link
Contributor Author

I am a little new to this, is there an example for a test for crashing?

Signed-off-by: Theodoros Tyrovouzis <teotyrov@gmail.com>
@Thodoris1999
Copy link
Contributor Author

I think I can make death tests to check for crashes, is it ok? I am asking because I have not seen it in any other one of gazebo's test suites.

@chapulina
Copy link
Contributor

I think I can make death tests to check for crashes, is it ok? I am asking because I have not seen it in any other one of gazebo's test suites.

Oh I don't think we need to go that far. Just exercising that code path and having the test run successfully should be enough to make sure there's no crash. This way, if someone reverts the changes from this PR in the future, the test will crash and we'll notice the regression.

Thanks!

Signed-off-by: Theodoros Tyrovouzis <teotyrov@gmail.com>
@Thodoris1999
Copy link
Contributor Author

Thodoris1999 commented May 4, 2022

I added a test with a simple SDF string. It passes with the current fix but if ran before the patch it crashes with Floating point exception (core dumped) as expected.

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Thanks for iterating!

@chapulina chapulina merged commit e3d8ae7 into gazebosim:ign-gazebo6 May 4, 2022
Core development automation moved this from In review to Done May 4, 2022
@Thodoris1999 Thodoris1999 deleted the hertz_double_fix branch May 4, 2022 16:42
@j-rivero j-rivero removed this from Done in Core development May 6, 2022
@chapulina chapulina added this to Inbox in Core development via automation May 9, 2022
@chapulina chapulina moved this from Inbox to Done in Core development May 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏯 fortress Ignition Fortress
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

SceneBroadcaster's _hertz options crash with doubles
2 participants