Remove filtering from realtime factor (RTF) calculation #1942
Conversation
Signed-off-by: Addisu Z. Taddese <[email protected]>
src/SimulationRunner.cc
Outdated
| this->realTimeFactor = math::precision( | ||
| static_cast<double>(simAvg.count()) / realAvg.count(), 4); | ||
| // Store the real time and sim time only if not paused. | ||
| constexpr double kAlpha = 0.999; |
There was a problem hiding this comment.
Just wondering if this would too aggressive? Sometimes spikes are important for development / debugging purposes. On the other hand, for debugging we could also just compute the per-iteration RTF values if needed using simTime / realTime so I don't have a strong opinion on this. The smooth RTF is great when trying to figure out the overall sim performance.
There was a problem hiding this comment.
I found when debugging, it's helpful to actually look at a histogram, I have used this one in the past: https://github.com/mjcarroll/ign_imgui
Maybe we should move this into a more main-line place if others would find it useful.
There was a problem hiding this comment.
Maybe we create a new filtered_stats topic that outputs this and the current stats topic can just be the instantaneous RTF (Δ simTime/Δ realTime). We can then use the filtered_stats for the GUI and the stats topic for development/debugging. For reference, gazebo-classic had an infinite window and computed RTF = total simTime/total realTime so it didn't fluctuate as much: https://github.com/gazebosim/gazebo-classic/blob/d3584008b47c745c1732a6cd07af68726cd900fa/gazebo/util/Diagnostics.cc#L104-L123
There was a problem hiding this comment.
Maybe we create a new filtered_stats topic that outputs this and the current stats topic can just be the instantaneous RTF (Δ simTime/Δ realTime).
We could also just compute the filtered stats / RTF on the GUI end instead of on the server? I haven't paid attention to the diagnostics much in classic. As for the RTF displayed in the GUI in gazebo-classic, it's computed on the GUI side and uses a size of 20. It subscribes to the ~/world_stats topic which is published at 100Hz (real time). So assuming sim is running at the same speed on real time, that's roughly 2 second window?
There was a problem hiding this comment.
Ah, my mistake. I assumed the RTF calculation in the diagnostics was what was displayed in the GUI of gazebo-classic. So I guess the big difference comes from the fact that it's published at 100Hz. With a size of 20, I calculate a 200ms second window, but in gz-sim, assuming we're running at 1000Hz, the window is 20ms.
My main concern was the GUI, so I can put this logic there. We can leave the RTF calculation in SimulationRunner intact so we don't change behavior for now, and change it to instantaneous RTF in Harmonic. Does that sound okay?
There was a problem hiding this comment.
ah yes 200ms window ha 😓
We can leave the RTF calculation in SimulationRunner intact so we don't change behavior for now, and change it to instantaneous RTF in Harmonic. Does that sound okay?
sounds good to me!
There was a problem hiding this comment.
Done in 40e7163 and follow-up PR in gazebosim/gz-gui#529
There was a problem hiding this comment.
I found when debugging, it's helpful to actually look at a histogram, I have used this one in the past: https://github.com/mjcarroll/ign_imgui
Maybe we should move this into a more main-line place if others would find it useful.
It could be added as an example in gz-sim. Better yet, it could be a feature of the "Topic Plotting" GUI plugin 😉.
Signed-off-by: Addisu Z. Taddese <[email protected]>
azeey
changed the title
Codecov Report
@@ Coverage Diff @@
## gz-sim7 #1942 +/- ##
===========================================
- Coverage 65.08% 65.03% -0.05%
===========================================
Files 352 352
Lines 28312 28292 -20
===========================================
- Hits 18428 18401 -27
- Misses 9884 9891 +7
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
🦟 Bug fix
Summary
The current RTF calculation uses a moving window of 20 iterations for smoothing. This apparently is not enough and the RTF displayed in the GUI fluctuates a lot. The fluctuation also gives an inflated sense of RTF when used with sensors that don't need to update every time step compared to the RTF obtained by simply doing
totalSimTime/totalRealTime.The approach in this PR essentially increases the moving window, but uses low pass filtered simTime and realTime as delayed versions of the same. The delayed times are then subtracted from the corresponding current time to get a larger window of time with which to compute the RTF.
The result is best demonstrated in this plot. Here, I'm running
examples/worlds/camera_sensor.sdfand plotting the RTF. After 30 seconds, I remove the image subscriber to show the change in RTF.It's interesting that even after the subscriber is removed, the RTF doesn't go to 1.0. #1938 might help, but we might also be sleeping too much in the update loop as shown in the figure below of running
empty.sdf.Hopefully, we can figure out why down the road.
Checklist
codecheckpassed (See contributing)Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-bymessages.🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸