Replace ignition_sensors list param with ignition field

[efernandez]

This PR removes the ignition_sensors list param and replaces it with an ignition ROS param field for the fuse_models::Unicycle2DIgnition sensor model. It also adds a virtual bool ignition() const method fo the base SensorModel class, so the fuse_optimizer nodes can rely on that to implement the ignition logic.

Some minor cleanup changes are also included in this PR.

[efernandez]

@svwilliams @ayrton04 for review

[svwilliams]

I'd like to propose an alternative approach and see how you feel about it.

First, I really like how your proposed solution cleans up a bunch of the code, converting annoying searches of collections of strings into a direct property lookup.

However, I feel like the decision to use a specific sensor model as an ignition source is really a system-level design question, and not something that is an intrinsic property of the sensor itself. But I do acknowledge that is not a completely true statement, as the existence of the Unicycle2DIgnition sensor model proves. If all sensors were designed with an ignition property controlled by the parameter server, I would feel better about it. Or at least all sensors derived from some reasonable base class.

But as an alternative, what if we make the ignition property a boolean parameter that is part of the optimizer config, in the same way that each sensor specifies the set of motion models to use:

sensor_models:
  sensor1:
    type: my_package::Sensor1Model
    motion_models: ['unicycle']
    ignition: true
  sensor2:
    type: my_package::Sensor2Model
    motion_models: ['unicycle']
    ignition: false

And we could make the ignition parameter optional and default to false.

Then, over in the Optimizer class, we could make a struct to hold the sensor model and the ignition parameter together:
https://github.com/locusrobotics/fuse/blob/devel/fuse_optimizers/include/fuse_optimizers/optimizer.h#L123

struct SensorModelInfo
{
  SensorModelUniquePtr model;
  bool ignition;
}
using SensorModels = std::unordered_map<std::string, SensorModelInfo>;

(only all the names should be far more clever).

That should allow the same type of direct lookup inside of the optimizer code:

if (sensor_models_.at(sensor_name).ignition)

without all of that annoying searching through a sorted list of names.

And if we were really ambitious, we could include the motion models in that structure as well, eliminating the need for the associated_motion_models_ variable.
https://github.com/locusrobotics/fuse/blob/devel/fuse_optimizers/include/fuse_optimizers/optimizer.h#L129

Would that solution fit your needs? Or is there a reason you think ignition-ness should be defined in the sensor properties and not in the optimizer/system properties?

[efernandez]

Would that solution fit your needs? Or is there a reason you think ignition-ness should be defined in the sensor properties and not in the optimizer/system properties?

Yes, I think that would also work for me. I can try to implement that and give it a try over the weekend.

Indeed, your proposal has the benefit that existing ignition sensor models don't have to change, i.e. they don't have to implement the ignition() method.

[efernandez]

@svwilliams Updated with your proposed approach.

[svwilliams]

Can you get this rebased on devel. I'm guessing something in a recent merge created a conflict.

[efernandez]

Rebased and fixed conflicts.