Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for specifying topics to record #315

Merged
merged 9 commits into from
Sep 5, 2020

Conversation

nkoenig
Copy link
Contributor

@nkoenig nkoenig commented Aug 26, 2020

A user can specify topics to record from the command line, or programmatically through the ServerConfig.

Use ign gazebo -h to see information about the --record-topic option including example usage.

The ServerConfig functions will be used by ign-launch.

Default logging behavior is persevered if no topics are specified.

Signed-off-by: Nate Koenig nate@openrobotics.org

Signed-off-by: Nate Koenig <nate@openrobotics.org>
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest adding a test to test/integration/log_system.cc passing the --record-topic option through the command line.

@@ -301,6 +301,17 @@ void ServerPrivate::AddRecordPlugin(const ServerConfig &_config)
cPathElem->Set<std::string>(cmpPath);
}

// If record topics specified, add in SDF
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No action needed on this PR, but just for future reference, we're trying to pass this kind of information to systems through components rather than the SDF now.

Nate Koenig added 4 commits August 31, 2020 08:26
Signed-off-by: Nate Koenig <nate@openrobotics.org>
Signed-off-by: Nate Koenig <nate@openrobotics.org>
Signed-off-by: Nate Koenig <nate@openrobotics.org>
Signed-off-by: Nate Koenig <nate@openrobotics.org>
@nkoenig
Copy link
Contributor Author

nkoenig commented Aug 31, 2020

I'd suggest adding a test to test/integration/log_system.cc passing the --record-topic option through the command line.

Added a test in bd76749

@codecov
Copy link

codecov bot commented Aug 31, 2020

Codecov Report

Merging #315 into ign-gazebo2 will increase coverage by 0.01%.
The diff coverage is 82.60%.

Impacted file tree graph

@@               Coverage Diff               @@
##           ign-gazebo2     #315      +/-   ##
===============================================
+ Coverage        78.10%   78.11%   +0.01%     
===============================================
  Files              183      183              
  Lines             9979    10022      +43     
===============================================
+ Hits              7794     7829      +35     
- Misses            2185     2193       +8     
Impacted Files Coverage Δ
include/ignition/gazebo/ServerConfig.hh 100.00% <ø> (ø)
src/ServerPrivate.cc 88.20% <57.14%> (-2.03%) ⬇️
src/systems/log/LogRecord.cc 82.35% <88.88%> (+0.45%) ⬆️
src/ServerConfig.cc 95.45% <100.00%> (+0.19%) ⬆️
src/ign.cc 68.91% <100.00%> (+0.86%) ⬆️

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 8bc99bf...46d71b6. Read the comment docs.

@nkoenig nkoenig requested a review from chapulina September 1, 2020 15:22
@iche033
Copy link
Contributor

iche033 commented Sep 2, 2020

Looking at the code, my understanding of the behavior is that if a topic is specified, the other topics necessary to playback a simulation in ign gazebo will not be recorded? So for example:

ign gazebo -r -v 4  --record-topic /clock --record-path "test_log"  log_record_resources.sdf

when playing back the state.tlog file, I see a blank scene in the UI which could be due to missing pose and scene data needed by the client. On the other hand, this simulation playback fine when recording using the cmd below since it records all default topics.

ign gazebo -r -v 4  --record-topic --record-path "test_log"  log_record_resources.sdf

Just want to confirm that's the intended behavior? If so I think it would be useful to offer the ability to record all default topics + the ones specified through --record-topic but that can be addressed later.

Signed-off-by: Nate Koenig <nate@openrobotics.org>
@nkoenig
Copy link
Contributor Author

nkoenig commented Sep 3, 2020

Looking at the code, my understanding of the behavior is that if a topic is specified, the other topics necessary to playback a simulation in ign gazebo will not be recorded? So for example:

ign gazebo -r -v 4  --record-topic /clock --record-path "test_log"  log_record_resources.sdf

when playing back the state.tlog file, I see a blank scene in the UI which could be due to missing pose and scene data needed by the client. On the other hand, this simulation playback fine when recording using the cmd below since it records all default topics.

ign gazebo -r -v 4  --record-topic --record-path "test_log"  log_record_resources.sdf

Just want to confirm that's the intended behavior? If so I think it would be useful to offer the ability to record all default topics + the ones specified through --record-topic but that can be addressed later.

I changed the behavior to be an add in 990c877.

…#333)

Signed-off-by: Nate Koenig <nate@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>

Co-authored-by: Nate Koenig <nate@openrobotics.org>
Co-authored-by: Louise Poubel <louise@openrobotics.org>
Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me!

@chapulina chapulina merged commit 5c151bf into ign-gazebo2 Sep 5, 2020
@chapulina chapulina deleted the log_record_topics branch September 5, 2020 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📜 blueprint Ignition Blueprint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants