ECM's ChangedState gets message with modified components #742

jennuine · 2021-04-06T19:53:21Z

🎉 New feature

Summary

This PR update's ECM's ChangedState functions to include added, removed, or modified components in the serialized state message.

Previously, ECM's ChangedState functions returned a message with the serialized state of entities that were changing (new or removed) in the current iteration but not when components were modified (new, removed, or modified).

Test it

A new test was added to UNIT_EntityComponentManager_TEST, to run all tests:

./build/ignition-gazebo4/bin/UNIT_EntityComponentManager_TEST

To run only the new test (for 1st iteration):

./build/ignition-gazebo4/bin/UNIT_EntityComponentManager_TEST --gtest_filter=EntityComponentManagerRepeat/EntityComponentManagerFixture.ChangedStateComponents/0

Also, INTEGRATION_log_system was updated to check for pose components in the state message from the /world/log_pendulum/changed_state topic:

./build/ignition-gazebo4/bin/INTEGRATION_log_system --gtest_filter=LogSystemTest.RecordAndPlayback

Checklist

Signed all commits for DCO
Added tests
Added example and/or tutorial
Updated documentation (as needed)
Updated migration guide (as needed)
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

Signed-off-by: Jenn Nguyen <[email protected]>

iche033 · 2021-04-07T00:31:55Z

src/EntityComponentManager.cc

@@ -1257,6 +1282,7 @@ void EntityComponentManager::SetState(
        // values.
        // igndbg << *comp << "  " << *newComp.get() << std::endl;
        // *comp = *newComp.get();
+        this->dataPtr->AddModifiedComponent(entity);


is this call needed here? Given that the above msg says internal error, I expect that something is wrong is it gets to this else block. Looking at the code, the component value is just being updated by removing the old component and creating a new one?

It's not needed currently but I put the call there in case the above TODO is addressed. Should I comment it out with a note to add it in when the TODO is addressed and if SetChanged is not called?

I see, yeah you can comment it out and add a note here

Done 70c4320

iche033 · 2021-04-07T00:34:32Z

src/EntityComponentManager.cc

@@ -1367,6 +1393,7 @@ void EntityComponentManager::SetState(
          }
        }
        this->SetChanged(entity, compIter.first, flag);
+        this->dataPtr->AddModifiedComponent(entity);


I think the SetChanged call above will also call AddModifiedComponent

Oops, thanks 70c4320

iche033 · 2021-04-07T00:39:54Z

src/EntityComponentManager.cc

@@ -199,6 +211,7 @@ void EntityComponentManager::ClearNewlyCreatedEntities()
 {
  std::lock_guard<std::mutex> lock(this->dataPtr->entityCreatedMutex);
  this->dataPtr->newlyCreatedEntities.clear();
+  this->dataPtr->modifiedComponents.clear();


should this call be in ClearNewlyCreatedEntities or maybe it fits better in SetAllComponentsUnchanged?

Moved to SetAllComponentsUnchanged 70c4320

iche033 · 2021-04-07T01:14:05Z

I wonder if we can safely remove this call to record the dynamic_pose/info topic now that the state msg also captures the pose changes - mainly so that we don't record the pose twice in the log file.

Signed-off-by: Jenn Nguyen <[email protected]>

codecov · 2021-04-08T00:28:25Z

Codecov Report

Merging #742 (7fd5cb7) into ign-gazebo4 (8829840) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@               Coverage Diff               @@
##           ign-gazebo4     #742      +/-   ##
===============================================
- Coverage        65.51%   65.50%   -0.01%     
===============================================
  Files              237      237              
  Lines            17249    17264      +15     
===============================================
+ Hits             11300    11309       +9     
- Misses            5949     5955       +6

Impacted Files	Coverage Δ
include/ignition/gazebo/EntityComponentManager.hh	`100.00% <ø> (ø)`
src/EntityComponentManager.cc	`86.38% <100.00%> (+0.35%)`	⬆️
src/gui/plugins/entity_tree/EntityTree.cc	`9.83% <0.00%> (-3.28%)`	⬇️

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 8829840...7fd5cb7. Read the comment docs.

jennuine · 2021-04-08T18:04:41Z

Here is a plot of a test I did running a subt simulation before this PR and another after. In both tests, I manually drove the robot to the same location. The state.tlog file size for the sim test before the PR is 6.5 MB and after is 15 MB.

In the legend, where it says before/after is the raw data and where it says smooth is just a Gaussian filter applied to the before/after data to make it easier to visualize. Although it's hard to get a real representation since I'm running the sim for only ~1.5 mins and I'm not averaging multiple runs, it's just to give a general idea.

I wonder if we can safely remove this call to record the dynamic_pose/info topic now that the state msg also captures the pose changes - mainly so that we don't record the pose twice in the log file.

I did a quick test by commenting out the call, recording data, then playing it back and everything seems to be working properly. Should this be addressed in another PR or this one? I believe there will need to be some clean up needed after removing the call (e.g., in test and possibly in LogPlayback)

iche033 · 2021-04-08T18:17:20Z

I did a quick test by commenting out the call, recording data, then playing it back and everything seems to be working properly. Should this be addressed in another PR or this one?

yeah let's do that in a separate PR. I think it'll require thorough testing to make sure we don't break anything.

iche033 · 2021-04-09T00:36:20Z

looks like there is a related test failure on homebrew. The INTEGRATION_log_system test failed with a segfault. Not sure if it's flaky

jennuine · 2021-04-09T17:40:17Z

looks like there is a related test failure on homebrew. The INTEGRATION_log_system test failed with a segfault. Not sure if it's flaky

I think this may be a flaky test, I just re-ran homebrew CI and the test passed. It's hard to pinpoint exactly where since I can't reproduce this but I believe the seg fault happened somewhere towards the end of this function
https://github.com/ignitionrobotics/ign-gazebo/blob/8829840d05c45db1599bb0fca7ef9c4914b807af/test/integration/log_system.cc#L1038

because this error message printed 100 times: [Err] [Physics.cc:1726] Internal error: link [29] not in entity map

iche033 · 2021-04-09T19:15:52Z

I do see a lot of the [Err] [Physics.cc:1726] Internal error: link [29] not in entity map msgs when running the integration_log_system test, which seems to be happening on ign-rendering4 branch as well (cc @chapulina)

But I can't reproduce the test failure.

* 🎈 3.8.0 (#688) Signed-off-by: Louise Poubel <[email protected]> * Make it so joint state publisher is quieter (#696) Signed-off-by: Michael Carroll <[email protected]> * [BULLET] Making GetContactsFromLastStepFeature optional in Collision Features (#690) * GetContactsFromLastStepFeature made optional Signed-off-by: Tomas Lorente <[email protected]> Co-authored-by: Addisu Z. Taddese <[email protected]> * Add test for thermal object temperatures below 0 kelvin (#621) Signed-off-by: Ashton Larkin <[email protected]> * Scenebroadcaster sensors (#698) * Add sensors to scene broadcaster Signed-off-by: Nate Koenig <[email protected]> * Update src/systems/scene_broadcaster/SceneBroadcaster.cc Co-authored-by: Michael Carroll <[email protected]> * Fix codecheck Signed-off-by: Nate Koenig <[email protected]> Co-authored-by: Nate Koenig <[email protected]> Co-authored-by: Michael Carroll <[email protected]> * Fix diffuse and ambient values for ackermann example (#707) Signed-off-by: Ammaar Solkar <[email protected]> * 🎈 5.0.0 (#731) Signed-off-by: Louise Poubel <[email protected]> * Support configuring particle scatter ratio in particle emitter system (#674) * set particle scatter ratio through sdf Signed-off-by: Ian Chen <[email protected]> * address feedback Signed-off-by: Ian Chen <[email protected]> * add todo note about merging forward Signed-off-by: Ian Chen <[email protected]> Co-authored-by: Ashton Larkin <[email protected]> * Update PlaybackScrubber description (#733) Signed-off-by: Ammaar Solkar <[email protected]> * Iterate through changed links only in UpdateSim (#678) Signed-off-by: Ashton Larkin <[email protected]> * Do not pass -Wno-unused-parameter to MSVC compiler (#716) Signed-off-by: Silvio Traversaro <[email protected]> * Use Protobuf_IMPORT_DIRS instead of PROTOBUF_IMPORT_DIRS for compatibility with Protobuf CMake config (#715) Signed-off-by: Silvio Traversaro <[email protected]> * Fix component inspector shutdown crash (#724) Signed-off-by: Louise Poubel <[email protected]> Co-authored-by: Alejandro Hernández Cordero <[email protected]> * Validate step size and RTF parameters (#740) Only set them if they are strictly positive. Signed-off-by: Luca Della Vedova <[email protected]> * Fix compute_rtfs arguments (#737) Signed-off-by: Caio Amaral <[email protected]> * Fixed collision visual bounding boxes (#746) Signed-off-by: Jenn Nguyen <[email protected]> * Fix CMakelists.txt merge Signed-off-by: Nate Koenig <[email protected]> * ECM's ChangedState gets message with modified components (#742) * ecm's ChangedState to contain modified components Signed-off-by: Jenn Nguyen <[email protected]> * updated log_system test Signed-off-by: Jenn Nguyen <[email protected]> * removed unnecessary calls Signed-off-by: Jenn Nguyen <[email protected]> Co-authored-by: Ian Chen <[email protected]> * fixed particle emitter forward playback (#745) Signed-off-by: Jenn Nguyen <[email protected]> * Merge pull request #730 from ignitionrobotics/particle_emitter Particle emitter based on SDF * 4 7 0 prep (#755) * Prepare for 4.7.0 Signed-off-by: Nate Koenig <[email protected]> * Added placeholder Signed-off-by: Nate Koenig <[email protected]> Co-authored-by: Nate Koenig <[email protected]> * Fix 'invalid animation update data' msg for actors (#754) Signed-off-by: Ashton Larkin <[email protected]> * Update benchmark comparison instructions (#766) (#766) Signed-off-by: Ashton Larkin <[email protected]> * [DiffDrive] add enable/disable (#772) * add enable/disable diffdrive Signed-off-by: Guillaume Doisy <[email protected]> * remove debug Signed-off-by: Guillaume Doisy <[email protected]> * do not subscribe to enable if topic is empty Signed-off-by: Guillaume Doisy <[email protected]> * add test Signed-off-by: Guillaume Doisy <[email protected]> * lint and style Signed-off-by: Guillaume Doisy <[email protected]> * change enable type to bool and renamed to enabled Signed-off-by: Guillaume Doisy <[email protected]> * Add odometry publisher system (#547) * Create Initial Odometry Publisher system plugin Add code for initial plugin that gets position from Pose component and calculates velocities based on rolling mean from displacement data. Signed-off-by: Maganty Rushyendra <[email protected]> * Remove Linear and Angular Velocity components Also renames frames in Odometry msg to include model name, and makes various style changes. Signed-off-by: Maganty Rushyendra <[email protected]> * Get World pose instead of pose of robot base frame Signed-off-by: Maganty Rushyendra <[email protected]> * Add documentation for variables and functions Includes minor stylistic changes. Signed-off-by: Maganty Rushyendra <[email protected]> * Check for valid odomTopic and update copyright year Signed-off-by: Maganty Rushyendra <[email protected]> * Add tests for OdometryPublisherSystem and fix velocity calculation bug Swap X and Y linear velocities when calculating odometry velocities relative to robotBaseFrame. Signed-off-by: Maganty Rushyendra <[email protected]> Co-authored-by: ahcorde <[email protected]> * Patch particle emitter2 service (#777) * Patch particle emitter2 service Signed-off-by: Nate Koenig <[email protected]> * Remove condition variable Signed-off-by: Nate Koenig <[email protected]> * Set emitter frame and relative pose Signed-off-by: Nate Koenig <[email protected]> Co-authored-by: Nate Koenig <[email protected]> * Preparing for 4.8.0 release (#780) Signed-off-by: Nate Koenig <[email protected]> Co-authored-by: Nate Koenig <[email protected]> * 👩‍🌾 Enable Focal CI (#646) Signed-off-by: Louise Poubel <[email protected]> Co-authored-by: Michael Carroll <[email protected]> * [TPE] Support setting individual link velocity (#427) Signed-off-by: claireyywang <[email protected]> Signed-off-by: Ian Chen <[email protected]> Co-authored-by: Ian Chen <[email protected]> Co-authored-by: Louise Poubel <[email protected]> * Don't store duplicate ComponentTypeId in ECM (#751) Signed-off-by: Louise Poubel <[email protected]> * Feature/hydrodynamics (#749) Implement hydrodynamics and thruster plugin. Signed-off-by: Arjo Chakravarty <[email protected]> Co-authored-by: Mabel Zhang <[email protected]> Co-authored-by: Carlos Agüero <[email protected]> * Fix macOS build: components::Name in benchmark (#784) Signed-off-by: Louise Poubel <[email protected]> Co-authored-by: Steve Peters <[email protected]> * Fix ColladaExporter submesh index bug (#763) Signed-off-by: Jorge Perez <[email protected]> * 👩‍🌾 Fix Windows build and some warnings (#782) Signed-off-by: Louise Poubel <[email protected]> Co-authored-by: Alejandro Hernández Cordero <[email protected]> * Prevent crash on Plotting plugin with mutex (#747) Signed-off-by: Louise Poubel <[email protected]> * Bump ign-physics version to 3.2 (#792) Signed-off-by: Louise Poubel <[email protected]> * Bump to ign-msgs 7.1 / sdformat 11.1, Windows fixes (#758) Signed-off-by: Louise Poubel <[email protected]> * Util: Use public API from libsdformat for detecting non-file source (#794) Signed-off-by: Eric Cousineau <[email protected]> * Fix included nested model expansion in SDF generation (#768) * fixed included nested model expansion Signed-off-by: Jenn Nguyen <[email protected]> * added resource path to test Signed-off-by: Jenn Nguyen <[email protected]> * use orig URIs & support multi level nesting Signed-off-by: Jenn Nguyen <[email protected]> * save fuel version when enabled Signed-off-by: Jenn Nguyen <[email protected]> * retrieve uri from map Signed-off-by: Jenn Nguyen <[email protected]> * copy included element Signed-off-by: Jenn Nguyen <[email protected]> * clear attributes before copying include element Signed-off-by: Jenn Nguyen <[email protected]> * Map canonical links to their models (#736) Signed-off-by: Ashton Larkin <[email protected]> * ColladaExporter, export submesh selected (#802) * Export only submesh if selected * Add test case for the PR * Attempting a unified solution Signed-off-by: Jorge Perez <[email protected]> Co-authored-by: Nate Koenig <[email protected]> Co-authored-by: Michael Carroll <[email protected]> Co-authored-by: Jose Tomas Lorente <[email protected]> Co-authored-by: Addisu Z. Taddese <[email protected]> Co-authored-by: Ashton Larkin <[email protected]> Co-authored-by: Nate Koenig <[email protected]> Co-authored-by: Nate Koenig <[email protected]> Co-authored-by: Ammaar Solkar <[email protected]> Co-authored-by: Ian Chen <[email protected]> Co-authored-by: Ashton Larkin <[email protected]> Co-authored-by: Silvio Traversaro <[email protected]> Co-authored-by: Alejandro Hernández Cordero <[email protected]> Co-authored-by: Luca Della Vedova <[email protected]> Co-authored-by: Caio Amaral <[email protected]> Co-authored-by: Jenn Nguyen <[email protected]> Co-authored-by: G.Doisy <[email protected]> Co-authored-by: Rushyendra Maganty <[email protected]> Co-authored-by: Claire Wang <[email protected]> Co-authored-by: Arjo Chakravarty <[email protected]> Co-authored-by: Mabel Zhang <[email protected]> Co-authored-by: Carlos Agüero <[email protected]> Co-authored-by: Steve Peters <[email protected]> Co-authored-by: Jorge Perez <[email protected]> Co-authored-by: Eric Cousineau <[email protected]> Co-authored-by: Jorge Perez <[email protected]>

jennuine added 3 commits April 1, 2021 16:53

ecm's ChangedState to contain modified components

0d10268

Signed-off-by: Jenn Nguyen <[email protected]>

updated log_system test

e6ffee0

Signed-off-by: Jenn Nguyen <[email protected]>

Merge branch 'ign-gazebo4' into jennuine/ecm_changedstate

b415c33

Signed-off-by: Jenn Nguyen <[email protected]>

jennuine requested a review from iche033 April 6, 2021 19:53

jennuine requested a review from chapulina as a code owner April 6, 2021 19:53

github-actions bot added the 🔮 dome Ignition Dome label Apr 6, 2021

jennuine mentioned this pull request Apr 6, 2021

Fix log playback for particle emitters #745

Merged

7 tasks

iche033 reviewed Apr 7, 2021

View reviewed changes

jennuine added 2 commits April 7, 2021 16:28

removed unnecessary calls

70c4320

Signed-off-by: Jenn Nguyen <[email protected]>

Merge branch 'ign-gazebo4' into jennuine/ecm_changedstate

a79e0cf

Merge branch 'ign-gazebo4' into jennuine/ecm_changedstate

7fd5cb7

iche033 approved these changes Apr 9, 2021

View reviewed changes

jennuine merged commit 9b24362 into ign-gazebo4 Apr 9, 2021

jennuine deleted the jennuine/ecm_changedstate branch April 9, 2021 19:22

jennuine mentioned this pull request Apr 9, 2021

Pose being recorded twice in LogRecord #753

Closed

jennuine mentioned this pull request May 27, 2021

Removed pose topic from log system #839

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ECM's ChangedState gets message with modified components #742

ECM's ChangedState gets message with modified components #742

jennuine commented Apr 6, 2021 •

edited

Loading

iche033 Apr 7, 2021

jennuine Apr 7, 2021

iche033 Apr 7, 2021

jennuine Apr 7, 2021

iche033 Apr 7, 2021

jennuine Apr 7, 2021

iche033 Apr 7, 2021

jennuine Apr 7, 2021

iche033 commented Apr 7, 2021

codecov bot commented Apr 8, 2021 •

edited

Loading

jennuine commented Apr 8, 2021

iche033 commented Apr 8, 2021

iche033 commented Apr 9, 2021 •

edited

Loading

jennuine commented Apr 9, 2021

iche033 commented Apr 9, 2021

ECM's ChangedState gets message with modified components #742

ECM's ChangedState gets message with modified components #742

Conversation

jennuine commented Apr 6, 2021 • edited Loading

🎉 New feature

Summary

Test it

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iche033 commented Apr 7, 2021

codecov bot commented Apr 8, 2021 • edited Loading

Codecov Report

jennuine commented Apr 8, 2021

iche033 commented Apr 8, 2021

iche033 commented Apr 9, 2021 • edited Loading

jennuine commented Apr 9, 2021

iche033 commented Apr 9, 2021

jennuine commented Apr 6, 2021 •

edited

Loading

codecov bot commented Apr 8, 2021 •

edited

Loading

iche033 commented Apr 9, 2021 •

edited

Loading