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 Camera #329

Merged
merged 34 commits into from
Sep 20, 2021
Merged

Segmentation Camera #329

merged 34 commits into from
Sep 20, 2021

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
Contributor

@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

include/ignition/rendering/Scene.hh Outdated Show resolved Hide resolved
include/ignition/rendering/Scene.hh Outdated Show resolved Hide resolved
include/ignition/rendering/Scene.hh Outdated Show resolved Hide resolved
include/ignition/rendering/Scene.hh Outdated Show resolved Hide resolved
include/ignition/rendering/SegmentationCamera.hh Outdated Show resolved Hide resolved
ogre2/src/Ogre2SegmentationCamera.cc Outdated Show resolved Hide resolved
ogre2/src/Ogre2SegmentationCamera.cc Outdated Show resolved Hide resolved
ogre2/src/Ogre2SegmentationCamera.cc Outdated Show resolved Hide resolved
examples/segmentation_camera/CMakeLists.txt Show resolved Hide resolved
examples/segmentation_camera/GlutWindow.cc Outdated Show resolved Hide resolved
examples/segmentation_camera/GlutWindow.cc Outdated Show resolved Hide resolved
examples/segmentation_camera/GlutWindow.cc Outdated Show resolved Hide resolved
examples/segmentation_camera/GlutWindow.cc Show resolved Hide resolved
ogre2/src/Ogre2SegmentationCamera.cc Outdated Show resolved Hide resolved
@AmrElsersy AmrElsersy requested review from ahcorde and mjcarroll May 28, 2021 22:20
@adlarkin adlarkin self-requested a review May 28, 2021 22:31
examples/segmentation_camera/Main.cc Outdated Show resolved Hide resolved
include/ignition/rendering/SegmentationCamera.hh Outdated Show resolved Hide resolved
include/ignition/rendering/SegmentationCamera.hh Outdated Show resolved Hide resolved
ogre2/src/Ogre2SegmentationCamera.cc Outdated Show resolved Hide resolved
ogre2/src/Ogre2SegmentationCamera.cc Outdated Show resolved Hide resolved
ogre2/src/Ogre2SegmentationCamera.cc Outdated Show resolved Hide resolved
@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.

@adlarkin adlarkin changed the base branch from ign-rendering3 to main June 7, 2021 15:06
Signed-off-by: Ashton Larkin <[email protected]>
@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).

examples/segmentation_camera/Main.cc Outdated Show resolved Hide resolved
examples/segmentation_camera/Main.cc Outdated Show resolved Hide resolved
examples/segmentation_camera/Main.cc Outdated Show resolved Hide resolved
examples/segmentation_camera/GlutWindow.cc Outdated Show resolved Hide resolved
examples/segmentation_camera/GlutWindow.cc Show resolved Hide resolved
ogre2/src/Ogre2SegmentationCamera.cc Outdated Show resolved Hide resolved
ogre2/src/Ogre2SegmentationCamera.cc Outdated Show resolved Hide resolved
ogre2/src/Ogre2SegmentationCamera.cc Outdated Show resolved Hide resolved
ogre2/src/Ogre2SegmentationCamera.cc Outdated Show resolved Hide resolved
ogre2/src/Ogre2SegmentationCamera.cc Outdated Show resolved Hide resolved
AmrElsersy and others added 3 commits June 14, 2021 14:12
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.

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 <[email protected]>
@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 <[email protected]>
Signed-off-by: Ashton Larkin <[email protected]>
@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 <[email protected]>
@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.

7 participants