Removed pose topic from log system #839

jennuine · 2021-05-27T19:31:00Z

Signed-off-by: Jenn Nguyen jenn@openrobotics.org

🦟 Bug fix

Fixes #753

Summary

With changes from #742 , pose information was being logged twice (in the dynamic_pose/info and changed_state topics). This PR removes the recording dynamic_pose/info topic from LogRecord and updates the integration test.

~~LogPlayback still uses pose messages for backward compatibility (i.e., users have log files before #742 changes) but should probably be removed when ported to Fortress.~~

EDIT: PR also removes pose from LogPlayback

Checklist

Signed all commits for DCO
Added tests
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 <jenn@openrobotics.org>

codecov · 2021-05-27T21:30:04Z

Codecov Report

Merging #839 (a3cd9a6) into main (e3ee0af) will increase coverage by 0.88%.
The diff coverage is n/a.

❗ Current head a3cd9a6 differs from pull request most recent head 4b4bf45. Consider uploading reports for the commit 4b4bf45 to get more accurate results

@@            Coverage Diff             @@
##             main     #839      +/-   ##
==========================================
+ Coverage   64.66%   65.55%   +0.88%     
==========================================
  Files         242      240       -2     
  Lines       19014    17793    -1221     
==========================================
- Hits        12296    11664     -632     
+ Misses       6718     6129     -589

Impacted Files	Coverage Δ
src/systems/log/LogPlayback.cc	`60.56% <ø> (ø)`
src/systems/log/LogRecord.cc	`80.64% <ø> (-0.24%)`	⬇️
src/ServerPrivate.cc	`83.79% <0.00%> (-3.36%)`	⬇️
src/gui/plugins/entity_tree/EntityTree.cc	`9.83% <0.00%> (-3.28%)`	⬇️
.../gui/plugins/transform_control/TransformControl.cc	`15.92% <0.00%> (-3.22%)`	⬇️
src/systems/scene_broadcaster/SceneBroadcaster.cc	`93.75% <0.00%> (-3.13%)`	⬇️
include/ignition/gazebo/components/Factory.hh	`96.05% <0.00%> (-2.64%)`	⬇️
src/Conversions.cc	`81.70% <0.00%> (-2.05%)`	⬇️
src/systems/joint_controller/JointController.cc	`77.17% <0.00%> (-1.40%)`	⬇️
...ems/collada_world_exporter/ColladaWorldExporter.cc	`94.17% <0.00%> (-1.33%)`	⬇️
... and 70 more

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 efa7d44...4b4bf45. Read the comment docs.

j-rivero

Good work of simplifying things.

With changes from #742 , pose information was being logged twice (in the dynamic_pose/info and changed_state topics). This PR removes the recording dynamic_pose/info topic from LogRecord and updates the integration test.

Could we please get a Changelog and Migration entry for this effect?

Going to merge the PR with ign-gazebo4 to get some fresh CI.

src/systems/log/LogPlayback.cc

Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>

jennuine · 2021-06-08T22:54:36Z

Could we please get a Changelog and Migration entry for this effect?

How's this a3438b6?

j-rivero · 2021-06-09T15:01:53Z

How's this a3438b6?

I think is fine, thanks @jennuine . If I understand correctly, anyone using the old topic should just change to changed_state. Is this correct?

j-rivero

Probably unrelated to this PR, but if I run make INTEGRATION_log_system , the result is:

gn-gazebo/build on  jennuine/remove_pose_record ❯ ./bin/INTEGRATION_log_system 
Running main() from /home/jrivero/code/ignition/ign-gazebo/test/gtest/src/gtest_main.cc
[==========] Running 12 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 12 tests from LogSystemTest
[ RUN      ] LogSystemTest.LogPlaybackStatistics
[Msg] Loading default world.
...
[Dbg] [EntityComponentManager.cc:688] Using components of type [4981278897826323946] / [ign_gazebo_components.WorldSdf].
[Msg] Loaded level [3]
[Msg] No systems loaded from SDF, loading defaults
[Err] [ServerConfig.cc:890] Failed to copy installed config [/usr/local/share/ignition/ignition-gazebo4/playback_server.config] to default config [/home/jrivero/.ignition/gazebo/playback_server.config].(file /usr/local/share/ignition/ignition-gazebo4/playback_server.config doesn't exist)
[Err] [SystemLoader.cc:66] Failed to load system plugin [ignition-gazebo-log-system] : couldn't find shared library.

Is this a known problem?

test/integration/log_system.cc

jennuine · 2021-06-09T16:26:43Z

If I understand correctly, anyone using the old topic should just change to changed_state. Is this correct?

Correct.

Probably unrelated to this PR, but if I run make INTEGRATION_log_system , the result is ... Is this a known problem?

The failure is coming from LogSystemTest.LogPlaybackStatistics which this PR doesn't affect (does not change playback only changes record) and is the first time I've seen this error. Also, that test is the first test in log_system.cc which leads me to think maybe there's something different with your setup?

mjcarroll · 2021-06-10T14:11:22Z

Probably unrelated to this PR, but if I run make INTEGRATION_log_system , the result is ... Is this a known problem?

There doesn't seem to be any history of it in the ign-gazebo4 builds that I saw.

jennuine · 2021-06-21T20:12:22Z

Hold off on merging until get a confirm from @nkoenig

nkoenig · 2021-07-01T16:26:46Z

Please don't merge this until after July 8th.

chapulina · 2021-07-01T19:46:22Z

I'm not sure about changing the contents of the logs in a stable release, downstream users may be counting on this topic. Can this be targeted at Fortress?

jennuine · 2021-07-01T20:55:56Z

Can this be targeted at Fortress?

I believe so, what do you think @nkoenig?

jennuine · 2021-07-13T23:55:47Z

Retargeting to Fortress and will be removing pose from LogPlayback

Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>

jennuine · 2021-07-15T00:01:20Z

The new recorded state.tlog file is from rolling_shapes.sdf but with only the sphere and ground plane models so that the log file would be relatively small.

Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>

jennuine · 2021-07-15T01:04:41Z

c7c943c Left a comment to not re-record state.tlog file with -r flag and to pause sim before terminating. Previously, I had done this and it resulted in strange pose behavior. The log file was 5.7s long and in the LogSystemTest.LogControl test, after seeking to 5s (close to the end but not exactly), x = -74 then after rewinding and calling server.Run for 2 iterations x = -75. If it was 3 iterations then the pose would be the expected initial pose x=0. When I re-recorded the log without -r, started/unpaused the sim manually, and made sure the program was paused before terminating everything worked as expected.

j-rivero · 2021-08-10T20:01:51Z

All changes looks good to me. I've merged the latest state of the branch and restart the Windows build to get fresh CI. Seems to be a flaky test in github actions.

If Windows build ends nicely, consider this approved. Nice work Jenn.

jennuine · 2021-08-11T01:03:10Z

If Windows build ends nicely, consider this approved

Windows didn't end nicely :( but looking through other merged PRs similar tests failed for windows as well (including LogSystemTest)

j-rivero · 2021-08-11T15:21:47Z

Windows didn't end nicely :( but looking through other merged PRs similar tests failed for windows as well (including LogSystemTest)

Oh, "nicely" in the context of Windows meant "compilation is ok" since we did not fix the failing tests yet.

removed pose topic from LogRecord

9eb8b82

Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>

jennuine requested a review from chapulina as a code owner May 27, 2021 19:31

github-actions bot added the 🔮 dome Ignition Dome label May 27, 2021

added todo for fortress

712e3a2

Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>

jennuine self-assigned this May 27, 2021

iche033 requested a review from j-rivero June 7, 2021 18:37

j-rivero reviewed Jun 7, 2021

View reviewed changes

src/systems/log/LogPlayback.cc Outdated Show resolved Hide resolved

j-rivero and others added 2 commits June 7, 2021 14:51

Merge branch 'ign-gazebo4' into jennuine/remove_pose_record

24ee56a

updated migration

a3438b6

Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>

jennuine requested a review from j-rivero June 8, 2021 22:54

j-rivero reviewed Jun 9, 2021

View reviewed changes

test/integration/log_system.cc Show resolved Hide resolved

test/integration/log_system.cc Show resolved Hide resolved

Merge branch 'ign-gazebo4' into jennuine/remove_pose_record

e763af0

nkoenig self-requested a review June 14, 2021 18:04

nkoenig approved these changes Jun 18, 2021

View reviewed changes

Merge branch 'ign-gazebo4' into jennuine/remove_pose_record

b81c0e3

jennuine marked this pull request as draft July 13, 2021 23:52

jennuine changed the base branch from ign-gazebo4 to main July 13, 2021 23:56

jennuine removed the 🔮 dome Ignition Dome label Jul 13, 2021

jennuine added the 🏯 fortress Ignition Fortress label Jul 13, 2021

jennuine added 3 commits July 13, 2021 16:57

Merge branch 'main' into jennuine/remove_pose_record

e7c104c

Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>

removed pose topic from LogPlayback

ea67972

Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>

updated migration

5effd40

Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>

jennuine changed the title ~~Removed pose topic from LogRecord~~ Removed pose topic from log system Jul 14, 2021

jennuine marked this pull request as ready for review July 15, 2021 00:01

comment about re-recording state.tlog

c7c943c

Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>

chapulina requested a review from mjcarroll August 2, 2021 18:22

chapulina requested a review from j-rivero August 9, 2021 18:58

Merge branch 'main' into jennuine/remove_pose_record

3174c77

Merge branch 'main' into jennuine/remove_pose_record

4b4bf45

j-rivero approved these changes Aug 11, 2021

View reviewed changes

jennuine merged commit 8f5103b into main Aug 11, 2021

jennuine deleted the jennuine/remove_pose_record branch August 11, 2021 15:31

jennuine mentioned this pull request Aug 25, 2021

Pose being recorded twice in LogRecord #753

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed pose topic from log system #839

Removed pose topic from log system #839

jennuine commented May 27, 2021 •

edited

Loading

codecov bot commented May 27, 2021 •

edited

Loading

j-rivero left a comment

jennuine commented Jun 8, 2021

j-rivero commented Jun 9, 2021

j-rivero left a comment

jennuine commented Jun 9, 2021

mjcarroll commented Jun 10, 2021

jennuine commented Jun 21, 2021

nkoenig commented Jul 1, 2021

chapulina commented Jul 1, 2021

jennuine commented Jul 1, 2021

jennuine commented Jul 13, 2021

jennuine commented Jul 15, 2021

jennuine commented Jul 15, 2021

j-rivero commented Aug 10, 2021

jennuine commented Aug 11, 2021

j-rivero commented Aug 11, 2021

Removed pose topic from log system #839

Removed pose topic from log system #839

Conversation

jennuine commented May 27, 2021 • edited Loading

🦟 Bug fix

Summary

Checklist

codecov bot commented May 27, 2021 • edited Loading

Codecov Report

j-rivero left a comment

Choose a reason for hiding this comment

jennuine commented Jun 8, 2021

j-rivero commented Jun 9, 2021

j-rivero left a comment

Choose a reason for hiding this comment

jennuine commented Jun 9, 2021

mjcarroll commented Jun 10, 2021

jennuine commented Jun 21, 2021

nkoenig commented Jul 1, 2021

chapulina commented Jul 1, 2021

jennuine commented Jul 1, 2021

jennuine commented Jul 13, 2021

jennuine commented Jul 15, 2021

jennuine commented Jul 15, 2021

j-rivero commented Aug 10, 2021

jennuine commented Aug 11, 2021

j-rivero commented Aug 11, 2021

jennuine commented May 27, 2021 •

edited

Loading

codecov bot commented May 27, 2021 •

edited

Loading