-
Notifications
You must be signed in to change notification settings - Fork 290
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
Logical audio sensor plugin #401
Conversation
Codecov Report
@@ Coverage Diff @@
## ign-gazebo3 #401 +/- ##
===============================================
+ Coverage 77.32% 77.54% +0.22%
===============================================
Files 205 208 +3
Lines 11055 11375 +320
===============================================
+ Hits 8548 8821 +273
- Misses 2507 2554 +47
Continue to review full report at Codecov.
|
b59c8dc
to
94b3914
Compare
@iche033 @nkoenig This PR is now ready for initial review. I tried to give a detailed overview and example usage in the PR description. If anything is still unclear, please let me know. Once I address your initial feedback, there are a few more things I'd like to do before merging this:
|
@iche033 @nkoenig I actually just realized that the current implementation will not work if we have sources/microphones attached to different models, because this would result in separate |
c664114
to
67eec9d
Compare
@nkoenig @iche033 @chapulina @mjcarroll this PR should be ready for actual review this time. I put up-to-date instructions in the PR description, and also updated the example SDF file to allow for moving sensors. Let me know if you have any questions! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are a few lingering questions that I have after my initial draft.
{ | ||
// Inline bracket to help doxygen filtering. | ||
inline namespace IGNITION_GAZEBO_VERSION_NAMESPACE { | ||
namespace logical_audio |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the logical_audio
namespace okay here? Or should it be a sub-namspace of components
?
{ | ||
// Inline bracket to help doxygen filtering. | ||
inline namespace IGNITION_GAZEBO_VERSION_NAMESPACE { | ||
namespace logical_audio |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the logical_audio
namespace okay here? Or should it be a sub-namspace of systems
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we're grouping a bunch of functions specific to logical audio, I think maybe we don't want to pollute the systems
namespace so I'm fine with either sticking with logical_audio
namespace or having it as a sub-namespace of systems
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I'll leave the logical_audio
namespace as is for now. If someone else points out a reason to change it, then I can update it accordingly.
src/systems/logical_audio_sensor_plugin/LogicalAudioSensorPlugin.cc
Outdated
Show resolved
Hide resolved
{ | ||
// Inline bracket to help doxygen filtering. | ||
inline namespace IGNITION_GAZEBO_VERSION_NAMESPACE { | ||
namespace logical_audio |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we're grouping a bunch of functions specific to logical audio, I think maybe we don't want to pollute the systems
namespace so I'm fine with either sticking with logical_audio
namespace or having it as a sub-namespace of systems
src/systems/logical_audio_sensor_plugin/LogicalAudioSensorPlugin.cc
Outdated
Show resolved
Hide resolved
Would it be possible to move the logical audio source and sink to the |
src/systems/logical_audio_sensor_plugin/LogicalAudioSensorPlugin.cc
Outdated
Show resolved
Hide resolved
src/systems/logical_audio_sensor_plugin/LogicalAudioSensorPlugin.cc
Outdated
Show resolved
Hide resolved
src/systems/logical_audio_sensor_plugin/LogicalAudioSensorPlugin.cc
Outdated
Show resolved
Hide resolved
I think this could be possible, but if you take a look at the original brainstorming conversation that motivated this PR, I decided to make this an ignition gazebo system plugin so that we wouldn't have to make any changes to the SDF spec. I think another reason for not placing this in Should I look into what would be involved in moving this over to Edit: There also seem to be other sensors as system plugins, like the logical camera, altimeter, and magnetometer. |
f35c133
to
7fc0213
Compare
src/systems/logical_audio_sensor_plugin/LogicalAudioSensorPlugin.hh
Outdated
Show resolved
Hide resolved
src/systems/logical_audio_sensor_plugin/LogicalAudioSensorPlugin.hh
Outdated
Show resolved
Hide resolved
src/systems/logical_audio_sensor_plugin/LogicalAudioSensorPlugin.hh
Outdated
Show resolved
Hide resolved
src/systems/logical_audio_sensor_plugin/LogicalAudioSensorPlugin.hh
Outdated
Show resolved
Hide resolved
src/systems/logical_audio_sensor_plugin/LogicalAudioSensorPlugin.hh
Outdated
Show resolved
Hide resolved
src/systems/logical_audio_sensor_plugin/LogicalAudioSensorPlugin.hh
Outdated
Show resolved
Hide resolved
src/systems/logical_audio_sensor_plugin/LogicalAudioSensorPlugin.hh
Outdated
Show resolved
Hide resolved
src/systems/logical_audio_sensor_plugin/LogicalAudioSensorPlugin.hh
Outdated
Show resolved
Hide resolved
7fc79b7
to
bf5cacb
Compare
@nkoenig @iche033 this is ready for another round of review. Here are a few notes about what has been added/modified since the last round of reviews:
|
Signed-off-by: Ashton Larkin <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the design of the parameters looks good to me. Nice tutorial. I left a few comments.
163df3d
to
c8432cd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few minor comments. Looks good!
Signed-off-by: Ashton Larkin <[email protected]>
53f2a1a
to
a023e01
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good. Just a couple small comments
Signed-off-by: Ashton Larkin <[email protected]>
4aa354e
to
670a517
Compare
Signed-off-by: Ashton Larkin <[email protected]>
767d07e
to
1a9f5d0
Compare
* Initial commit Signed-off-by: Martiño Crespo <[email protected]> * Added plugin files Signed-off-by: Martiño Crespo <[email protected]> * Initial .sdf file Signed-off-by: Martiño Crespo <[email protected]> * Pipeline for getting sensor contacts Signed-off-by: Martiño Crespo <[email protected]> * Added visualization of postion and forces Signed-off-by: Martiño Crespo <[email protected]> * Added plugin for Transform Control Signed-off-by: Martiño Crespo <[email protected]> * Added initial interpolation Signed-off-by: Martiño Crespo <[email protected]> * Code check Signed-off-by: Martiño Crespo <[email protected]> * Update info after specified milliseconds Signed-off-by: Martiño Crespo <[email protected]> * Set marker lifetime for better performance Signed-off-by: Martiño Crespo <[email protected]> * Added Depth Camera Signed-off-by: Martiño Crespo <[email protected]> * Added callback and unpacking to Depth Camera messages Signed-off-by: Martiño Crespo <[email protected]> * Compute and visualize normal forces Signed-off-by: Martiño Crespo <[email protected]> * PR Feedback 1 Signed-off-by: Martiño Crespo <[email protected]> * Update sdf example to a more realistic environment Signed-off-by: Martiño Crespo <[email protected]> * Allow moving Depth Camera from model origin Signed-off-by: Martiño Crespo <[email protected]> * Make markers dimensions available as parameters Signed-off-by: Martiño Crespo <[email protected]> * Visualize sensor as a marker instead of <visual> and filter out normal forces outside sensor Signed-off-by: Martiño Crespo <[email protected]> * PR Feedback 2 Signed-off-by: Martiño Crespo <[email protected]> * PR Feedback 3 Signed-off-by: Martiño Crespo <[email protected]> * Minor fixes for sdf, sensor marker and profiler Signed-off-by: Martiño Crespo <[email protected]> * PR Feedback 4 Signed-off-by: Martiño Crespo <[email protected]> * fix mac warning Signed-off-by: Mabel Zhang <[email protected]> * Helper function to set component data (gazebosim#436) Signed-off-by: Louise Poubel <[email protected]> * Remove unneeded if statement (gazebosim#432) Signed-off-by: John Shepherd <[email protected]> * Fixes flaky RecordAndPlayback test in INTEGRATION_log_system (gazebosim#463) The flakiness comes from two sources: 1. Poses recorded by the LogRecorder are published by the SceneBroadcaster system throttled at 60 Hz. The throttle mechanism uses real-time instead of sim-time which causes a variance in the number of recorded poses from run to run. However, the expected number of recorded poses was calculated with the assumption that the simulation would run with a 1.0 RTF. If the CPU load is high, there could be a mismatch between the expected and the actual number of recorded poses, which causes the test to fail. This can be checked by running the test with `cpulimit -l 20 -f bin/INTEGRATION_log_system -- --gtest_filter="*RecordAndPlayback"` 2. An attempt is made to match up played back poses with the closest timestamp in the recorded file. These poses are again published by the SceneBroadcaster, so they are subject to the same kind of timing issues as the recorded poses. The solution in this patch for the first issue is to determine the expected number of recorded poses by counting them in a separate system that mimics the throttling mechanism of ign-transport. For the second issue, a testing system is added to the playback server bypassing the SceSceneBroadcaster altogether. Signed-off-by: Addisu Z. Taddese <[email protected]> * Make PeerTracker test more robust (gazebosim#452) Signed-off-by: Louise Poubel <[email protected]> Co-authored-by: Michael Carroll <[email protected]> * Clarify how sim time is interpreted in a System's step (gazebosim#467) Signed-off-by: Ashton Larkin <[email protected]> * 3 ➡️ 4: fixes for gazebosim#463 (gazebosim#469) Signed-off-by: Louise Poubel <[email protected]> * Link link tutrial (gazebosim#472) Signed-off-by: Louise Poubel <[email protected]> * Switch to async state service request (gazebosim#461) Signed-off-by: Ian Chen <[email protected]> * Use a std::promise/std::future to avoid busy waiting the step ack messages in NetworkManagerPrimary (gazebosim#470) Signed-off-by: Ivan Santiago Paunovic <[email protected]> Signed-off-by: Louise Poubel <[email protected]> Co-authored-by: Louise Poubel <[email protected]> * Fix tests that use Fuel paths with uppercase letters (gazebosim#480) As of gazebosim/gz-fuel-tools#130, fuel paths use all lowercase letters. This fixes some tests that had uppercase letters. Signed-off-by: Addisu Z. Taddese <[email protected]> * 4.1.0 (gazebosim#485) Signed-off-by: Ashton Larkin <[email protected]> * Update key event handling (gazebosim#466) Signed-off-by: John Shepherd <[email protected]> * Fix slot in Plotting plugin (gazebosim#490) Signed-off-by: Alejandro Hernández <[email protected]> * Tape Measure Plugin (gazebosim#456) Signed-off-by: John Shepherd <[email protected]> * Satisfy make codecheck (gazebosim#491) Signed-off-by: Louise Poubel <[email protected]> * Move deselect and preview termination to render thread (gazebosim#493) Signed-off-by: John Shepherd <[email protected]> * Fix codecheck (gazebosim#499) Signed-off-by: Nate Koenig <[email protected]> Co-authored-by: Nate Koenig <[email protected]> * Logical Audio Sensor Plugin (gazebosim#401) Signed-off-by: Ashton Larkin <[email protected]> * Codecheck - initialize primitive in LogicalAudio component (gazebosim#502) Signed-off-by: Ashton Larkin <[email protected]> * 3 to 4: fix codecheck for ign-gazebo4 Signed-off-by: Ashton Larkin <[email protected]> * 3 to 4: resolve codecheck warnings Signed-off-by: Ashton Larkin <[email protected]> * add frame_id and child_frame_id attribute support for DiffDrive (gazebosim#361) Add configuration of the odom frame_id and child_frame_id fields from sdf attributes <frame_id> and <child_frame_id> Signed-off-by: Guillaume <[email protected]> Signed-off-by: Guillaume Doisy <[email protected]> * Require ign-gui 4.1.0 (gazebosim#505) Signed-off-by: Louise Poubel <[email protected]> * Fix shadow artifacts by disabling double sided rendering (gazebosim#446) * read double sided sdf param Signed-off-by: Ian Chen <[email protected]> * update migration Signed-off-by: Ian Chen <[email protected]> * Make the tunnels example world more interesting (gazebosim#462) Signed-off-by: Louise Poubel <[email protected]> * add double sided msg to sdf conversion Signed-off-by: Ian Chen <[email protected]> Co-authored-by: Louise Poubel <[email protected]> * Add ability to record video based on sim time (gazebosim#414) * add ability to record video from gui camera using sim time Signed-off-by: Ian Chen <[email protected]> * add msg Signed-off-by: Ian Chen <[email protected]> * use QueryBoolText Signed-off-by: Ian Chen <[email protected]> Co-authored-by: Nate Koenig <[email protected]> Co-authored-by: Louise Poubel <[email protected]> * Add lockstep mode to video recording (gazebosim#419) Signed-off-by: Ian Chen <[email protected]> Signed-off-by: Louise Poubel <[email protected]> Co-authored-by: Nate Koenig <[email protected]> Co-authored-by: Louise Poubel <[email protected]> * Kinetic energy monitor plugin (gazebosim#492) Signed-off-by: Gonzalo de Pedro <[email protected]> Signed-off-by: Nate Koenig <[email protected]> Co-authored-by: Nate Koenig <[email protected]> Co-authored-by: Louise Poubel <[email protected]> * Disable right click menu when using measuring tool (gazebosim#458) Signed-off-by: John Shepherd <[email protected]> Co-authored-by: Louise Poubel <[email protected]> * Bump to 3.6.0 (gazebosim#524) Signed-off-by: Louise Poubel <[email protected]> * Don't make docs on macOS (gazebosim#528) add comment about doxygen bug Signed-off-by: Louise Poubel <[email protected]> Signed-off-by: Steve Peters <[email protected]> Co-authored-by: Louise Poubel <[email protected]> * Updates to ardupilot migration tutorial (gazebosim#525) Signed-off-by: Louise Poubel <[email protected]> * Update gtest to 1.10.0 for Windows compilation (ign-gazebo3) (gazebosim#506) * Compile new gtest with c++11 * Use INSTANTIATE_TEST_SUITE_P instead of deprecated -INSTANTIATE_TEST_CASE_P Signed-off-by: Jose Luis Rivero <[email protected]> * Apply suggestions from code review Signed-off-by: Michael Carroll <[email protected]> Co-authored-by: Louise Poubel <[email protected]> * change nullptr to a int ptr for qt 5.15.2 bug (gazebosim#527) See: https://bugreports.qt.io/browse/QTBUG-89114 Signed-off-by: acxz <[email protected]> Co-authored-by: Louise Poubel <[email protected]> Signed-off-by: Louise Poubel <[email protected]> * Generate valid topics everywhere (support names with spaces) (gazebosim#522) Signed-off-by: Louise Poubel <[email protected]> * One more tutorial version bump Signed-off-by: Michael Carroll <[email protected]> * Fix bad merge Signed-off-by: Michael Carroll <[email protected]> * change nullptr to a int ptr for qt 5.15.2 bug (gazebosim#527) See: https://bugreports.qt.io/browse/QTBUG-89114 Signed-off-by: acxz <[email protected]> Co-authored-by: Louise Poubel <[email protected]> Signed-off-by: Louise Poubel <[email protected]> * Generate valid topics everywhere (support names with spaces) (gazebosim#522) Signed-off-by: Louise Poubel <[email protected]> * Change deprecated test case->suite Signed-off-by: Michael Carroll <[email protected]> * 3 ➡️ 4 (gazebosim#533) * Clarify how sim time is interpreted in a System's step (gazebosim#467) * add frame_id and child_frame_id attribute support for DiffDrive (gazebosim#361) * Add ability to record video based on sim time (gazebosim#414) * add ability to record video from gui camera using sim time * add msg * use QueryBoolText * Add lockstep mode to video recording (gazebosim#419) * Disable right click menu when using measuring tool (gazebosim#458) * Bump to 3.6.0 (gazebosim#524) * Don't make docs on macOS (gazebosim#528) * Updates to ardupilot migration tutorial (gazebosim#525) * Update gtest to 1.10.0 for Windows compilation (ign-gazebo3) (gazebosim#506) * Compile new gtest with c++11 * Use INSTANTIATE_TEST_SUITE_P instead of deprecated -INSTANTIATE_TEST_CASE_P * Apply suggestions from code review * One more tutorial version bump * Fix bad merge * change nullptr to a int ptr for qt 5.15.2 bug (gazebosim#527) * Generate valid topics everywhere (support names with spaces) (gazebosim#522) * Change deprecated test case->suite Co-authored-by: Ashton Larkin <[email protected]> Co-authored-by: G.Doisy <[email protected]> Co-authored-by: Ian Chen <[email protected]> Co-authored-by: Nate Koenig <[email protected]> Co-authored-by: Louise Poubel <[email protected]> Co-authored-by: John Shepherd <[email protected]> Co-authored-by: Steve Peters <[email protected]> Co-authored-by: Jose Luis Rivero <[email protected]> Co-authored-by: Akash Patel <[email protected]> * Add support for topic statistics on breadcrumb deployments (gazebosim#532) * Add support for topic statistics on breadcrumb deployments Signed-off-by: Nate Koenig <[email protected]> * Require version 9.1 of ignition transport Signed-off-by: Nate Koenig <[email protected]> * Move to after mutex Signed-off-by: Nate Koenig <[email protected]> Co-authored-by: Nate Koenig <[email protected]> Co-authored-by: Carlos Agüero <[email protected]> Co-authored-by: Martiño Crespo <[email protected]> Co-authored-by: Louise Poubel <[email protected]> Co-authored-by: Mabel Zhang <[email protected]> Co-authored-by: John Shepherd <[email protected]> Co-authored-by: Addisu Z. Taddese <[email protected]> Co-authored-by: Michael Carroll <[email protected]> Co-authored-by: Ashton Larkin <[email protected]> Co-authored-by: Ian Chen <[email protected]> Co-authored-by: Ivan Santiago Paunovic <[email protected]> Co-authored-by: Alejandro Hernández Cordero <[email protected]> Co-authored-by: Nate Koenig <[email protected]> Co-authored-by: Nate Koenig <[email protected]> Co-authored-by: G.Doisy <[email protected]> Co-authored-by: Gonzo <[email protected]> Co-authored-by: Steve Peters <[email protected]> Co-authored-by: Jose Luis Rivero <[email protected]> Co-authored-by: Akash Patel <[email protected]> Co-authored-by: Carlos Agüero <[email protected]>
Here's my design/approach for audio sensors in
ign-gazebo3
(ignition citadel). For more information about the approach being taken, take a look at this discussion, and in particular, this comment.The idea is to create
source
andmicrophone
objects, with the following key characteristics:volumelevel
: The volume level emitted from the source (ranges from 0.0 to 1.0). The volume will decrease as the distance from the source increases. The way the volume decreases is based on theattenuationshape
andattenuationfunction
of the source (currently, the only values for these parameters aresphere
andlinear
, respectively).playduration
: How long the audio source plays for (in simulation seconds). 0 means infinite play time.volumethreshold
: The minimum volume level the microphone can detect audio at (ranges from 0.0 to 1.0).At the end of each simulation step, microphones check to see if they have detected audio emitted from any of the audio sources that are currently playing. Audio sources can be started/stopped through the SDF file and/or through service calls. If a microphone can detect a source, a message is published to the microphone's "detection topic". This message contains the ID of the source that was detected, along with the volume level that was heard at the microphone's location.
Take a look at the tutorial to learn how to use/test this plugin.
TODO:
Signed-off-by: Ashton Larkin [email protected]