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

Replace ign common plugin loader with ign-plugin #38

Closed
wants to merge 25 commits into from
Closed
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion include/ignition/sensors/CameraSensor.hh
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@

#include <sdf/sdf.hh>

#include <ignition/common/PluginMacros.hh>
#include <ignition/common/Time.hh>

#include <ignition/msgs.hh>
Expand Down
1 change: 0 additions & 1 deletion include/ignition/sensors/DepthCameraSensor.hh
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@

#include <ignition/common/Event.hh>

#include <ignition/common/PluginMacros.hh>
#include <ignition/common/Time.hh>

#include <ignition/msgs.hh>
Expand Down
2 changes: 1 addition & 1 deletion include/ignition/sensors/LogicalCameraSensor.hh
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@

#include <sdf/sdf.hh>

#include <ignition/common/PluginMacros.hh>
#include <ignition/plugin/RegisterMore.hh>
Copy link
Contributor

Choose a reason for hiding this comment

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

No other plugins have RegisterMore on their headers, can it be removed here?

#include <ignition/common/Time.hh>

#include <ignition/math/Angle.hh>
Expand Down
8 changes: 8 additions & 0 deletions include/ignition/sensors/Manager.hh
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,14 @@ namespace ignition
/// is returned on erro.
public: ignition::sensors::SensorId CreateSensor(sdf::ElementPtr _sdf);

/// \brief Add a sensor from a sensor instance.
/// \sa Sensor()
/// \param[in] _sensor pointer to the sensor
/// \return A sensor id that refers to the created sensor. NO_SENSOR
/// is returned on error.
public: ignition::sensors::SensorId AddSensor(
std::unique_ptr<sensors::Sensor> _sensor);

/// \brief Create a sensor from SDF without a known sensor type.
///
/// This creates sensors by looking at the given sdf element.
Expand Down
7 changes: 2 additions & 5 deletions include/ignition/sensors/SensorFactory.hh
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
#include <sdf/sdf.hh>

#include <ignition/common/Console.hh>
#include <ignition/common/PluginMacros.hh>
#include <ignition/plugin/Loader.hh>

#include <ignition/sensors/config.hh>
#include <ignition/sensors/Export.hh>
Expand All @@ -42,9 +42,6 @@ namespace ignition
/// \brief Base sensor plugin interface
class IGNITION_SENSORS_VISIBLE SensorPlugin
{
/// \brief Allows using shorter APIS in common::PluginLoader
public: IGN_COMMON_SPECIALIZE_INTERFACE(ignition::sensors::SensorPlugin)

/// \brief Instantiate new sensor
/// \return New sensor
public: virtual Sensor *New() = 0;
Expand Down Expand Up @@ -182,7 +179,7 @@ namespace ignition

/// \brief Sensor registration macro
#define IGN_SENSORS_REGISTER_SENSOR(classname) \
IGN_COMMON_REGISTER_SINGLE_PLUGIN(\
IGNITION_ADD_PLUGIN(\
ignition::sensors::SensorTypePlugin<classname>, \
ignition::sensors::SensorPlugin)
}
Expand Down
1 change: 0 additions & 1 deletion include/ignition/sensors/ThermalCameraSensor.hh
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@

#include <ignition/common/Event.hh>

#include <ignition/common/PluginMacros.hh>
#include <ignition/common/Time.hh>

#include <ignition/msgs.hh>
Expand Down
2 changes: 2 additions & 0 deletions src/AirPressureSensor.cc
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
#include <ignition/common/Profiler.hh>
#include <ignition/transport/Node.hh>

#include <ignition/plugin/Register.hh>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this include needed at every sensor? Shouldn't SensorFactory.hh be enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According with the migration guide https://github.com/ignitionrobotics/ign-plugin/blob/94482af3e9b2cc1cbb7c2dbeb2c7846f8283f0f6/MIGRATION.md#registering-a-plugin

I added this header for registering plugins. All sensors are individual components that why I need to use ignition/plugin/Register.hh in the cc file. I tried to include this header (or #include <ignition/plugin/RegisterMore.hh>) in the SensorFactory.hh but I was not able to compile the code because an undefined reference:

``/usr/bin/ld: ../lib/libignition-sensors4-imu.so.4.0.0~pre1: undefined reference `IgnitionPluginHook'

Copy link
Contributor

@chapulina chapulina Sep 16, 2020

Choose a reason for hiding this comment

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

According with the migration guide

I see. I mean, we'd still be including that header, just indirectly. The way ign-sensors is exposing the plugin functionality is through SensorFactory. So technically each sensor doesn't need to know about ign-plugins, they just know they need to use the IGN_SENSORS_REGISTER_SENSOR macro from the SensorFactory.hh header.

I tried to include this header (or #include <ignition/plugin/RegisterMore.hh>) in the SensorFactory.hh but I was not able to compile the code because an undefined reference:

SensorFactory.hh uses the IGNITION_ADD_PLUGIN macro from ignition/plugin/Register.hh, so I believe it should include that header. Otherwise, that header is not usable on its own.

And it probably needs to link against ignition-plugin${IGN_PLUGIN_VER}::register to fix the undefined reference. I believe that if you link to it publicly, then each individual sensor won't need to also link against register, only against ign-sensors's core library?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chapulina, I was trying your suggestion but I'm not able to compile the code. I was having a look to other packages that make use of ignition-plugin and the approach it's similar to the one that I used here. For example:

Copy link
Contributor

@chapulina chapulina Sep 17, 2020

Choose a reason for hiding this comment

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

it's similar to the one that I used here

I think the key difference here is that the IGNITION_ADD_PLUGIN macro is used in SensorFactory, not in each plugin. All the examples you gave use that macro directly on the plugin files.

Does SensorFactory.hh work if you don't include Register.hh above it? If it doesn't, that sounds like an architecture flaw. A header should work when included by itself, without the need for extra includes above it, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All plugin are using the MACRO but it's hidden under this other one IGN_SENSORS_REGISTER_SENSOR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chapulina I can remove IGN_SENSORS_REGISTER_SENSOR and make use of `IGNITION_ADD_PLUGIN.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can remove IGN_SENSORS_REGISTER_SENSOR and make use of `IGNITION_ADD_PLUGIN.

That sounds like a big migration effort for users. But since our support for custom sensors is not really there yet (#9), maybe this isn't a big deal. I'd be ok with it. What do you think, @iche033 ?


#include "ignition/sensors/GaussianNoiseModel.hh"
#include "ignition/sensors/Noise.hh"
#include "ignition/sensors/SensorTypes.hh"
Expand Down
10 changes: 5 additions & 5 deletions src/AltimeterSensor.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,13 @@
*/

#include <ignition/common/Profiler.hh>
#include <ignition/plugin/Register.hh>
#include <ignition/sensors/Noise.hh>
#include <ignition/sensors/SensorTypes.hh>
#include <ignition/sensors/SensorFactory.hh>
#include <ignition/sensors/AltimeterSensor.hh>
chapulina marked this conversation as resolved.
Show resolved Hide resolved
#include <ignition/transport/Node.hh>

#include "ignition/sensors/Noise.hh"
#include "ignition/sensors/SensorTypes.hh"
#include "ignition/sensors/SensorFactory.hh"
#include "ignition/sensors/AltimeterSensor.hh"

using namespace ignition;
using namespace sensors;

Expand Down
24 changes: 24 additions & 0 deletions src/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ target_link_libraries(${PROJECT_LIBRARY_TARGET_NAME}
ignition-math${IGN_MATH_VER}::ignition-math${IGN_MATH_VER}
ignition-common${IGN_COMMON_VER}::ignition-common${IGN_COMMON_VER}
ignition-transport${IGN_TRANSPORT_VER}::ignition-transport${IGN_TRANSPORT_VER}
ignition-plugin${IGN_PLUGIN_VER}::loader
sdformat${SDF_VER}::sdformat${SDF_VER}
PRIVATE
ignition-common${IGN_COMMON_VER}::profiler
Expand Down Expand Up @@ -47,10 +48,15 @@ target_link_libraries(${camera_target}
PUBLIC
${rendering_target}
PRIVATE
ignition-plugin${IGN_PLUGIN_VER}::register
ignition-msgs${IGN_MSGS_VER}::ignition-msgs${IGN_MSGS_VER}
ignition-transport${IGN_TRANSPORT_VER}::ignition-transport${IGN_TRANSPORT_VER}
)

target_compile_options(${camera_target}
PUBLIC -fvisibility=hidden -fvisibility-inlines-hidden
)
ahcorde marked this conversation as resolved.
Show resolved Hide resolved

set(depth_camera_sources DepthCameraSensor.cc)
ign_add_component(depth_camera SOURCES ${depth_camera_sources} GET_TARGET_NAME depth_camera_target)
target_compile_definitions(${depth_camera_target} PUBLIC DepthCameraSensor_EXPORTS)
Expand All @@ -68,6 +74,7 @@ target_link_libraries(${lidar_target}
PUBLIC
${rendering_target}
PRIVATE
ignition-plugin${IGN_PLUGIN_VER}::register
ignition-msgs${IGN_MSGS_VER}::ignition-msgs${IGN_MSGS_VER}
ignition-transport${IGN_TRANSPORT_VER}::ignition-transport${IGN_TRANSPORT_VER}
)
Expand All @@ -87,21 +94,38 @@ ign_add_component(logical_camera SOURCES ${logical_camera_sources} GET_TARGET_NA
target_compile_definitions(${logical_camera_target} PUBLIC LogicalCameraSensor_EXPORTS)
target_link_libraries(${logical_camera_target}
PRIVATE
ignition-plugin${IGN_PLUGIN_VER}::register
ignition-msgs${IGN_MSGS_VER}::ignition-msgs${IGN_MSGS_VER}
ignition-transport${IGN_TRANSPORT_VER}::ignition-transport${IGN_TRANSPORT_VER}
)

set(magnetometer_sources MagnetometerSensor.cc)
ign_add_component(magnetometer SOURCES ${magnetometer_sources} GET_TARGET_NAME magnetometer_target)
target_link_libraries(${magnetometer_target}
PRIVATE
ignition-plugin${IGN_PLUGIN_VER}::register
)

set(imu_sources ImuSensor.cc)
ign_add_component(imu SOURCES ${imu_sources} GET_TARGET_NAME imu_target)
target_link_libraries(${imu_target}
PRIVATE
ignition-plugin${IGN_PLUGIN_VER}::register
)

set(altimeter_sources AltimeterSensor.cc)
ign_add_component(altimeter SOURCES ${altimeter_sources} GET_TARGET_NAME altimeter_target)
target_link_libraries(${altimeter_target}
PRIVATE
ignition-plugin${IGN_PLUGIN_VER}::register
)

set(air_pressure_sources AirPressureSensor.cc)
ign_add_component(air_pressure SOURCES ${air_pressure_sources} GET_TARGET_NAME air_pressure_target)
target_link_libraries(${air_pressure_target}
PRIVATE
ignition-plugin${IGN_PLUGIN_VER}::register
)

set(rgbd_camera_sources RgbdCameraSensor.cc)
ign_add_component(rgbd_camera SOURCES ${rgbd_camera_sources} GET_TARGET_NAME rgbd_camera_target)
Expand Down
20 changes: 10 additions & 10 deletions src/CameraSensor.cc
Original file line number Diff line number Diff line change
Expand Up @@ -24,17 +24,16 @@
#include <ignition/common/Profiler.hh>
#include <ignition/common/StringUtils.hh>
#include <ignition/math/Angle.hh>
#include <ignition/transport/Node.hh>

#include <ignition/math/Helpers.hh>

#include "ignition/sensors/CameraSensor.hh"
#include "ignition/sensors/ImageGaussianNoiseModel.hh"
#include "ignition/sensors/ImageNoise.hh"
#include "ignition/sensors/Manager.hh"
#include "ignition/sensors/RenderingEvents.hh"
#include "ignition/sensors/SensorFactory.hh"
#include "ignition/sensors/SensorTypes.hh"
#include <ignition/plugin/Register.hh>
#include <ignition/sensors/CameraSensor.hh>
#include <ignition/sensors/ImageGaussianNoiseModel.hh>
#include <ignition/sensors/ImageNoise.hh>
#include <ignition/sensors/Manager.hh>
#include <ignition/sensors/RenderingEvents.hh>
#include <ignition/sensors/SensorFactory.hh>
#include <ignition/sensors/SensorTypes.hh>
#include <ignition/transport/Node.hh>

using namespace ignition;
using namespace sensors;
Expand Down Expand Up @@ -595,3 +594,4 @@ double CameraSensor::Baseline() const
}

IGN_SENSORS_REGISTER_SENSOR(CameraSensor)
IGNITION_ADD_PLUGIN_ALIAS(CameraSensor, "ignition::plugin::Camera")
ahcorde marked this conversation as resolved.
Show resolved Hide resolved
2 changes: 2 additions & 0 deletions src/DepthCameraSensor.cc
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@
#include <ignition/math/Angle.hh>
#include <ignition/math/Helpers.hh>

#include <ignition/plugin/Register.hh>

#include <ignition/transport/Node.hh>

#include "ignition/sensors/DepthCameraSensor.hh"
Expand Down
10 changes: 6 additions & 4 deletions src/GpuLidarSensor.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,16 @@
* limitations under the License.
*
*/

#include <ignition/msgs/pointcloud_packed.pb.h>
#include <ignition/msgs/Utility.hh>
#include <ignition/transport/Node.hh>

#include <ignition/common/Console.hh>
#include <ignition/common/Profiler.hh>
#include "ignition/sensors/GpuLidarSensor.hh"
#include "ignition/sensors/SensorFactory.hh"
#include <ignition/msgs/Utility.hh>
#include <ignition/plugin/Register.hh>
#include <ignition/sensors/GpuLidarSensor.hh>
#include <ignition/sensors/SensorFactory.hh>
#include <ignition/transport/Node.hh>

using namespace ignition::sensors;

Expand Down
12 changes: 6 additions & 6 deletions src/ImuSensor.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,16 @@
* limitations under the License.
*
*/

#include <ignition/msgs/imu.pb.h>

#include <ignition/common/Profiler.hh>
#include <ignition/plugin/Register.hh>
#include <ignition/sensors/Noise.hh>
#include <ignition/sensors/SensorFactory.hh>
#include <ignition/sensors/SensorTypes.hh>
#include <ignition/sensors/ImuSensor.hh>
#include <ignition/transport/Node.hh>

#include "ignition/sensors/Noise.hh"
#include "ignition/sensors/SensorFactory.hh"
#include "ignition/sensors/SensorTypes.hh"
#include "ignition/sensors/ImuSensor.hh"

using namespace ignition;
using namespace sensors;

Expand Down
12 changes: 6 additions & 6 deletions src/Lidar.cc
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,15 @@
#include <ignition/common/Console.hh>
#include <ignition/common/Event.hh>
#include <ignition/common/Profiler.hh>
#include <ignition/plugin/Register.hh>
#include <ignition/sensors/Lidar.hh>
#include <ignition/sensors/Noise.hh>
#include <ignition/sensors/SensorFactory.hh>
#include <ignition/sensors/SensorTypes.hh>
#include <ignition/sensors/GaussianNoiseModel.hh>
#include <ignition/transport/Node.hh>
#include <sdf/Lidar.hh>

#include "ignition/sensors/Lidar.hh"
#include "ignition/sensors/Noise.hh"
#include "ignition/sensors/SensorFactory.hh"
#include "ignition/sensors/SensorTypes.hh"
#include "ignition/sensors/GaussianNoiseModel.hh"

using namespace ignition::sensors;

/// \brief Private data for Lidar class
Expand Down
2 changes: 2 additions & 0 deletions src/LogicalCameraSensor.cc
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
#include <ignition/math/Frustum.hh>
#include <ignition/math/Helpers.hh>

#include <ignition/plugin/Register.hh>

#include "ignition/sensors/SensorFactory.hh"
#include "ignition/sensors/LogicalCameraSensor.hh"

Expand Down
11 changes: 6 additions & 5 deletions src/MagnetometerSensor.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,16 @@
*/

#include <ignition/msgs/magnetometer.pb.h>

#include <ignition/common/Profiler.hh>
#include <ignition/plugin/Register.hh>
#include <ignition/sensors/Noise.hh>
#include <ignition/sensors/SensorFactory.hh>
#include <ignition/sensors/SensorTypes.hh>
#include <ignition/sensors/MagnetometerSensor.hh>
#include <ignition/transport/Node.hh>
#include <sdf/Magnetometer.hh>

#include "ignition/sensors/Noise.hh"
#include "ignition/sensors/SensorFactory.hh"
#include "ignition/sensors/SensorTypes.hh"
#include "ignition/sensors/MagnetometerSensor.hh"

using namespace ignition;
using namespace sensors;

Expand Down
14 changes: 12 additions & 2 deletions src/Manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,7 @@
#include "ignition/sensors/Manager.hh"
#include <memory>
#include <unordered_map>
#include <ignition/common/PluginLoader.hh>
#include <ignition/common/Plugin.hh>
#include <ignition/plugin/Loader.hh>
#include <ignition/common/Profiler.hh>
#include <ignition/common/SystemPaths.hh>
#include <ignition/common/Console.hh>
Expand Down Expand Up @@ -101,6 +100,17 @@ void Manager::RunOnce(const ignition::common::Time &_time, bool _force)
}
}

/////////////////////////////////////////////////
ignition::sensors::SensorId Manager::AddSensor(
std::unique_ptr<sensors::Sensor> _sensor)
{
if (!_sensor)
return NO_SENSOR;
SensorId id = _sensor->Id();
this->dataPtr->sensors[id] = std::move(_sensor);
return id;
}

/////////////////////////////////////////////////
ignition::sensors::SensorId Manager::CreateSensor(const sdf::Sensor &_sdf)
{
Expand Down
2 changes: 2 additions & 0 deletions src/RgbdCameraSensor.cc
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
#include <ignition/common/Profiler.hh>
#include <ignition/math/Helpers.hh>

#include <ignition/plugin/Register.hh>

#include <ignition/rendering/Camera.hh>
#include <ignition/rendering/DepthCamera.hh>
#include <ignition/transport/Node.hh>
Expand Down
16 changes: 10 additions & 6 deletions src/SensorFactory.cc
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
*
*/

#include <ignition/common/PluginLoader.hh>
#include <ignition/common/SystemPaths.hh>
#include <ignition/common/Console.hh>
#include "ignition/sensors/config.hh"
Expand All @@ -35,7 +34,7 @@ class ignition::sensors::SensorFactoryPrivate
public: ignition::common::SystemPaths systemPaths;

/// \brief For loading plugins
public: ignition::common::PluginLoader pluginLoader;
public: ignition::plugin::Loader pluginLoader;
};

using namespace ignition;
Expand Down Expand Up @@ -75,17 +74,22 @@ std::shared_ptr<SensorPlugin> SensorFactory::LoadSensorPlugin(
return std::shared_ptr<SensorPlugin>();
}

auto pluginNames = this->dataPtr->pluginLoader.LoadLibrary(fullPath);
auto pluginNames = this->dataPtr->pluginLoader.LoadLib(fullPath);

if (pluginNames.empty())
{
ignerr << "Unable to load sensor plugin file for [" << fullPath << "]\n";
return std::shared_ptr<SensorPlugin>();
}

// Assume the first plugin is the one we're interested in
std::string pluginName = *(pluginNames.begin());
// Assume the last plugin is the one we're interested in
ahcorde marked this conversation as resolved.
Show resolved Hide resolved
std::string pluginName = "";
for (auto name : pluginNames)
{
pluginName = name;
}

common::PluginPtr pluginPtr =
plugin::PluginPtr pluginPtr =
this->dataPtr->pluginLoader.Instantiate(pluginName);

auto sensorPlugin = pluginPtr->QueryInterfaceSharedPtr<
Expand Down
Loading