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

Clarify how time is represented in each phase of a System step #467

Merged
merged 1 commit into from
Dec 2, 2020

Conversation

adlarkin
Copy link
Contributor

Signed-off-by: Ashton Larkin ashton@openrobotics.org

@github-actions github-actions bot added the 📜 blueprint Ignition Blueprint label Nov 30, 2020
@adlarkin
Copy link
Contributor Author

One other thing to note is that the initial time is 0 when using the ign gazebo command (i.e., paused simulation), while the initial time is dt if the ign gazebo -r command is used (i.e., starting simulation by default). Would this be something worth adding to the documentation?

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.

This looks reasonable to me. All 3 callbacks are consistent and have the same time for a given step. The relationship between each callback and that sim time is given by the callback's name: PreUpdate happens before that time is reached, Update happens during, PostUpdate happens after.

I think we should also update this tutorial. We may even want to add some more details to the tutorial, like explaining that there's no time zero when the simulation is running.

@adlarkin
Copy link
Contributor Author

I think we should also update this tutorial. We may even want to add some more details to the tutorial, like explaining that there's no time zero when the simulation is running.

Does anyone have a good way to explain why there's no time zero when simulation starts un-paused, but why there's a time zero when simulation starts paused? The ladder case is simply because simulation hasn't started yet, so the simulation time is zero... but, I'm having a harder time figuring out an intuitive way to explain why there's no time zero for the former case.

@chapulina
Copy link
Contributor

Does anyone have a good way to explain why there's no time zero when simulation starts un-paused

When simulation starts unpaused, the very first iteration is already affecting simulation. Since there's no way to alter time zero, there isn't a time zero in this case.

When simulation starts paused, systems will keep seeing time zero until simulation is unpaused.

The same goes for any paused state in the middle of simulation. If the user pauses right after sim time 1 min, all paused updates should keep receiving sim time 1 min, which is in the past, with dt == 0, until simulation is unpaused.

@azeey
Copy link
Contributor

azeey commented Dec 1, 2020

When simulation starts unpaused, the very first iteration is already affecting simulation. Since there's no way to alter time zero, there isn't a time zero in this case.

This might make sense from the view point of being able to alter state, but not from knowing what the current state is. i.e, I think
the state of the world on the very first call to PreUpdate is the state at time zero, even if simTime says it's 0+dt. If a system wants to report the initial state of the world, this would be the time to do it. Of course, if the system uses simTime as the timestamp, it will be incorrect.

So, IMO, we should document that simTime does not refer to the current time, but the time reached after PreUpdate and Update calls have finished. If *Update functions are called with simulation paused, time does not advance; therefore, the time reached after PreUpdate and Update is the same as the starting time, which explains why there is a difference in starting simulation paused and un-paused.

@adlarkin adlarkin requested a review from maryaB-osr as a code owner December 1, 2020 15:46
@adlarkin
Copy link
Contributor Author

adlarkin commented Dec 1, 2020

Thanks, @azeey! I think that's a great explanation. I tried to incorporate this in e8a3401; let me know if you think this should be modified in any way.

One other (minor) thing to note is that since we are using Doxygen for the documentation in System.hh, links are automatically generated whenever a reference is made to the UpdateInfo struct (which is desired behavior). However, since the tutorial is in markdown, I'm wondering if we should add a manual link to UpdateInfo in the tutorial so that readers of the tutorial have a reference to it - is anyone opposed to me adding these links in to the tutorial?

@azeey
Copy link
Contributor

azeey commented Dec 1, 2020

I'm wondering if we should add a manual link to UpdateInfo in the tutorial so that readers of the tutorial have a reference to it - is anyone opposed to me adding these links in to the tutorial?

I'm not opposed to this, but the tutorials are also rendered using Doxygen and if you use the namespace qualified name (eg. ignition::gazebo::UpdateInfo), I believe Doxygen will automatically generate the link. This is nice because the link never has to be updated manually when we update gazebo versions.

@adlarkin
Copy link
Contributor Author

adlarkin commented Dec 1, 2020

I'm not opposed to this, but the tutorials are also rendered using Doxygen and if you use the namespace qualified name (eg. ignition::gazebo::UpdateInfo), I believe Doxygen will automatically generate the link.

Thanks for pointing that out; I wasn't aware we could do this. I've used this approach to add links in the tutorial in 0153ef8.

@adlarkin adlarkin requested a review from chapulina December 1, 2020 16:47
@adlarkin adlarkin force-pushed the adlarkin/system_step_documentation_fix branch from 0153ef8 to 770b1e7 Compare December 2, 2020 13:50
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
@adlarkin adlarkin force-pushed the adlarkin/system_step_documentation_fix branch from 770b1e7 to 6817a52 Compare December 2, 2020 13:51
@adlarkin adlarkin merged commit 6817a52 into ign-gazebo2 Dec 2, 2020
@adlarkin adlarkin deleted the adlarkin/system_step_documentation_fix branch December 2, 2020 14:37
mjcarroll added a commit that referenced this pull request Jan 8, 2021
* Clarify how sim time is interpreted in a System's step (#467)
* add frame_id and child_frame_id attribute support for DiffDrive (#361)
* Add ability to record video based on sim time (#414)
* add ability to record video from gui camera using sim time
* add msg
* use QueryBoolText
* Add lockstep mode to video recording (#419)
* Disable right click menu when using measuring tool (#458)
* Bump to 3.6.0 (#524)
* Don't make docs on macOS (#528)
* Updates to ardupilot migration tutorial (#525)
* Update gtest to 1.10.0 for Windows compilation (ign-gazebo3) (#506)
* Compile new gtest with c++11
* Use INSTANTIATE_TEST_SUITE_P instead of deprecated -INSTANTIATE_TEST_CASE_P
* Apply suggestions from code review
* One more tutorial version bump
* Fix bad merge
* change nullptr to a int ptr for qt 5.15.2 bug (#527)
* Generate valid topics everywhere (support names with spaces) (#522)
* Change deprecated test case->suite

Co-authored-by: Ashton Larkin <ashton@openrobotics.org>
Co-authored-by: G.Doisy <doisyg@users.noreply.github.com>
Co-authored-by: Ian Chen <ichen@osrfoundation.org>
Co-authored-by: Nate Koenig <nate@openrobotics.org>
Co-authored-by: Louise Poubel <louise@openrobotics.org>
Co-authored-by: John Shepherd <john@openrobotics.org>
Co-authored-by: Steve Peters <scpeters@openrobotics.org>
Co-authored-by: Jose Luis Rivero <jrivero@osrfoundation.org>
Co-authored-by: Akash Patel <17132214+acxz@users.noreply.github.com>
Lobotuerk added a commit to Lobotuerk/ign-gazebo that referenced this pull request Jan 11, 2021
* Initial commit

Signed-off-by: Martiño Crespo <marticres@gmail.com>

* Added plugin files

Signed-off-by: Martiño Crespo <marticres@gmail.com>

* Initial .sdf file

Signed-off-by: Martiño Crespo <marticres@gmail.com>

* Pipeline for getting sensor contacts

Signed-off-by: Martiño Crespo <marticres@gmail.com>

* Added visualization of postion and forces

Signed-off-by: Martiño Crespo <marticres@gmail.com>

* Added plugin for Transform Control

Signed-off-by: Martiño Crespo <marticres@gmail.com>

* Added initial interpolation

Signed-off-by: Martiño Crespo <marticres@gmail.com>

* Code check

Signed-off-by: Martiño Crespo <marticres@gmail.com>

* Update info after specified milliseconds

Signed-off-by: Martiño Crespo <marticres@gmail.com>

* Set marker lifetime for better performance

Signed-off-by: Martiño Crespo <marticres@gmail.com>

* Added Depth Camera

Signed-off-by: Martiño Crespo <marticres@gmail.com>

* Added callback and unpacking to Depth Camera messages

Signed-off-by: Martiño Crespo <marticres@gmail.com>

* Compute and visualize normal forces

Signed-off-by: Martiño Crespo <marticres@gmail.com>

* PR Feedback 1

Signed-off-by: Martiño Crespo <marticres@gmail.com>

* Update sdf example to a more realistic environment

Signed-off-by: Martiño Crespo <marticres@gmail.com>

* Allow moving Depth Camera from model origin

Signed-off-by: Martiño Crespo <marticres@gmail.com>

* Make markers dimensions available as parameters

Signed-off-by: Martiño Crespo <marticres@gmail.com>

* Visualize sensor as a marker instead of <visual> and filter out normal forces outside sensor

Signed-off-by: Martiño Crespo <marticres@gmail.com>

* PR Feedback 2

Signed-off-by: Martiño Crespo <marticres@gmail.com>

* PR Feedback 3

Signed-off-by: Martiño Crespo <marticres@gmail.com>

* Minor fixes for sdf, sensor marker and profiler

Signed-off-by: Martiño Crespo <marticres@gmail.com>

* PR Feedback 4

Signed-off-by: Martiño Crespo <marticres@gmail.com>

* fix mac warning

Signed-off-by: Mabel Zhang <mabel@openrobotics.org>

* Helper function to set component data (gazebosim#436)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

* Remove unneeded if statement (gazebosim#432)

Signed-off-by: John Shepherd <john@openrobotics.org>

* Fixes flaky RecordAndPlayback test in INTEGRATION_log_system (gazebosim#463)

The flakiness comes from two sources:
1. Poses recorded by the LogRecorder are published by the
   SceneBroadcaster system throttled at 60 Hz. The throttle mechanism
   uses real-time instead of sim-time which causes a variance in the
   number of recorded poses from run to run. However, the expected number of
   recorded poses was calculated with the assumption that the simulation
   would run with a 1.0 RTF. If the CPU load is high, there could be a
   mismatch between the expected and the actual number of recorded
   poses, which causes the test to fail.
   This can be checked by running the test with
`cpulimit -l 20 -f  bin/INTEGRATION_log_system  -- --gtest_filter="*RecordAndPlayback"`

2. An attempt is made to match up played back poses with the closest
   timestamp in the recorded file. These poses are again published by
   the SceneBroadcaster, so they are subject to the same kind of timing
   issues as the recorded poses.

The solution in this patch for the first issue is to determine the
expected number of recorded poses by counting them in a separate system
that mimics the throttling mechanism of ign-transport. For the second
issue, a testing system is added to the playback server bypassing the
SceSceneBroadcaster altogether.

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

* Make PeerTracker test more robust (gazebosim#452)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

Co-authored-by: Michael Carroll <michael@openrobotics.org>

* Clarify how sim time is interpreted in a System's step (gazebosim#467)

Signed-off-by: Ashton Larkin <ashton@openrobotics.org>

* 3 ➡️  4: fixes for gazebosim#463 (gazebosim#469)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

* Link link tutrial (gazebosim#472)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

* Switch to async state service request (gazebosim#461)

Signed-off-by: Ian Chen <ichen@osrfoundation.org>

* Use a std::promise/std::future to avoid busy waiting the step ack messages in NetworkManagerPrimary (gazebosim#470)

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Louise Poubel <louise@openrobotics.org>

Co-authored-by: Louise Poubel <louise@openrobotics.org>

* Fix tests that use Fuel paths with uppercase letters (gazebosim#480)

As of gazebosim/gz-fuel-tools#130, fuel paths use all lowercase letters. This fixes some tests that had uppercase letters.
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

* 4.1.0 (gazebosim#485)

Signed-off-by: Ashton Larkin <ashton@openrobotics.org>

* Update key event handling (gazebosim#466)

Signed-off-by: John Shepherd <john@openrobotics.org>

* Fix slot in Plotting plugin (gazebosim#490)

Signed-off-by: Alejandro Hernández <ahcorde@gmail.com>

* Tape Measure Plugin (gazebosim#456)

Signed-off-by: John Shepherd <john@openrobotics.org>

* Satisfy make codecheck (gazebosim#491)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

* Move deselect and preview termination to render thread (gazebosim#493)

Signed-off-by: John Shepherd <john@openrobotics.org>

* Fix codecheck (gazebosim#499)

Signed-off-by: Nate Koenig <nate@openrobotics.org>

Co-authored-by: Nate Koenig <nate@openrobotics.org>

* Logical Audio Sensor Plugin (gazebosim#401)

Signed-off-by: Ashton Larkin <ashton@openrobotics.org>

* Codecheck - initialize primitive in LogicalAudio component (gazebosim#502)

Signed-off-by: Ashton Larkin <ashton@openrobotics.org>

* 3 to 4: fix codecheck for ign-gazebo4

Signed-off-by: Ashton Larkin <ashton@openrobotics.org>

* 3 to 4: resolve codecheck warnings

Signed-off-by: Ashton Larkin <ashton@openrobotics.org>

* add frame_id and child_frame_id attribute support for DiffDrive (gazebosim#361)

Add configuration of the odom frame_id and child_frame_id fields from sdf attributes <frame_id> and <child_frame_id>

Signed-off-by: Guillaume <guillaume.doisy@wyca.fr>
Signed-off-by: Guillaume Doisy <guillaume.doisy@wyca.fr>

* Require ign-gui 4.1.0 (gazebosim#505)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

* Fix shadow artifacts by disabling double sided rendering (gazebosim#446)

* read double sided sdf param

Signed-off-by: Ian Chen <ichen@osrfoundation.org>

* update migration

Signed-off-by: Ian Chen <ichen@osrfoundation.org>

* Make the tunnels example world more interesting (gazebosim#462)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

* add double sided msg to sdf conversion

Signed-off-by: Ian Chen <ichen@osrfoundation.org>

Co-authored-by: Louise Poubel <louise@openrobotics.org>

* Add ability to record video based on sim time (gazebosim#414)

* add ability to record video from gui camera using sim time

Signed-off-by: Ian Chen <ichen@osrfoundation.org>

* add msg

Signed-off-by: Ian Chen <ichen@osrfoundation.org>

* use QueryBoolText

Signed-off-by: Ian Chen <ichen@osrfoundation.org>

Co-authored-by: Nate Koenig <nate@openrobotics.org>
Co-authored-by: Louise Poubel <louise@openrobotics.org>

* Add lockstep mode to video recording (gazebosim#419)

Signed-off-by: Ian Chen <ichen@osrfoundation.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>

Co-authored-by: Nate Koenig <nate@openrobotics.org>
Co-authored-by: Louise Poubel <louise@openrobotics.org>

* Kinetic energy monitor plugin (gazebosim#492)

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>
Signed-off-by: Nate Koenig <nate@openrobotics.org>

Co-authored-by: Nate Koenig <nate@openrobotics.org>
Co-authored-by: Louise Poubel <louise@openrobotics.org>

* Disable right click menu when using measuring tool (gazebosim#458)

Signed-off-by: John Shepherd <john@openrobotics.org>

Co-authored-by: Louise Poubel <louise@openrobotics.org>

* Bump to 3.6.0 (gazebosim#524)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

* Don't make docs on macOS (gazebosim#528)

add comment about doxygen bug

Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>

Co-authored-by: Louise Poubel <louise@openrobotics.org>

* Updates to ardupilot migration tutorial (gazebosim#525)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

* Update gtest to 1.10.0 for Windows compilation (ign-gazebo3) (gazebosim#506)

* Compile new gtest with c++11
* Use INSTANTIATE_TEST_SUITE_P instead of deprecated -INSTANTIATE_TEST_CASE_P

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>

* Apply suggestions from code review

Signed-off-by: Michael Carroll <michael@openrobotics.org>

Co-authored-by: Louise Poubel <louise@openrobotics.org>

* change nullptr to a int ptr for qt 5.15.2 bug (gazebosim#527)

See: https://bugreports.qt.io/browse/QTBUG-89114
Signed-off-by: acxz <17132214+acxz@users.noreply.github.com>

Co-authored-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>

* Generate valid topics everywhere (support names with spaces) (gazebosim#522)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

* One more tutorial version bump

Signed-off-by: Michael Carroll <michael@openrobotics.org>

* Fix bad merge

Signed-off-by: Michael Carroll <michael@openrobotics.org>

* change nullptr to a int ptr for qt 5.15.2 bug (gazebosim#527)

See: https://bugreports.qt.io/browse/QTBUG-89114
Signed-off-by: acxz <17132214+acxz@users.noreply.github.com>

Co-authored-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>

* Generate valid topics everywhere (support names with spaces) (gazebosim#522)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

* Change deprecated test case->suite

Signed-off-by: Michael Carroll <michael@openrobotics.org>

* 3 ➡️ 4 (gazebosim#533)

* Clarify how sim time is interpreted in a System's step (gazebosim#467)
* add frame_id and child_frame_id attribute support for DiffDrive (gazebosim#361)
* Add ability to record video based on sim time (gazebosim#414)
* add ability to record video from gui camera using sim time
* add msg
* use QueryBoolText
* Add lockstep mode to video recording (gazebosim#419)
* Disable right click menu when using measuring tool (gazebosim#458)
* Bump to 3.6.0 (gazebosim#524)
* Don't make docs on macOS (gazebosim#528)
* Updates to ardupilot migration tutorial (gazebosim#525)
* Update gtest to 1.10.0 for Windows compilation (ign-gazebo3) (gazebosim#506)
* Compile new gtest with c++11
* Use INSTANTIATE_TEST_SUITE_P instead of deprecated -INSTANTIATE_TEST_CASE_P
* Apply suggestions from code review
* One more tutorial version bump
* Fix bad merge
* change nullptr to a int ptr for qt 5.15.2 bug (gazebosim#527)
* Generate valid topics everywhere (support names with spaces) (gazebosim#522)
* Change deprecated test case->suite

Co-authored-by: Ashton Larkin <ashton@openrobotics.org>
Co-authored-by: G.Doisy <doisyg@users.noreply.github.com>
Co-authored-by: Ian Chen <ichen@osrfoundation.org>
Co-authored-by: Nate Koenig <nate@openrobotics.org>
Co-authored-by: Louise Poubel <louise@openrobotics.org>
Co-authored-by: John Shepherd <john@openrobotics.org>
Co-authored-by: Steve Peters <scpeters@openrobotics.org>
Co-authored-by: Jose Luis Rivero <jrivero@osrfoundation.org>
Co-authored-by: Akash Patel <17132214+acxz@users.noreply.github.com>

* Add support for topic statistics on breadcrumb deployments (gazebosim#532)

* Add support for topic statistics on breadcrumb deployments

Signed-off-by: Nate Koenig <nate@openrobotics.org>

* Require version 9.1 of ignition transport

Signed-off-by: Nate Koenig <nate@openrobotics.org>

* Move to after mutex

Signed-off-by: Nate Koenig <nate@openrobotics.org>

Co-authored-by: Nate Koenig <nate@openrobotics.org>
Co-authored-by: Carlos Agüero <caguero@osrfoundation.org>

Co-authored-by: Martiño Crespo <marticres@gmail.com>
Co-authored-by: Louise Poubel <louise@openrobotics.org>
Co-authored-by: Mabel Zhang <mabel@openrobotics.org>
Co-authored-by: John Shepherd <john@openrobotics.org>
Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>
Co-authored-by: Michael Carroll <michael@openrobotics.org>
Co-authored-by: Ashton Larkin <ashton@openrobotics.org>
Co-authored-by: Ian Chen <ichen@osrfoundation.org>
Co-authored-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Co-authored-by: Nate Koenig <nkoenig@users.noreply.github.com>
Co-authored-by: Nate Koenig <nate@openrobotics.org>
Co-authored-by: G.Doisy <doisyg@users.noreply.github.com>
Co-authored-by: Gonzo <42421541+gonzodepedro@users.noreply.github.com>
Co-authored-by: Steve Peters <scpeters@openrobotics.org>
Co-authored-by: Jose Luis Rivero <jrivero@osrfoundation.org>
Co-authored-by: Akash Patel <17132214+acxz@users.noreply.github.com>
Co-authored-by: Carlos Agüero <caguero@osrfoundation.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📜 blueprint Ignition Blueprint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants