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

Remove plugin interface and support custom sensors #90

Merged
merged 22 commits into from
Aug 12, 2021

Conversation

chapulina
Copy link
Contributor

@chapulina chapulina commented Feb 5, 2021

Requires gazebosim/gz-plugin#35

This does a lot of the same things as #38, except for:

  • I got rid of the IGN_SENSORS_REGISTER_SENSOR in favor of using IGNITION_ADD_PLUGIN directly. That matches better what we've been doing on other libraries and simplifies some registration details. The downside is that it isn't being tick-tocked yet. But I wonder if this was even usable by downstream users, considering Support custom sensors #9. What do you think, @iche033 ?
  • I didn't need to change any tests or anything in the Manager
  • There's no assumption that the interface we want is the last one, we iterate until finding the correct interface.

I only tested this locally, curious to see how it will work on CI.


6 months later...

This PR now depends on gazebosim/sdformat#652

This pull request started as a migration from Ignition Common plugins to Ignition Plugins. As we iterated on it, it became clear that it wasn't feasible to link against the sensor plugins, and then load them at runtime one more time, which is what the old implementation was doing.

We can't escape linking against each sensor if we want to use their APIs in Gazebo systems. But we can live without the plugin interface. With that in mind, we decided to completely remove the plugin interface instead of migrating it to Ignition Plugin.

See the migration guide for more information on what has changed, and the new examples for how to use the current APIs, including an example of how to implement custom sensors.


Let me know what you think, @ahcorde

@chapulina chapulina added enhancement New feature or request needs upstream release Blocked by a release of an upstream library labels Feb 5, 2021
@github-actions github-actions bot added the 🏢 edifice Ignition Edifice label Feb 5, 2021
@chapulina chapulina mentioned this pull request Feb 5, 2021
src/SensorFactory.cc Outdated Show resolved Hide resolved
@ahcorde
Copy link
Contributor

ahcorde commented Feb 5, 2021

Did you try to run any test on ign-gazebo ? or Did you try to run ign gazebo with some sensor plugins?

All sensor plugins are failing

	 47 - INTEGRATION_air_pressure_system (Failed)
	 49 - INTEGRATION_altimeter_system (SEGFAULT)
	 67 - INTEGRATION_diff_drive_system (Failed)
	 81 - INTEGRATION_imu_system (SEGFAULT)
	 99 - INTEGRATION_logical_camera_system (SEGFAULT)
	103 - INTEGRATION_magnetometer_system (SEGFAULT)
	127 - INTEGRATION_sdf_include (Failed)
	139 - INTEGRATION_log_system (Failed)
	141 - INTEGRATION_wheel_slip (Failed)
	147 - INTEGRATION_camera_video_record_system (SEGFAULT)
	149 - INTEGRATION_depth_camera (SEGFAULT)
	151 - INTEGRATION_gpu_lidar (SEGFAULT)
	153 - INTEGRATION_rgbd_camera (SEGFAULT)
	155 - INTEGRATION_sensors_system (SEGFAULT)

When I run for example ign gazebo -r -v 4 camera_sensor.sdf I got this error:

[Err] [SensorFactory.cc:119] Unable to instantiate sensor plugin for [ignition-sensors4-camera]
[Err] [Sensors.cc:516] Failed to create sensor [camera::link::camera]
[Err] [SceneManager.cc:993] Unable to find sensor []
[Err] [RenderUtil.cc:503] Failed to create sensor []

This is the issue why I think we should use GLOBAL instead of LOCAL.

@chapulina
Copy link
Contributor Author

Did you try to run any test on ign-gazebo ?

Argh, no I hadn't, and now I see it doesn't work. It was too good to be true. I'll look into it more.

I'm having a hard time trying out #38 because it's outdated, so I'm pulling changes from it. I tried fixing conflicts but I must have done something wrong because it wasn't working for me 😕

@ahcorde
Copy link
Contributor

ahcorde commented Feb 5, 2021

I will update my PR

Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
@chapulina chapulina mentioned this pull request Feb 17, 2021
8 tasks
Signed-off-by: Louise Poubel <[email protected]>
@chapulina chapulina marked this pull request as ready for review February 17, 2021 02:41
src/Lidar.cc Outdated Show resolved Hide resolved
test/integration/camera_plugin.cc Outdated Show resolved Hide resolved
Signed-off-by: Louise Poubel <[email protected]>
@chapulina
Copy link
Contributor Author

I opened this for review.

@chapulina chapulina added the beta Targeting beta release of upcoming collection label Mar 17, 2021
@chapulina chapulina changed the title Use ign-plugin and support custom sensors Remove plugin interface and support custom sensors Aug 5, 2021
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
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.

bitbucket-pipelines.yml Show resolved Hide resolved
include/ignition/sensors/Util.hh Show resolved Hide resolved
include/ignition/sensors/Util.hh Show resolved Hide resolved
tutorials/custom_sensors.md Outdated Show resolved Hide resolved
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: Louise Poubel <[email protected]>
@codecov
Copy link

codecov bot commented Aug 10, 2021

Codecov Report

Merging #90 (949f519) into main (d21c6f3) will increase coverage by 0.28%.
The diff coverage is 56.84%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #90      +/-   ##
==========================================
+ Coverage   77.22%   77.51%   +0.28%     
==========================================
  Files          23       24       +1     
  Lines        2358     2326      -32     
==========================================
- Hits         1821     1803      -18     
+ Misses        537      523      -14     
Impacted Files Coverage Δ
src/AirPressureSensor.cc 85.50% <ø> (-0.21%) ⬇️
src/AltimeterSensor.cc 87.95% <ø> (-0.15%) ⬇️
src/CameraSensor.cc 76.49% <ø> (+1.77%) ⬆️
src/DepthCameraSensor.cc 73.83% <ø> (-0.11%) ⬇️
src/GpuLidarSensor.cc 88.15% <ø> (-0.08%) ⬇️
src/ImuSensor.cc 90.09% <ø> (-0.10%) ⬇️
src/Lidar.cc 72.19% <ø> (-0.15%) ⬇️
src/LogicalCameraSensor.cc 89.55% <ø> (-0.16%) ⬇️
src/MagnetometerSensor.cc 88.09% <ø> (-0.15%) ⬇️
src/SensorFactory.cc 25.00% <0.00%> (-29.44%) ⬇️
... and 8 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 d21c6f3...949f519. Read the comment docs.

@chapulina chapulina dismissed ahcorde’s stale review August 10, 2021 22:59

Style issues are resolved and CI is all happy

@chapulina chapulina removed the needs upstream release Blocked by a release of an upstream library label Aug 10, 2021
@chapulina
Copy link
Contributor Author

chapulina commented Aug 11, 2021

As mentioned on the migration guide, it wasn't possible to tick-tock all of the API. So once this is merged, downstream (i.e. ign-gazebo) will be broken. So here are the next steps:

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants