-
Notifications
You must be signed in to change notification settings - Fork 270
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
Use the symbols if they are already loaded #304
Conversation
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
…ionrobotics/ign-gazebo into ahcorde/replace/pluginload
Signed-off-by: ahcorde <[email protected]>
this->sensorFactory.CreateSensor< | ||
sensors::AirPressureSensor>(data); | ||
std::make_unique<sensors::AirPressureSensor>(); | ||
sensor->Load(data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking at the CreateSensor function, it also makes a sensor->Init()
call in addition to sensor->Load
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added init() method 3f42c00
src/systems/sensors/Sensors.cc
Outdated
cameraSensor->Load(_sdf); | ||
sensorId = this->dataPtr->sensorManager.AddSensor(std::move(cameraSensor)); | ||
} | ||
else if (_sdf.Type() == sdf::SensorType::THERMAL_CAMERA) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you also need to do the same for depth and rgbd camera? or do they work with CreateSensor
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
depth and rgbd camera classes are not being used, we don't need to link against them, we can use createSensor
. If need to modify some of the specific parameters, then we need to instanciate a class and create a specific case for them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can try sensors_demos.sdf
which is creating: camera
, depth_camera
, gpu_lidar
, rgbd_camera
and thermal_camera
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is now targeting Edifice, I think lines 485 - 504 can be replaced with:
ignition::sensors::SensorId sensorId = this->dataPtr->sensorManager.CreateSensor(_sdf);
At least, this worked for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took the liberty of applying this change in 2f674e6. Feel free to revert.
Signed-off-by: ahcorde <[email protected]>
@ahcorde This needs a bit of an update |
We're retargeting the upstream PR gazebosim/gz-sensors#38 to Ign-E, and this one as well. Removing from the Dome beta. |
src/SystemLoader.cc
Outdated
@@ -78,7 +78,11 @@ class ignition::gazebo::SystemLoaderPrivate | |||
return false; | |||
} | |||
|
|||
auto pluginName = *pluginNames.begin(); | |||
std::string pluginName = ""; | |||
for (auto name : pluginNames) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for (auto name : pluginNames) | |
for (const auto &name : pluginNames) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I applied this change in 2f674e6
src/systems/sensors/Sensors.cc
Outdated
cameraSensor->Load(_sdf); | ||
sensorId = this->dataPtr->sensorManager.AddSensor(std::move(cameraSensor)); | ||
} | ||
else if (_sdf.Type() == sdf::SensorType::THERMAL_CAMERA) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is now targeting Edifice, I think lines 485 - 504 can be replaced with:
ignition::sensors::SensorId sensorId = this->dataPtr->sensorManager.CreateSensor(_sdf);
At least, this worked for me.
Signed-off-by: Nate Koenig <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #304 +/- ##
==========================================
- Coverage 77.38% 77.35% -0.04%
==========================================
Files 213 213
Lines 11954 11966 +12
==========================================
+ Hits 9251 9256 +5
- Misses 2703 2710 +7
Continue to review full report at Codecov.
|
With the proposal on gazebosim/gz-plugin#35, I think this PR isn't necessary. |
Removing beta label, we won't have time to wrap this up before code freeze. Let's retarget at Ignition-F. |
Superseded by #617 |
This PR is part of this other PR gazebosim/gz-sensors#38 and it's related with this issue gazebosim/gz-sensors#18
The symbols for some sensors are already loaded when the ignition::gazebo::systems::Sensors is loaded (In particular Camera and ThermalCamera). Instead of creating the sensor using the Manager class (which needs to reload the library which is already loaded by the system Sensor, it's generating some issue.) I instantiated the class and add it to the Manager.
Signed-off-by: ahcorde [email protected]