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

Add support for specifying log record period #1636

Merged
merged 4 commits into from
Aug 16, 2022
Merged

Conversation

iche033
Copy link
Contributor

@iche033 iche033 commented Aug 9, 2022

Signed-off-by: Ian Chen [email protected]

🎉 New feature

Summary

Current log recorder records states on every update. By specifying the log record period, the user is able to control how often the simulator records states. A new command line arg --record-period is added (gazebo-classic also has the same arg name)

Test it

Run the INTEGRATION_log_system test.

You can also try enabling log recording through the command line, e.g.

# record at 10ms instead of the default 1ms and save the log file in a `log_test` dir
ign gazebo -v 4 -r --record --record-path ./log_test --record-period 0.01  pendulum_links.sdf

then playback the log file:

ign gazebo -v 4 --playback  ./log_test

You should see that the playback is now choppy due to reduced recording rate.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • 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 added 2 commits August 8, 2022 22:20
Signed-off-by: Ian Chen <[email protected]>
@iche033 iche033 requested a review from chapulina as a code owner August 9, 2022 05:37
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Aug 9, 2022
Signed-off-by: Ian Chen <[email protected]>
@codecov
Copy link

codecov bot commented Aug 9, 2022

Codecov Report

Merging #1636 (f4728cc) into ign-gazebo6 (8c9489d) will increase coverage by 0.07%.
The diff coverage is 94.11%.

@@               Coverage Diff               @@
##           ign-gazebo6    #1636      +/-   ##
===============================================
+ Coverage        64.40%   64.48%   +0.07%     
===============================================
  Files              320      320              
  Lines            25892    25935      +43     
===============================================
+ Hits             16677    16723      +46     
+ Misses            9215     9212       -3     
Impacted Files Coverage Δ
include/ignition/gazebo/EntityComponentManager.hh 100.00% <ø> (ø)
include/ignition/gazebo/ServerConfig.hh 100.00% <ø> (ø)
src/ign.cc 64.83% <80.00%> (+0.22%) ⬆️
src/systems/scene_broadcaster/SceneBroadcaster.cc 92.16% <83.33%> (-0.27%) ⬇️
src/EntityComponentManager.cc 89.38% <100.00%> (+0.02%) ⬆️
src/ServerConfig.cc 90.99% <100.00%> (+0.30%) ⬆️
src/ServerPrivate.cc 82.96% <100.00%> (+0.15%) ⬆️
src/systems/log/LogRecord.cc 79.15% <100.00%> (+1.01%) ⬆️
src/SimulationRunner.cc 91.89% <0.00%> (+0.95%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@chapulina chapulina added the enhancement New feature or request label Aug 9, 2022
Copy link
Contributor

@caguero caguero left a comment

Choose a reason for hiding this comment

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

Just for testing I tried 0.1 and 1 record-periods. I was expecting very choppy results but I think the playback stops for a lot of time. Not sure if there's something going on.

@iche033
Copy link
Contributor Author

iche033 commented Aug 12, 2022

Just for testing I tried 0.1 and 1 record-periods. I was expecting very choppy results but I think they playback stops for a lot of time. Not sure if there's something going on.

I can reproduce the issue. Just dumping my findings here: Found that it's to do with the scene_broadcaster in playback mode. It's not able to pick up periodic changed states and send them to the gui. The logic has implicit assumption that states are updated often enough so that every time it's about to send the states to the gui, there are state changes available. However, log playback now updates at lower rate. You will only see the pendulum move if by some timing coincidence that log playback updates at the same iteration as when scenebroadcaster is about to publish its states.

Unthrottling the scene_broadcaster makes it work but I haven't found a good fix yet

@iche033
Copy link
Contributor Author

iche033 commented Aug 12, 2022

playback scene broadcaster issue should be fixed in f4728cc. Now if no periodic changes are available when scene broadcaster is publishing, I force it to do an offcycle state update the next time it sees periodic changes. This should ensure all changed states from log playback are published to the GUI.

Copy link
Contributor

@caguero caguero left a comment

Choose a reason for hiding this comment

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

Works for me now with 0.1 and 1.

@iche033 iche033 merged commit cd97cdf into ign-gazebo6 Aug 16, 2022
@iche033 iche033 deleted the record_period branch August 16, 2022 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request 🏯 fortress Ignition Fortress
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants