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

Label Component & System #853

Merged
merged 19 commits into from
Sep 22, 2021
Merged

Label Component & System #853

merged 19 commits into from
Sep 22, 2021

Conversation

AmrElsersy
Copy link
Contributor

@AmrElsersy AmrElsersy commented Jun 9, 2021

🎉 New feature

related to #134 #135

Summary

The goal of the PR is to add a way to add labels to visuals via SDF to be used in Segmentation & BoundingBox cameras.

Depends on

SDF #592

Features

  • Label Component
  • Label Configure System that creates Label component to the visual
  • Editing RenderUtil to pass these label components as UserData to the visuals

How to use

use that plugin format to add label in the tag (for ex: 1)

  <visual>
      <plugin filename="ignition-gazebo-label-system" name="ignition::gazebo::systems::Label">
        <label>1</label>
      </plugin>
  </visual>

The plugin can be added also to the parent model

 <model>
      <plugin filename="ignition-gazebo-label-system" name="ignition::gazebo::systems::Label">
        <label>1</label>
      </plugin>
      <visual> ... </visual>
  </model>

Or to the tag of fuel models

    <include>
      <uri> ... </uri>
      <plugin filename="ignition-gazebo-label-system" name="ignition::gazebo::systems::Label">
        <label>1</label>
      </plugin>
    </include>

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

Notes:

  • The code supposes that each model in sdf will have 1 link and that link will have 1 visual, tell me if you think there is any problems of that
  • I deleted some curly bracket lines because I had a warning that RenderUtil::Update function has exceed 500 line

Signed-off-by: AmrElsersy <amrelsersay@gmail.com>
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Jun 9, 2021
include/ignition/gazebo/components/Label.hh Outdated Show resolved Hide resolved
include/ignition/gazebo/components/Label.hh Outdated Show resolved Hide resolved
src/rendering/RenderUtil.cc Outdated Show resolved Hide resolved
src/rendering/RenderUtil.cc Outdated Show resolved Hide resolved
src/rendering/RenderUtil.cc Outdated Show resolved Hide resolved
src/rendering/RenderUtil.cc Outdated Show resolved Hide resolved
src/systems/label/Label.hh Outdated Show resolved Hide resolved
src/systems/label/Label.hh Outdated Show resolved Hide resolved
@adlarkin adlarkin self-requested a review June 10, 2021 16:28
Modified the RenderUtil & SdfEntityCreator to recognize the new components

Signed-off-by: AmrElsersy <amrelsersay@gmail.com>
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.

Looking good so far! Would you mind adding an integration test for the plugin? You can refer to other plugin integration tests as an example in the test/integration directory.

src/systems/label/Label.cc Outdated Show resolved Hide resolved
src/systems/label/Label.cc Outdated Show resolved Hide resolved
src/systems/label/Label.hh Outdated Show resolved Hide resolved
include/ignition/gazebo/components/BoundingBoxCamera.hh Outdated Show resolved Hide resolved
include/ignition/gazebo/components/SegmentationCamera.hh Outdated Show resolved Hide resolved
src/systems/label/Label.cc Outdated Show resolved Hide resolved
src/systems/label/Label.cc Outdated Show resolved Hide resolved
src/systems/label/Label.cc Outdated Show resolved Hide resolved
src/systems/label/Label.hh Outdated Show resolved Hide resolved
src/systems/sensors/Sensors.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
Signed-off-by: AmrElsersy <amrelsersay@gmail.com>
@AmrElsersy
Copy link
Contributor Author

AmrElsersy commented Jul 19, 2021

When there is a model of many links such as the Double pendulum, I loop over the visuals inside the model and set their label with the specified label

    <include>
      <name>Double pendulum</name>
      <pose>-3 0 0 0 0 0</pose>
      <uri>https://fuel.ignitionrobotics.org/1.0/OpenRobotics/models/Double pendulum with base</uri>
      <plugin filename="ignition-gazebo-label-system" name="ignition::gazebo::systems::Label">
        <label>2</label>
      </plugin>
    </include>

But the problem is that it considers each part of the pendulum is a separate object, as each part of it has different ogre ID
visible_2d_fuel

So do you have an idea about how to make it consider the whole model as the same object ?? .. most sensors are based on material switching, by switching the material of each ogre ID, so each projected pixel will have the value of ogre ID embedded into it

the only case that it works in it, is in the semantic segmentation, as we only consider the label but not ogre ID
but all other stuff are based on OGRE ID such as panoptic segmentation / 2D boxes (both modes) and 3D Boxes

semantic

instance

Note: all work fine if the item is simple (1 link) or it is a complex item but built with mesh
The case where the problems happen is when the model contains many links, we want the user when he specify a label for the whole model, the sensor considers the whole model as 1 item

@chapulina @adlarkin @ahcorde

@adlarkin
Copy link
Contributor

When there is a model of many links such as the Double pendulum, I loop over the visuals inside the model and set their label with the specified label

    <include>
      <name>Double pendulum</name>
      <pose>-3 0 0 0 0 0</pose>
      <uri>https://fuel.ignitionrobotics.org/1.0/OpenRobotics/models/Double pendulum with base</uri>
      <plugin filename="ignition-gazebo-label-system" name="ignition::gazebo::systems::Label">
        <label>2</label>
      </plugin>
    </include>

But the problem is that it considers each part of the pendulum is a separate object, as each part of it has different ogre ID
visible_2d_fuel

So do you have an idea about how to make it consider the whole model as the same object ?? .. most sensors are based on material switching, by switching the material of each ogre ID, so each projected pixel will have the value of ogre ID embedded into it

the only case that it works in it, is in the semantic segmentation, as we only consider the label but not ogre ID
but all other stuff are based on OGRE ID such as panoptic segmentation / 2D boxes (both modes) and 3D Boxes

semantic

instance

Note: all work fine if the item is simple (1 link) or it is a complex item but built with mesh
The case where the problems happen is when the model contains many links, we want the user when he specify a label for the whole model, the sensor considers the whole model as 1 item

@AmrElsersy let's continue this conversation in gazebosim/gz-rendering#334 since you posted some follow-up comments about this there

Signed-off-by: AmrElsersy <amrelsersay@gmail.com>
@AmrElsersy AmrElsersy requested a review from ahcorde August 6, 2021 11:17
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.

Conflicts

…into main

Signed-off-by: AmrElsersy <amrelsersay@gmail.com>
@AmrElsersy AmrElsersy requested a review from ahcorde August 10, 2021 16:28
@AmrElsersy
Copy link
Contributor Author

I could track the actor visual and set its user data with the label ... but when the material switching happen in the rendering PR .. I found a strange behavior in the actor visual

actor

@chapulina @adlarkin

Signed-off-by: AmrElsersy <amrelsersay@gmail.com>
…into main

Signed-off-by: AmrElsersy <amrelsersay@gmail.com>
examples/worlds/segmentation_camera.sdf Outdated Show resolved Hide resolved
examples/worlds/segmentation_camera.sdf Outdated Show resolved Hide resolved
examples/worlds/segmentation_camera.sdf Outdated Show resolved Hide resolved
examples/worlds/segmentation_camera.sdf Outdated Show resolved Hide resolved
adlarkin and others added 4 commits September 21, 2021 22:38
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.

This PR has been updated to only include functionality for the segmentation sensor for now. Once the initial fortress release is out and we add in the bounding box sensor in a minor release, we can merge #1040 to re-add the bounding box functionality for this plugin.

src/systems/label/Label.cc Outdated Show resolved Hide resolved
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
@adlarkin
Copy link
Contributor

@chapulina I forgot to mention that when I run segmentation_camera.sdf, I see quite a bit of errors in the console:

[Err] [SystemPaths.cc:467] Could not resolve file [hatchback.png]
[Err] [SystemPaths.cc:377] Unable to find file with URI [model://hatchback/materials/textures/wheels3.png]
[Err] [SystemPaths.cc:467] Could not resolve file [model://hatchback/materials/textures/wheels3.png]
[Err] [Material.cc:164] Unable to find texture [model://hatchback/materials/textures/wheels3.png] as a locally cached texture or in path [/home/adl-ws/.ignition/fuel/fuel.ignitionrobotics.org/openrobotics/models/hatchback blue/2/meshes]
[Err] [SystemPaths.cc:467] Could not resolve file [pickup_diffuse.jpg]
[Err] [SystemPaths.cc:467] Could not resolve file [wheels2.jpg]
[Err] [SystemPaths.cc:467] Could not resolve file [suv.png]
[Err] [SystemPaths.cc:467] Could not resolve file [wheels_01.png]
[Err] [SystemPaths.cc:467] Could not resolve file [collapsed_house_diffuse.jpg]
[GUI] [Err] [SystemPaths.cc:467] Could not resolve file [hatchback.png]
[GUI] [Err] [SystemPaths.cc:377] Unable to find file with URI [model://hatchback/materials/textures/wheels3.png]
[GUI] [Err] [SystemPaths.cc:467] Could not resolve file [model://hatchback/materials/textures/wheels3.png]
[GUI] [Err] [Material.cc:164] Unable to find texture [model://hatchback/materials/textures/wheels3.png] as a locally cached texture or in path [/home/adl-ws/.ignition/fuel/fuel.ignitionrobotics.org/openrobotics/models/hatchback blue/2/meshes]
[GUI] [Err] [SystemPaths.cc:467] Could not resolve file [pickup_diffuse.jpg]
[GUI] [Err] [SystemPaths.cc:467] Could not resolve file [wheels2.jpg]
[GUI] [Err] [SystemPaths.cc:467] Could not resolve file [suv.png]
[GUI] [Err] [SystemPaths.cc:467] Could not resolve file [wheels_01.png]
[GUI] [Err] [SystemPaths.cc:467] Could not resolve file [collapsed_house_diffuse.jpg]

I don't think these errors are related to this PR though. Have you seen this before?

@chapulina
Copy link
Contributor

I don't think these errors are related to this PR though. Have you seen this before?

Agreed that these don't seem to be related to the PR, the models don't seem right

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

I'm having some issues loading some of the models in the example world, but besides that everything else seems to be in place 👍

@mjcarroll mjcarroll mentioned this pull request Sep 22, 2021
7 tasks
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina chapulina removed the needs upstream release Blocked by a release of an upstream library label Sep 22, 2021
@codecov
Copy link

codecov bot commented Sep 22, 2021

Codecov Report

Merging #853 (65f186a) into main (2c2605f) will decrease coverage by 0.01%.
The diff coverage is 49.14%.

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

@@            Coverage Diff             @@
##             main     #853      +/-   ##
==========================================
- Coverage   63.61%   63.60%   -0.02%     
==========================================
  Files         252      254       +2     
  Lines       19794    19937     +143     
==========================================
+ Hits        12592    12680      +88     
- Misses       7202     7257      +55     
Impacted Files Coverage Δ
include/ignition/gazebo/EntityComponentManager.hh 100.00% <ø> (ø)
include/ignition/gazebo/rendering/SceneManager.hh 100.00% <ø> (ø)
src/gui/plugins/spawn/Spawn.cc 10.25% <0.00%> (-0.05%) ⬇️
src/systems/sensors/Sensors.cc 70.48% <0.00%> (-1.27%) ⬇️
src/systems/user_commands/UserCommands.cc 59.10% <0.00%> (-1.78%) ⬇️
src/rendering/SceneManager.cc 28.77% <28.57%> (-0.09%) ⬇️
src/SdfEntityCreator.cc 83.75% <33.33%> (-0.39%) ⬇️
src/rendering/RenderUtil.cc 36.34% <44.51%> (+0.48%) ⬆️
src/EntityComponentManager.cc 84.95% <87.69%> (+0.28%) ⬆️
include/ignition/gazebo/components/Component.hh 100.00% <100.00%> (ø)
... and 4 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 d889c5f...a72d342. Read the comment docs.

Signed-off-by: Louise Poubel <louise@openrobotics.org>
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.

6 participants