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

Upgrade ign-sensors and support custom sensors #617

Merged
merged 22 commits into from
Aug 13, 2021

Conversation

chapulina
Copy link
Contributor

@chapulina chapulina commented Feb 6, 2021

Part of gazebosim/gz-sensors#18

Summary

Requires gazebosim/gz-plugin#35 and gazebosim/gz-sensors#90.

This is an alternative to #304.

tl;dr: Loading symbols globally is a way of making we find them after they've been registered by another library.


TODO: write up a summary of what I think is happening.

Test it

  • Use all the PRs listed above
  • Run worlds that contain sensors, like ign gazebo sensors.sdf or ign gazebo sensors_demo.sdf
  • Sensor data should be available through topics, camera images should be visible on widgets

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example world 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

Note to maintainers: Remember to use Squash-Merge

Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina chapulina added enhancement New feature or request sensors Sensors and sensor data labels Feb 6, 2021
@github-actions github-actions bot added the 🏢 edifice Ignition Edifice label Feb 6, 2021
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
…itionrobotics/ign-gazebo into chapulina/5/ign-plugin_sensors

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

I made a comment about the place to change the flag. I tested locally and all system plugins are failing.

Don't merge the other related PR yet until we are sure we can use this solution.

src/SystemLoader.cc Outdated Show resolved Hide resolved
@chapulina chapulina added the needs upstream release Blocked by a release of an upstream library label Feb 17, 2021
@chapulina chapulina marked this pull request as ready for review February 17, 2021 20:29
@chapulina chapulina added the beta Targeting beta release of upcoming collection label Mar 17, 2021
@chapulina chapulina self-assigned this Mar 19, 2021
@chapulina chapulina removed the beta Targeting beta release of upcoming collection label Mar 23, 2021
@chapulina
Copy link
Contributor Author

Removing beta label, we won't have time to wrap this up before code freeze. Let's retarget at Ignition-F.

@chapulina chapulina added 🏯 fortress Ignition Fortress and removed 🏢 edifice Ignition Edifice labels Apr 1, 2021
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@ahcorde
Copy link
Contributor

ahcorde commented Aug 12, 2021

The INTEGRATION_examples_build is failing on MacOS becuase it's not able to find ignition/sensors/Util.hh.

@chapulina chapulina removed the needs upstream release Blocked by a release of an upstream library label Aug 12, 2021
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@codecov
Copy link

codecov bot commented Aug 13, 2021

Codecov Report

Merging #617 (341647b) into main (e3ee0af) will decrease coverage by 0.30%.
The diff coverage is 59.15%.

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

@@            Coverage Diff             @@
##             main     #617      +/-   ##
==========================================
- Coverage   64.66%   64.35%   -0.31%     
==========================================
  Files         242      245       +3     
  Lines       19014    19217     +203     
==========================================
+ Hits        12296    12368      +72     
- Misses       6718     6849     +131     
Impacted Files Coverage Δ
include/ignition/gazebo/Link.hh 100.00% <ø> (ø)
include/ignition/gazebo/Server.hh 100.00% <ø> (ø)
include/ignition/gazebo/rendering/RenderUtil.hh 100.00% <ø> (ø)
src/SdfEntityCreator.cc 84.53% <0.00%> (-0.89%) ⬇️
src/gui/Gui.cc 65.35% <ø> (+0.23%) ⬆️
src/gui/GuiRunner.cc 85.91% <0.00%> (-0.34%) ⬇️
src/gui/plugins/scene3d/Scene3D.cc 10.83% <ø> (+0.01%) ⬆️
src/systems/log/LogRecord.cc 80.56% <ø> (-0.31%) ⬇️
src/rendering/RenderUtil.cc 34.67% <14.28%> (-0.17%) ⬇️
src/systems/physics/Physics.cc 70.27% <20.00%> (-0.20%) ⬇️
... and 22 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 b40ca3b...bb6b58a. Read the comment docs.

@chapulina
Copy link
Contributor Author

The 23 test failures on Jenkins Ubuntu are due to gazebo-tooling/release-tools#494 and can be ignored for now. #971 should fix them if we don't fix it in the infra. The rest of CI looks good. Merging!

@chapulina chapulina merged commit e040ec2 into main Aug 13, 2021
@chapulina chapulina deleted the chapulina/5/ign-plugin_sensors branch August 13, 2021 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request 🏯 fortress Ignition Fortress sensors Sensors and sensor data
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants