Skip to content

Segmentation Camera#329

Merged
adlarkin merged 34 commits intogazebosim:mainfrom
AmrElsersy:segmentation
Sep 20, 2021
Merged

Segmentation Camera#329
adlarkin merged 34 commits intogazebosim:mainfrom
AmrElsersy:segmentation

Conversation

@AmrElsersy
Copy link
Contributor

@AmrElsersy AmrElsersy commented May 28, 2021

🎉 New feature

related to #134

Summary

Segmentation Camera to capture semantic information, Implemented using OGRE 2 by using the material switching to provide Semantic & Panoptic Segmentation

Semantic Segmentation

  • Pixels of same label from different items, have the same color & id

Panoptic Segmentation

  • Pixels of same label from different items, have different color & id
  • 1 channel for label id & 2 channels for instance id

Features:

  • Supports both Semantic Ids map(where each pixel has a value = label id) & Colored map(where each pixel has a color) for visualization purposes
  • The user label each item by setting its UserData
  • Each unlabeled item is considered as a background
  • Added example in ign-rendering/examples to try it easily

@AmrElsersy AmrElsersy requested a review from iche033 as a code owner May 28, 2021 18:47
@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label May 28, 2021
Copy link

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

Overall looking really good. A few notes:

  • Copyright 2021 on newly-created files
  • ifdefs get all caps
  • Initialize pointers to nullptr

@AmrElsersy AmrElsersy requested review from ahcorde and mjcarroll May 28, 2021 22:20
@adlarkin adlarkin self-requested a review May 28, 2021 22:31
@AmrElsersy AmrElsersy requested a review from ahcorde May 31, 2021 16:40
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

Signed-off-by: AmrElsersy <amrelsersay@gmail.com>
@adlarkin adlarkin changed the base branch from ign-rendering3 to main June 7, 2021 15:06
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
@adlarkin adlarkin added 🏯 fortress Ignition Fortress and removed 🏰 citadel Ignition Citadel labels Jun 7, 2021
@adlarkin adlarkin self-requested a review June 10, 2021 14:45
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 add some tests for the segmentation camera as well? It would be great to have unit tests (in the src/ directory) and an integration test (in the test/integration/ directory).

AmrElsersy and others added 3 commits June 14, 2021 14:12
Signed-off-by: AmrElsersy <amrelsersay@gmail.com>
Signed-off-by: AmrElsersy <amrelsersay@gmail.com>
Signed-off-by: AmrElsersy <amrelsersay@gmail.com>
Signed-off-by: AmrElsersy <amrelsersay@gmail.com>
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
adlarkin referenced this pull request Sep 16, 2021
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
Michael Carroll and others added 8 commits September 16, 2021 12:17
Signed-off-by: Michael Carroll <michael@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
Signed-off-by: Ian Chen <ichen@osrfoundation.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.

The sensor itself seems to work well, but I did find a few issues with mouse functionality in the example. Since we want to get this in for Fortress, I'm approving this so that it can get merged, but I've ticketed an issue that outlines the mouse functionality issues in the example so that we can work on fixing them later: #414

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

codecov bot commented Sep 18, 2021

Codecov Report

Merging #329 (b40fa21) into main (b2da7e1) will increase coverage by 0.31%.
The diff coverage is 71.16%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #329      +/-   ##
==========================================
+ Coverage   54.74%   55.06%   +0.31%     
==========================================
  Files         187      191       +4     
  Lines       18890    19265     +375     
==========================================
+ Hits        10342    10608     +266     
- Misses       8548     8657     +109     
Impacted Files Coverage Δ
include/ignition/rendering/Scene.hh 0.00% <ø> (ø)
include/ignition/rendering/base/BaseScene.hh 0.00% <0.00%> (ø)
ogre2/src/Ogre2SegmentationMaterialSwitcher.cc 62.77% <62.77%> (ø)
.../ignition/rendering/base/BaseSegmentationCamera.hh 70.96% <70.96%> (ø)
ogre2/src/Ogre2SegmentationCamera.cc 75.67% <75.67%> (ø)
include/ignition/rendering/SegmentationCamera.hh 100.00% <100.00%> (ø)
ogre2/src/Ogre2Scene.cc 76.69% <100.00%> (+0.15%) ⬆️
ogre2/src/Ogre2ThermalCamera.cc 89.51% <100.00%> (ø)
src/base/BaseScene.cc 75.33% <100.00%> (+0.48%) ⬆️
... and 3 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 b2da7e1...b40fa21. Read the comment docs.

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

adlarkin commented Sep 20, 2021

It looks like there's a new warning on ubuntu CI that we should fix before merging:

   CONFIGURATION WARNINGS:
   -- Missing dependency [IgnOGRE] (Components: RTShaderSystem, Terrain, Overlay)
Call Stack (most recent call first):

@iche033
Copy link
Contributor

iche033 commented Sep 20, 2021

It looks like there's a new warning on ubuntu CI that we should fix before merging:

could be related to #400, which is fixed in #411

@adlarkin
Copy link
Contributor

fixed in #411

Yeah, I think you're right. It looks like #411 hasn't been forward ported to main yet, so should we go ahead and merge this since the Ubuntu CI warning seems to be a false positive?

Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
@adlarkin adlarkin dismissed stale reviews from ahcorde and mjcarroll September 20, 2021 21:57

Feedback has been addressed

@adlarkin adlarkin enabled auto-merge (squash) September 20, 2021 21:59
@adlarkin adlarkin merged commit 7aade51 into gazebosim:main Sep 20, 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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants