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 segmentation and bounding box sensor types #592

Merged
merged 16 commits into from
Aug 19, 2021

Conversation

AmrElsersy
Copy link
Contributor

@AmrElsersy AmrElsersy commented Jun 13, 2021

🎉 New feature

related to #134 #135

Summary

Added new types for SDF::Sensor to support Segmentation & BoundingBox Sensors

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Code check passed (In source directory, run sh tools/code_check.sh)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review

Copy link
Contributor

@adlarkin adlarkin 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 so far, but we should do a few more things before merging this:

  1. Can you update test/integration/link_dom.cc to test the code path from XML to C++ for the new sensor types? Here's an example you can follow: https://github.com/osrf/sdformat/blob/sdformat11_11.1.0/test/integration/link_dom.cc#L337-L346
  2. Can you update the sensors unit test? You'll need to add the new types here: https://github.com/osrf/sdformat/blob/sdformat11_11.1.0/src/Sensor_TEST.cc#L243-L285
  3. Can you update the sensor documentation to include these new types? You'll want to update sdf/1.9/sensor.sdf: https://github.com/osrf/sdformat/blob/1a779837277034ec45f67cc9f1ea9a47bf4bfd9a/sdf/1.9/sensor.sdf#L11-L32
  4. Can you change the name of the PR to say something like "add segmentation and bounding box sensor types"? The current PR name is generic, and doesn't really give any information about the changes being made.

@chapulina
Copy link
Contributor

I noticed that the bounding box camera implementation in gazebosim/gz-rendering#334 accepts some configuration: visible / full. Do we want users to be able to configure that through SDF? Are there any other configurations we may want to add here for either sensor?

@adlarkin
Copy link
Contributor

I noticed that the bounding box camera implementation in ignitionrobotics/ign-rendering#334 accepts some configuration: visible / full. Do we want users to be able to configure that through SDF? Are there any other configurations we may want to add here for either sensor?

I think it's likely that we will want users to configure visible/full through SDF, and there's a chance that there are other configurations we'd want to add to these sensors as well. I think that the best approach is to finish reviewing/iterating over all of the other bounding box/segmentation PRs before merging this PR - that way, if we find that the SDF configuration needs to be modified at all, we can do so before merging.

@AmrElsersy
Copy link
Contributor Author

AmrElsersy commented Jun 16, 2021

@chapulina @adlarkin
I do these configurations in ign-sensors, by getting them through the SDF
like that: https://github.com/ignitionrobotics/ign-sensors/pull/133/files#diff-3a47a41297adece53d9e25cb71940e30d655d1a491020c1a8fe918a4f1a95ac2R146

std::string type = sdfElement->Get<std::string>("segmentation_type");

is that a bad way to do that ? :D

Signed-off-by: AmrElsersy <[email protected]>
@AmrElsersy AmrElsersy changed the title Add Sensor types Add segmentation and bounding box sensor types Jun 17, 2021
@AmrElsersy AmrElsersy requested a review from adlarkin June 17, 2021 01:31
@adlarkin
Copy link
Contributor

adlarkin commented Jun 17, 2021

I do these configurations in ign-sensors, by getting them through the SDF
like that: https://github.com/ignitionrobotics/ign-sensors/pull/133/files#diff-3a47a41297adece53d9e25cb71940e30d655d1a491020c1a8fe918a4f1a95ac2R146

std::string type = sdfElement->Get<std::string>("segmentation_type");

is that a bad way to do that ? :D

I think what you're doing in ign-sensors is correct, but we want to make sure that we document these configurations in SDF so that users know there are additional/unique parameters that they can configure for the segmentation and bounding box sensors.

So, we may need to add files like segmentation_camera.sdf and boundingbox_camera.sdf (I'm not sure what the naming conventions should be for these files - do you know, @chapulina?) to the sdf/1.9/ directory, and then include these files in sdf/1.9/sensors.sdf. These new *.sdf files would specify the elements that are unique to the segmentation and bounding box sensors. Here's an example with the air pressure sensor:

@chapulina
Copy link
Contributor

I'm not sure what the naming conventions should be for these files - do you know, @chapulina?

Just looking at the existing files, your suggestions look good

@chapulina chapulina added this to Inbox in Core development via automation Jun 28, 2021
@chapulina chapulina moved this from Inbox to In review in Core development Jun 28, 2021
@AmrElsersy
Copy link
Contributor Author

I will add the sdf examples in the ign-sensors repo, as we discussed ashton, so I think this PR should be ready to review.

Copy link
Contributor

@adlarkin adlarkin left a comment

Choose a reason for hiding this comment

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

Can you also address the changes mentioned in #592 (comment)?

test/sdf/sensors.sdf Outdated Show resolved Hide resolved
test/sdf/sensors.sdf Outdated Show resolved Hide resolved
Signed-off-by: AmrElsersy <[email protected]>
@AmrElsersy AmrElsersy requested a review from adlarkin July 19, 2021 11:05
@adlarkin
Copy link
Contributor

Tests in CI are failing, @AmrElsersy can you address the failures?

Signed-off-by: AmrElsersy <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Aug 2, 2021

Codecov Report

Merging #592 (e57f69a) into main (afdf3a5) will increase coverage by 0.03%.
The diff coverage is 100.00%.

❗ Current head e57f69a differs from pull request most recent head c705b80. Consider uploading reports for the commit c705b80 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #592      +/-   ##
==========================================
+ Coverage   87.94%   87.98%   +0.03%     
==========================================
  Files          72       72              
  Lines       10893    10929      +36     
==========================================
+ Hits         9580     9616      +36     
  Misses       1313     1313              
Impacted Files Coverage Δ
src/Camera.cc 83.55% <100.00%> (+1.23%) ⬆️
src/Sensor.cc 81.19% <100.00%> (+0.83%) ⬆️

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 afdf3a5...c705b80. Read the comment docs.

sdf/1.9/bounding_box_camera.sdf Outdated Show resolved Hide resolved
sdf/1.9/bounding_box_camera.sdf Outdated Show resolved Hide resolved
sdf/1.9/bounding_box_camera.sdf Outdated Show resolved Hide resolved
sdf/1.9/bounding_box_camera.sdf Outdated Show resolved Hide resolved
sdf/1.9/bounding_box_camera.sdf Outdated Show resolved Hide resolved
sdf/1.9/segmentation_camera.sdf Outdated Show resolved Hide resolved
sdf/1.9/segmentation_camera.sdf Outdated Show resolved Hide resolved
sdf/1.9/segmentation_camera.sdf Outdated Show resolved Hide resolved
test/integration/link_dom.cc Show resolved Hide resolved
test/integration/link_dom.cc Show resolved Hide resolved
include/sdf/Camera.hh Outdated Show resolved Hide resolved
include/sdf/Camera.hh Outdated Show resolved Hide resolved
include/sdf/Camera.hh Outdated Show resolved Hide resolved
include/sdf/Camera.hh Outdated Show resolved Hide resolved
sdf/1.9/camera.sdf Outdated Show resolved Hide resolved
src/Camera_TEST.cc Outdated Show resolved Hide resolved
src/Camera_TEST.cc Show resolved Hide resolved
test/integration/link_dom.cc Show resolved Hide resolved
test/integration/link_dom.cc Show resolved Hide resolved
src/Camera.cc Outdated Show resolved Hide resolved
Signed-off-by: AmrElsersy <[email protected]>
Signed-off-by: Ashton Larkin <[email protected]>
Copy link
Contributor

@adlarkin adlarkin left a comment

Choose a reason for hiding this comment

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

Nice work!

Copy link
Collaborator

@azeey azeey 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! Just a couple nits.

include/sdf/Camera.hh Outdated Show resolved Hide resolved
include/sdf/Camera.hh Outdated Show resolved Hide resolved
@AmrElsersy AmrElsersy requested a review from azeey August 18, 2021 22:11
@adlarkin adlarkin merged commit c919d23 into gazebosim:main Aug 19, 2021
Core development automation moved this from In review to Done Aug 19, 2021
@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/gsoc-2021-machine-learning-extension-to-ignition-gazebo/1070/1

@j-rivero j-rivero removed this from Done in Core development May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏯 fortress Ignition Fortress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants