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

Segmentation Sensor #133

Merged
merged 29 commits into from
Sep 22, 2021
Merged

Segmentation Sensor #133

merged 29 commits into from
Sep 22, 2021

Conversation

AmrElsersy
Copy link
Contributor

@AmrElsersy AmrElsersy commented Jun 13, 2021

🎉 New feature

related to #134

Summary

Segmentation Camera Sensor that allow users to use it easily in SDF, and It publishes the sensor data image to ign-transport

Depends on

SDF #592
Rendering #329

Demo

Semantic

seamantic

Instance

instance

Test it

Will add a tutorial later.

Format

        <sensor name="segmentation_camera" type="segmentation">
          <camera>
            <horizontal_fov>1.047</horizontal_fov>
            <image>
              <width>320</width>
              <height>240</height>
            </image>
            <clip>
              <near>0.1</near>
              <far>100</far>
            </clip>
          </camera>
          <always_on>1</always_on>
          <update_rate>30</update_rate>
          <visualize>true</visualize>
          <topic>camera</topic>
          <colored>true</colored>
          <segmentation_type>semantic</segmentation_type>
        </sensor>

It requires visuals annotating in the sdf via Gazeb #853

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

Signed-off-by: AmrElsersy <amrelsersay@gmail.com>
Signed-off-by: AmrElsersy <amrelsersay@gmail.com>
Signed-off-by: AmrElsersy <amrelsersay@gmail.com>
@AmrElsersy AmrElsersy requested a review from iche033 as a code owner June 13, 2021 13:38
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Jun 13, 2021
@AmrElsersy AmrElsersy changed the title Segmentation Segmentation Sensor Jun 13, 2021
include/ignition/sensors/SegmentationCameraSensor.hh Outdated Show resolved Hide resolved
include/ignition/sensors/SegmentationCameraSensor.hh Outdated Show resolved Hide resolved
include/ignition/sensors/SegmentationCameraSensor.hh Outdated Show resolved Hide resolved
src/SegmentationCameraSensor.cc Outdated Show resolved Hide resolved
src/SegmentationCameraSensor.cc Outdated Show resolved Hide resolved
src/SegmentationCameraSensor.cc Outdated Show resolved Hide resolved
src/SegmentationCameraSensor.cc Outdated Show resolved Hide resolved
test/integration/segmentation_camera_plugin.cc Outdated Show resolved Hide resolved
test/integration/segmentation_camera_plugin.cc Outdated Show resolved Hide resolved
test/integration/segmentation_camera_plugin.cc Outdated Show resolved Hide resolved
Signed-off-by: AmrElsersy <amrelsersay@gmail.com>
@AmrElsersy AmrElsersy requested a review from ahcorde June 15, 2021 00:29
include/ignition/sensors/SegmentationCameraSensor.hh Outdated Show resolved Hide resolved
src/SegmentationCameraSensor.cc Outdated Show resolved Hide resolved
src/SegmentationCameraSensor.cc Show resolved Hide resolved
test/integration/segmentation_camera_plugin.cc Outdated Show resolved Hide resolved
@chapulina chapulina added the needs upstream release Blocked by a release of an upstream library label Jun 16, 2021
include/ignition/sensors/SegmentationCameraSensor.hh Outdated Show resolved Hide resolved
include/ignition/sensors/SegmentationCameraSensor.hh Outdated Show resolved Hide resolved
include/ignition/sensors/SegmentationCameraSensor.hh Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
src/CMakeLists.txt Show resolved Hide resolved
test/integration/segmentation_camera_plugin.cc Outdated Show resolved Hide resolved
test/integration/segmentation_camera_plugin.cc Outdated Show resolved Hide resolved
test/integration/segmentation_camera_plugin.cc Outdated Show resolved Hide resolved
test/integration/segmentation_camera_plugin.cc Outdated Show resolved Hide resolved
test/integration/segmentation_camera_plugin.cc Outdated Show resolved Hide resolved
Signed-off-by: AmrElsersy <amrelsersay@gmail.com>
Signed-off-by: AmrElsersy <amrelsersay@gmail.com>
@AmrElsersy
Copy link
Contributor Author

The sensor supports now publishing both colored and labels maps together instead of publishing them individually

sensor_semantic

include/ignition/sensors/SegmentationCameraSensor.hh Outdated Show resolved Hide resolved
src/SegmentationCameraSensor.cc Outdated Show resolved Hide resolved
src/SegmentationCameraSensor.cc Outdated Show resolved Hide resolved
src/SegmentationCameraSensor.cc Outdated Show resolved Hide resolved
src/SegmentationCameraSensor.cc Outdated Show resolved Hide resolved
void BuildScene(rendering::ScenePtr scene)
{
math::Vector3d leftPosition(3, -1.5, 0);
math::Vector3d rightPosition(3, 1.5, 0);
math::Vector3d middlePosition(3, 0, 0);
math::Vector3d hiddenPosition(8, 0, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add one more box that is partially hidden? Perhaps it can be poking out of the right of the left-most box. It'd be nice to add test coverage to make sure that for partially hidden objects, we only see the part that is visible in the segmented image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is already tested in the rendering::SegmentationCamera

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, it seems like there is some overlap/redundancy in testing between ign-sensors and ign-rendering, but I'm not sure what kind of tests should go in which repository for a given sensor and its rendering capabilities... @iche033 would you mind chiming in to help clarify which tests belong to which repository?

test/integration/segmentation_camera_plugin.cc Outdated Show resolved Hide resolved
test/integration/segmentation_camera_plugin.cc Outdated Show resolved Hide resolved
test/integration/segmentation_camera_plugin.cc Outdated Show resolved Hide resolved
test/integration/segmentation_camera_plugin.cc Outdated Show resolved Hide resolved
Signed-off-by: AmrElsersy <amrelsersay@gmail.com>
@AmrElsersy AmrElsersy requested a review from adlarkin July 17, 2021 03:36
include/ignition/sensors/SegmentationCameraSensor.hh Outdated Show resolved Hide resolved
include/ignition/sensors/SegmentationCameraSensor.hh Outdated Show resolved Hide resolved
src/SegmentationCameraSensor.cc Outdated Show resolved Hide resolved
src/SegmentationCameraSensor.cc Outdated Show resolved Hide resolved
src/SegmentationCameraSensor.cc Outdated Show resolved Hide resolved
src/SegmentationCameraSensor.cc Outdated Show resolved Hide resolved
src/SegmentationCameraSensor.cc Outdated Show resolved Hide resolved
src/SegmentationCameraSensor.cc Outdated Show resolved Hide resolved
test/integration/segmentation_camera_plugin.cc Outdated Show resolved Hide resolved
void BuildScene(rendering::ScenePtr scene)
{
math::Vector3d leftPosition(3, -1.5, 0);
math::Vector3d rightPosition(3, 1.5, 0);
math::Vector3d middlePosition(3, 0, 0);
math::Vector3d hiddenPosition(8, 0, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, it seems like there is some overlap/redundancy in testing between ign-sensors and ign-rendering, but I'm not sure what kind of tests should go in which repository for a given sensor and its rendering capabilities... @iche033 would you mind chiming in to help clarify which tests belong to which repository?

@adlarkin adlarkin mentioned this pull request Jul 19, 2021
8 tasks
Signed-off-by: AmrElsersy <amrelsersay@gmail.com>
adlarkin and others added 4 commits August 26, 2021 13:42
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
…lization

Signed-off-by: AmrElsersy <amrelsersay@gmail.com>
Signed-off-by: AmrElsersy <amrelsersay@gmail.com>
@adlarkin
Copy link
Contributor

adlarkin commented Sep 1, 2021

I'm unable to build this PR in a colcon workspace. I believe I am including all of the dependent PRs (gazebosim/sdformat#592, gazebosim/gz-rendering#329, gazebosim/gz-msgs#165), but I see the following error output when building:

Starting >>> ignition-sensors6
[Processing: ignition-sensors6]                                    
--- stderr: ignition-sensors6                                       
../../lib/libignition-sensors6-segmentation_camera.so.6.0.0~pre1: undefined reference to `std::filesystem::__cxx11::path::_M_split_cmpts()'
../../lib/libignition-sensors6-segmentation_camera.so.6.0.0~pre1: undefined reference to `std::filesystem::__cxx11::directory_iterator::directory_iterator(std::filesystem::__cxx11::path const&, std::filesystem::directory_options, std::error_code*)'
../../lib/libignition-sensors6-segmentation_camera.so.6.0.0~pre1: undefined reference to `std::filesystem::__cxx11::directory_iterator::operator++()'
../../lib/libignition-sensors6-segmentation_camera.so.6.0.0~pre1: undefined reference to `std::filesystem::__cxx11::directory_iterator::operator*() const'
collect2: error: ld returned 1 exit status
make[2]: *** [bin/INTEGRATION_segmentation_camera] Error 1
make[1]: *** [test/integration/CMakeFiles/INTEGRATION_segmentation_camera.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....
make: *** [all] Error 2
---
Failed   <<< ignition-sensors6 [42.8s, exited with code 2]

I am also seeing similar issues in #136. It looks like it's only an issue with the integration tests, which makes me think that this behavior started in ed5c136, after merging in the changes from #90. I was able to go back to commit e2c5b52 (the commit before the merge mentioned above) and build this PR just fine.

@iche033
Copy link
Contributor

iche033 commented Sep 1, 2021

I'm unable to build this PR in a colcon workspace

std::filesystem::directory_iterator was added in this commit 21b5d57

How about switching to use DirIter from ign-common? Example usage here

Signed-off-by: AmrElsersy <amrelsersay@gmail.com>
Signed-off-by: AmrElsersy <amrelsersay@gmail.com>
@chapulina chapulina added the beta Targeting beta release of upcoming collection label Sep 8, 2021
adlarkin referenced this pull request in gazebosim/gz-rendering Sep 16, 2021
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
chapulina and others added 4 commits September 20, 2021 18:14
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
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.

LGTM! We'll need to wait for gazebosim/gz-rendering#419 to be merged before merging this, since the label map behavior and integration test fails without it

tutorials/05_segmentation_camera.md Outdated Show resolved Hide resolved
@chapulina
Copy link
Contributor

I think we should leave the tutorials without numbers on their filenames. That doesn't scale well

@adlarkin
Copy link
Contributor

I think we should leave the tutorials without numbers on their filenames

So, instead of 05_segmentation_camera.md, just segmentation_camera.md?

@chapulina
Copy link
Contributor

So, instead of 05_segmentation_camera.md, just segmentation_camera.md?

Yup 👍 I see some other files being moved too. That can be reverted

Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
@adlarkin
Copy link
Contributor

adlarkin commented Sep 22, 2021

Okay, all numbers have been removed from the tutorial files in 8642b60. We will probably need to make a release of ign-rendering before merging this, since gazebosim/gz-rendering#419 was just merged. CI on this PR will be red until an ign-rendering release is made, since gazebosim/gz-rendering#419 fixes the label map behavior and integration test failures.

@codecov
Copy link

codecov bot commented Sep 22, 2021

Codecov Report

Merging #133 (8642b60) into main (e99a8bc) will decrease coverage by 2.37%.
The diff coverage is 52.80%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #133      +/-   ##
==========================================
- Coverage   77.90%   75.52%   -2.38%     
==========================================
  Files          24       25       +1     
  Lines        2394     2644     +250     
==========================================
+ Hits         1865     1997     +132     
- Misses        529      647     +118     
Impacted Files Coverage Δ
src/SegmentationCameraSensor.cc 52.80% <52.80%> (ø)

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 e99a8bc...8642b60. Read the comment docs.

@chapulina chapulina merged commit fb97d96 into gazebosim:main Sep 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection 🏯 fortress Ignition Fortress needs upstream release Blocked by a release of an upstream library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants