Limit scene broadcast publications when paused #497
Conversation
Signed-off-by: Nate Koenig <[email protected]>
|
I haven't tried this yet, but have you checked that manipulating models in the GUI while paused still works? |
Codecov Report
@@ Coverage Diff @@
## ign-gazebo3 #497 +/- ##
===============================================
+ Coverage 77.74% 77.78% +0.04%
===============================================
Files 210 209 -1
Lines 11628 11641 +13
===============================================
+ Hits 9040 9055 +15
+ Misses 2588 2586 -2
Continue to review full report at Codecov.
|
|
I was able to move the models through the GUI when paused. On the other hand, setting the model pose through |
Signed-off-by: Nate Koenig <[email protected]>
|
The I've add a |
It looks like that broke some things. The problem of checking for changes on pre-update is that systems are adding their changes during that call, and we can't guarantee the order in which systems run. Try for example moving the |
There was a problem hiding this comment.
the component inspector fails to show any data sometimes.
This is related to the original change, not the pre-update one.
The GUI runner doesn't run its own loop, it just reacts to changes from the server. So all systems' Update functions are only called when there's a new state coming from the server. If we don't publish while paused, systems that rely on periodic Updates don't work properly. For example, selecting a new entity won't update the ComponentInspector completely until the next Update call.
I think this could be solved by changing the way GUI systems' update calls are triggered. Maybe we can add a loop to the GuiRunner so that it calls updates periodically even if there aren't new state messages from the server. Unfortunately, the GuiRunner isn't PIMPLized, so it will be tricky to change a lot in Citadel. Maybe we can target a change like that to Edifice.
|
Before I go any further with this PR, I want to make sure the general direction is aligned with our goals. @chapulina , I not opposed to declining this if you think there is a different or better way to improve performance while paused. |
|
Not publishing periodic changes while paused seems reasonable to me. We just need to adjust the GUI's expectations to match that. There may also be other downstream consumers of the state message who would be affected. I worry about such a big change in a released version, so I would suggest we target this at Edifice. |
…uiRunner that will periodically update the GUI plugins Signed-off-by: Nate Koenig <[email protected]>
I think I've addressed the problems in 17bb8f1 |
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
| // \todo(anyone) Pimplize GuiRunner and move these global variables to the | ||
| // private class. | ||
| gRunning = true; | ||
| gUpdateThread = std::thread([&]() |
There was a problem hiding this comment.
This will increase the number of updates while unpaused. I don't think it may cause immediate issues though, just something to keep an eye on.
…caster_paused Suggestion to #497
Signed-off-by: Louise Poubel <[email protected]>
I don't think the scenebroadcaster needs to send frequent messages when paused.
Signed-off-by: Nate Koenig [email protected]