Publish all periodic change components in Scene Broadcaster

[luca-della-vedova]

As reported in #529, the only way users have to have components published regularly is to specify them as OneTimeChanged, but this incurs in the large overhead of re-serializing and publishing the whole simulation state.

When looking at the code I noticed that a component being marked as PeriodicChange didn't seem to have any effect on the SceneBroadcaster since it only published Pose components at regular intervals.
Also, I ran a quick grep in the code:

luca@luca-focal:~/ws_dome/src/ign-gazebo$ grep -r . -e PeriodicChange
./include/ignition/gazebo/Types.hh:      PeriodicChange = 1,
./src/EntityComponentManager_TEST.cc:  manager.SetChanged(e1, c1.first, ComponentState::PeriodicChange);
./src/EntityComponentManager_TEST.cc:  EXPECT_EQ(ComponentState::PeriodicChange,
./src/EntityComponentManager.cc:    result = ComponentState::PeriodicChange;
./src/EntityComponentManager.cc:  if (_c == ComponentState::PeriodicChange)
./src/systems/wheel_slip/WheelSlip.cc:                      ComponentState::PeriodicChange);
./src/systems/physics/Physics.cc:                ComponentState::PeriodicChange);
./src/systems/physics/Physics.cc:                ComponentState::PeriodicChange);
./src/systems/physics/Physics.cc:                ComponentState::PeriodicChange :
./src/systems/physics/Physics.cc:                  ComponentState::PeriodicChange :
./src/systems/physics/Physics.cc:                ComponentState::PeriodicChange :
./src/systems/physics/Physics.cc:                ComponentState::PeriodicChange :
./src/systems/physics/Physics.cc:                ComponentState::PeriodicChange :
./src/systems/physics/Physics.cc:                ComponentState::PeriodicChange :
./src/systems/physics/Physics.cc:                ComponentState::PeriodicChange :
./src/systems/physics/Physics.cc:                ComponentState::PeriodicChange :
./src/systems/physics/Physics.cc:                ComponentState::PeriodicChange :
./src/systems/multicopter_control/MulticopterVelocityControl.cc:                     ? ComponentState::PeriodicChange
./src/systems/log/LogPlayback.cc:        ComponentState::PeriodicChange);

I noticed that the vast majority of PeriodicChange components are indeed Pose components in Physics (unless users create Linear / Angular Velocity / Acceleration components), with a few exceptions in MulticopterVelocityControl (Actuator component) and WheelSlip (SlipComplianceCmd component).

With this in mind, I decided to just change the periodic publishing of SceneBroadcaster from only Pose components to all the components that have been marked as PeriodicChange.

From a performance point of view, this should have no impact in a default simulation, since entities by default only have a Pose component. The overhead will appear when the user marks any component as PeriodicChange, loads a MulticopterVelocityControl or WheelSlip system or creates a LinearVelocity, AngularVelocity, LinearAcceleration, AngularAcceleration or their World variant components, since any of those will also be added at the 60Hz SceneBroadcaster publisher.

[codecov]

Codecov Report

Merging #544 (a49facd) into ign-gazebo3 (8998740) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@               Coverage Diff               @@
##           ign-gazebo3     #544      +/-   ##
===============================================
+ Coverage        77.70%   77.72%   +0.01%     
===============================================
  Files              208      208              
  Lines            11574    11580       +6     
===============================================
+ Hits              8994     9000       +6     
  Misses            2580     2580              

Impacted Files	Coverage Δ
include/ignition/gazebo/EntityComponentManager.hh	100.00% <ø> (ø)
src/EntityComponentManager.cc	83.61% <100.00%> (+0.15%)	⬆️
src/systems/scene_broadcaster/SceneBroadcaster.cc	97.86% <100.00%> (+<0.01%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8998740...a49facd. Read the comment docs.

[ahcorde]

Something went wrong with the EntityComponentManagerPerfrormance.Each test:

  [ RUN      ] EntityComponentManagerPerfrormance.Each
  /github/workspace/test/performance/each.cc:128: Failure
  Expected: (cacheEntityAvg) < (cachelessEntityAvg), actual: 6346.98 vs 4991.98
  Matching Entity Count =		1
  Nonmatching Entity Count =	1
  Each Iterations =		100
  Cache total =			634698 ns
  Cache avg per iter =		6346.9799999999996 ns
  Cache avg per iter*entity =	6346.9799999999996 ns
  Cacheless total =		499198 ns
  Cacheless avg per iter=		4991.9799999999996 ns
  Cacheless avg per iter*entity=	4991.9799999999996 ns
  
  [  FAILED  ] EntityComponentManagerPerfrormance.Each (41120 ms)

[iche033]

hmm I've seen that test fail before but I don't know if it's due to the same issue. We can try rerunning to see if it's a flaky test.

[chapulina]

The idea looks good to me, I think it's always good to make the logic not specific to some component type.

My only concern is the granularity. If just one component of a given type was periodically changed, all other components of that type will also be sent. I can't think of a situation like that right now, so I think this can be addressed if / when the need arises.

[luca-della-vedova]

The function name is a bit misleading, because it's returning the types of the components, not the components.

Done!

The idea looks good to me, I think it's always good to make the logic not specific to some component type.

My only concern is the granularity. If just one component of a given type was periodically changed, all other components of that type will also be sent. I can't think of a situation like that right now, so I think this can be addressed if / when the need arises.

I agree with the granularity concern. From what I understand the SceneBroadcaster system can be somewhat of a bottleneck so I can have a look at this and see if the implementation would be OK or if it gets too complicated, from my current understanding I think it would require:

• Changing the EntityComponentManager::State(...) function (or more likely add a new one) to only serialize components if they are attached to the entity that flagged them as PeriodicChange.
• The information is already in the periodicChangedComponents set since its key is a pair of ComponentTypeId and ComponentId. It would probably need to be changed to a [unordered_]map<[unordered_]set> though, so we can easily find for each component type, if an entity set it.
• Make sure every downstream system correctly uses the SetChanged API and doesn't assume that just because a ComponentTypeId has been flagged for a single entity all the Components of that type will be published.

From what I can see the Physics system already sets PeriodicChange to its components, there might be some minor tunings needed in the way it processes WorldPoseCmd components but the rest should be OK.

Anyway all of this would be work for a performance improvement followup PR. I'll try to benchmark the possible performance gain before looking too much into it.

My last question for this PR is whether you think I should add an integration test in scene_broadcaster for this feature, something like "create a system that flags a component as periodic change, wait for the publishing, make sure the component is there". It might need a bit of work to implement but it sounds like it might be good to have as well.

[chapulina]

I should add an integration test

That would be great, but I wouldn't block on it if it turns out to be too convoluted.

I'd ask however that we at least add some unit tests for ComponentTypesWithPeriodicChanges

[luca-della-vedova]

I'd ask however that we at least add some unit tests for ComponentTypesWithPeriodicChanges

Added the unit tests 931d58a. I just added them to the SetChanged unit test since it had the setup logic I needed for component creation / deletion.

[iche033]

Make sure every downstream system correctly uses the SetChanged API and doesn't assume that just because a ComponentTypeId has been flagged for a single entity all the Components of that type will be published.

Just want to check my understanding of the current behavior. If 2 entities (e.g. e1, and e2) both have a Pose component and we only call SetChanged on e1's Pose component with PeriodicChange, I should expect that only e1's Pose component would be serialized to msg when State is called right?

Update

ok I just verified that it's indeed the current behavior by expanding the SetChanged test:

diff --git a/src/EntityComponentManager_TEST.cc b/src/EntityComponentManager_TEST.cc
index f2a88169..0ef35872 100644
--- a/src/EntityComponentManager_TEST.cc
+++ b/src/EntityComponentManager_TEST.cc
@@ -2102,6 +2102,24 @@ TEST_P(EntityComponentManagerFixture, SetChanged)
 
   // Mark as changed
   manager.SetChanged(e1, c1.first, ComponentState::PeriodicChange);
+
+  // check that only e1 c1 is serialized into a message
+  msgs::SerializedStateMap stateMsg;
+  manager.State(stateMsg);
+  {
+    ASSERT_EQ(1, stateMsg.entities_size());
+
+    auto iter = stateMsg.entities().find(e1);
+    const auto &e1Msg = iter->second;
+    EXPECT_EQ(e1, e1Msg.id());
+    ASSERT_EQ(1, e1Msg.components_size());
+
+    auto compIter = e1Msg.components().begin();
+    const auto &e1c1Msg = compIter->second;
+    EXPECT_EQ(IntComponent::typeId, e1c1Msg.type());
+    EXPECT_EQ(123, std::stoi(e1c1Msg.component()));
+  }
+
   manager.SetChanged(e2, c2.first, ComponentState::OneTimeChange);
 
   EXPECT_TRUE(manager.HasOneTimeComponentChanges());

[luca-della-vedova]

Make sure every downstream system correctly uses the SetChanged API and doesn't assume that just because a ComponentTypeId has been flagged for a single entity all the Components of that type will be published.

Just want to check my understanding of the current behavior. If 2 entities (e.g. e1, and e2) both have a Pose component and we only call SetChanged on e1's Pose component with PeriodicChange, I should expect that only e1's Pose component would be serialized to msg when State is called right?

True, there is a check to skip publishing the component if another entity flagged it, I got confused with the behavior of OneTimeChange that forces serialization of the whole state.
Anyway I added your test in fcd662b