Emplace API for factories

Once the new emplace API has been implemented for the whole wolf nodes, now, we have some weird and confusing things that we should discuss.

Let's remember how we defined the functionalities:

• emplace = create + link
• link = store the parent node pointer and add a pointer of this to the parent's list of children.

Now, in wolf, the nodes should be rarely created, they should be emplaced if possible (meaning, the parent pointer is known at the time of creating the node).

Factories

We have factories for sensors, processors and landmarks. These factories call create(), a static function that should be implemented in each non-abstract derived class (it is normally a make_shared() call and few things more). So, the first question is:

Should we change to emplace() factories?

In my opinion, it would have sense to rename all create() by emplace() and in its implementation, move from make_shared() to emplace(). It would be coherent, simple and easy to change.

ProcessorBase::configure(sensor_ptr)

In problem, there are 2 functions (with several variants): installSensor() and installProcessor(). They basically call the corresponding factory and then (since the factory just creates) call link() for the created sensor or processor. If we move to emplacing factories, we should remove this link(). However, in installProcessor(), apart from that, there a call to a pure virtual function ProcessorBase::configure(sensor_ptr). The second question is:

Which is the sense of this function? Could we remove it?

Since I don't know the sense of this function, I really don't know if it can be removed. If we keep it, I would definitely call it inside the ProcessorBase::emplace() function, along with the call to link().

These two proposed changes would make the whole API more coherent and would avoid confusions. The final objective is trying to avoid make_shared() within the whole wolf code.

added Agreement required label

I imagine that a useful functionality for any wolf node would be storing a casted pointer to its parent. For this, maybe we could add a virtual linkDerived() in all base classes called inside link() that is implemented in each of the corresponding base classes.

changed title from Processor factory confusing API to Emplace API for factories

I've done a quick search for the configure(sensor_ptr) function, and in most places it seems to be empty. I've found an example of non-empty configuration at processor_tracker_feature_trifocal.cpp in the vision plugin:

void ProcessorTrackerFeatureTrifocal::configure(SensorBasePtr _sensor)
{
    _sensor->setNoiseStd(Vector2s::Ones() * params_tracker_feature_trifocal_->pixel_noise_std);
}

This could be done in linkDerived() as well. Actually, I do not imagine the case in which something that can be done now in configure(sensor_ptr) couldn't be done in linkDerived(sensor_ptr).

I forgot this issue when talking about emplace API in the meeting. @jsola, can take a look and say if it's OK with you moving to emplacing factories?

There are two questions here:

1. Factories and emplace(): here, I don't quite understand the proposition. Is just a mere renaming of create() into emplace()? The functionality is not quite the same.

2. Processor::configure(sensor): thin function is to provide a means for the processor to pick up some parameters of the sensor. Which parameters, is undefined. The very reason for having this in a separate function is now unclear (I forgot), but at the time we did it it was not possible to do it otherwise.

There might be a number of examples. For example, the size of the image is a parameter of sensor, but it is used by the processor. It is not convenient to add this parameter as one of the processor parameters in e.g. a processor.yaml file, since the param is really a param of the sensor, used by the processor.

The specific parameters to be passed is only known at the derived level. This has to be taken into account if this configure() function wants to be replaced by another mechanism.

Personally, I do not like configure(), we included it only out of necessity, so I am in for any improvement.

1. Now, in installProcessor() and installSensor(), immediately after calling the create factory, we call link(). I propose to change the factory so that it emplaces the sensors and processors. So changing name and functionality.

2. With the new paradigm, this things can be done in the time of linking. So, I propose linkDerived() for these kind of things. Actually, for all the wolf classes (imagine you want to keep a casted pointer of your parent node).

Wouldn't it be interesting to duplicate installProcessor() & installSensor()? I mean we could have

• emplaceProcessor() & emplaceSensor() <- uses emplace()
• createProcessor() & createSensor() <- uses create()

The reason to propose this duplication is that, as far as I know, emplacing with a nullptr is not equivalent to just creating an object and I think it might be useful to mantain the ability to just create objects that may be floating around.

Emplacing with a nullptr and creating is exactly the same except for Factors (in which the landmark/frame/capture/feature_other will store the factor in the constrained_by_ list in case of emplacing). So, for processors and sensors, it is exactly the same.

In the other hand, the functionality of installSensor() and installProcessor() is emplacing a sensor and a processor in the wolf tree. Actually, the current implementation calls create (thru the factory) and link() just after.

We had a thread long ago on the keywords we want to use, and their specific meaning in wolf. A brief list follows, with some dummy explanations:

• parent::add // no longer available
• child::link
• child::emplace // construct and link
• problem::install // construct, link, and configure

The difference between emplace and install is that emplace just constructs and object and links it, while install does the same but with a more complete objective of really letting it ready for wolf. We can eliminate this difference: in many cases, it is true that there is absolutely no difference, because there is no configure step really being done. However, I kind of like the "install" thing for sensors and processors, because the word is very intuitive of what is going to happen, unlike "emplace" that really no normal human being knows what it means. We took emplace from the STL library of vectors, lists, maps, etc, which means 'construct and add to container in one go'.

On the other hand, emplacing with a nullptr is kind of a counter-sense. Of course it can be done, but it is like cheating the emplace() intended behavior.

create() on its own was useful up to now because after that we issued an add(). With the new API, this would require a link(). The thing is that the linking was performed by a base class, and just the create was left to the derived class which is actually the one that can create the derived object.

If we switch to just emplace(), then this emplace has to be implemented in the derived class.

I have no particular preference for this at this point for the case of the create functions that are no related to factories. But it seems to me that, if we have factories, these should only create objects, not emplace them, and so a create method for sensors, processors and landmarks seems still necessary. No?

There are two different and independent debates. I try to make clear what I propose and why.

1. Emplace vs. install
The difference of install and emplace is quite subjective... I mean, emplacing a wolf node can be defined as "letting it ready for wolf" (in your words) which for most of the nodes is just creating and emplacing, and in sensors and processors can be something more (I think it doesn't, but anyway).

I am not proposing renaming the "install" word which I think is very pertinent and intuitive.

I am not proposing changing the functionality of "install" which is now emplacing (with a configuration normally empty).

I propose to remove configure() and include this functionality in a new virtual linkDerived() also useful for other types of wolf nodes.

2. Emplacing factories
In general, in wolf we do not expect users to just create (without linking) wolf nodes. That's why we provide emplace() and not create(). We allow creation without linking (via make_shared or emplace(nullptr)) for testing purposes and some cases in the processors.

In the specific case of factories, sensor and processor factories are used in the installers (so, emplacing, not just creating) and landmark factory is used while loading a map (so emplacing landmarks, not creating). Of course, exceptionally, factories can be used in tests just for creation, but only in tests. I think that "cheating" the emplace() is not that bad in this exceptional case.

I just propose changing 2 lines of code, for example in SensorOdom2D would be

SensorOdom2D::create(...)
{
    ... 
    std::make_shared<SensorOdom2D>(...);
    ...
}

by

SensorOdom2D::emplace(hardware_ptr, ...)
{
    ... 
    SensorBase::emplace<SensorOdom2D>(hardware_ptr, ...);
    ...
}

And the corresponding changes in the factory registration and query.

Pros:

• The code in Problem::installXXX() is coherent with what we expect the developers to do: emplace.
• Coherence: By default, nodes in wolf are emplaced. Accordingly, we provide emplace() and emplacing factories.

Conts:

• In the tests, if someone wants to use a factory for creation, the factory should be used "cheating" it with a nullptr.

Overall this is a very good design. I am still concerned about new users, both mere users and contributors to WOLF.

• For the first, mere users, all this discussion is mostly irrelevant, since we propose our Problem::autoSetup() method and the (still unclear to me) ROS callbacks that should mostly make things super easy. Mere users will write YAML files and ROS nodes, with the callbacks being simplified already by some kind of helper provided by WOLF somewhere.

• For the second, contributors, I worry about WOLF being too complicated, and the whole point of this discussion is to make life easier. This is why I worry about, for example, the name of functions, but also the amount of 'features' that we incorporate. In a way, I would like to remove features rather than adding them. So the idea of having a general way of creating/linking nodes is appealing to me, and a good design of this feature is crucial for its success.

Overall, the design @joanvallve is proposing is very good. Simple and general. But in view of my comments above, I would introduce the following:

• In my opinion, the keyword 'emplace' is not very nice. It makes sense to people used to use the STL::emplace functions for std::vector, std::map, list, etc. But to other more beginners it might seem odd. Since the only alternative that comes to my mind is "install", and it is not super good either for nodes other than sensor and processor, let us accept "emplace".
• For similar reasons, linkDerived makes sense to WOLF developers that programmed the core, since link is used there. But for other users there is no recall to anything familiar. In this sense, I would like to have a better name, and hence my preference for configure. This name can perfectly apply to all nodes of WOLF: we can configure anything, not only processors.
• I would like to provide the capability of also creating floating nodes, just for the sake of simplicity at the time of programming tests. The fact that we advanced like this during all this time is maybe an indication that, for programmers, this is a good feature to have.

I think, however, that regarding the create/emplace/configure discussion we are being a little biased towards decisions of the past. Let us see the options we have (not discussing about names now):

• Option 1: with systematic pointers to parent.

  • Node constructors systematically accept the parent's pointer. They configure the child.
  • Node creators systematically accept the parent's pointer. They configure the child through the constructor. They do not touch the wolf tree.
  • Node emplacers systematically accept the parent's pointer. They configure the child through the constructor. They also do other things related to the wolf tree.
  • There is no need for configure or linkDerived.

• Option 2: without pointers to parent

  • Node constructors cannot configure the node with info from parent.
  • Node creators with no pointer to parent cannot configure the node with info from parent.
  • Node creators with pointer to parent can configure the node, but they require a ChildDerived::configure method to configure the child. It is unclear whether creators should or should not configure the child -- the API just says "create a child".
  • Node emplacers will definitely configure the child, and ChildDerived::configure will be needed.

So I wonder, which one is better? If we opt for Option 2 (we are almost there), then we better make clear what is the scope of each level: constructor, creator, emplacer.

Finally, regarding the design of all this, in !313 (merged) and #248 (closed) I am proposing a couple of macros that will make the life of contributors easy. They current implement the factory methods create and createAutoconf for sensors and processors. This deserves the following comments:

• create can be migrated to emplace should we avoid mere creation as in @joanvallve suggestion.
• create can co-exist with emplace in factories, and really only emplace should be implemented in classes: in the factory, create calls emplace with a nullptr to parent, and returns the object pointer (preferred) // or // emplace calls create, then configures it and links it to the wolf tree (not preferred).
• createAutoconf can be renamed to create (the API is different), then the two comments above apply.
• The old create methods can eventually disappear if we end up working only with the param server. This will impact a few parts of wolf, but overall a lot of gtests and demos. Currently, YAML files are bing parsed by two methods (ParamParserYAML and ParamsXxxFactory), and this is very dangerous: now, both parsers do not necessarily agree on the same YAML structure!!

mentioned in issue #248 (closed)

We agree on that the main problem is for developers since for users everything is encapsulated in the auto-configuration.

Also, we all worry about WOLF being too complicated for new developers. Function names are very important but as you said, also the amount of features that we provide/ask for new developers to understand and familiarize with.

My key point in this issue is: why do we have to provide create factories?

I think we shouldn't. We should move to emplace factories. Let me argue this and reply to your comments at the same time.

1. We already have emplace() I agree that it is a not so nice keyword. However, we already agreed on emplace = create + link (if we find a better name, I'm open to change it of course). In WOLF, we provide emplace() functionality as the main way to create (and link) a node. If you want to just create a node, you have to use make_shared. So, the developer has to be already familiarized with emplace (both, name and functionality). So we can only gain simplicity by removing create.
2. Less errors: This was the main reason to switch to the new emplace API. We do not want to rely on the developers to remember to link after create. Whoever is going to play a little with WOLF, will have to learn to emplace nodes. If we remove creators, there won't be any confusion: You cannot either create or emplace, we just emplace.
3. We want to install: At the end of the day, the factories are made for installing sensors and processors, and emplacing landmarks. Why do we want factories that only create? Wherever a create factory is called in wolf, linking is done afterwards.
4. Floating nodes: We still allow make_shared, this functionality is not forbidden at all. We are just talking about factories (processors,sensors and landmarks). How many tests are calling directly a factory instead of installXXX()?
5. Coherence: The rest of wolf nodes do not have create.

If we move from create to emplace factories, the discussion about option 1 or 2 is simplified to:

"Should the constructors systematically take the parent pointer?".

We would only have 2 levels: constructor, emplacer (either the factory or the base class function). Except for factors, we are almost in option 2: the constructors do not have the pointer of the parent node. Then configure() is needed. I think this is a better option since if we want to allow a floating node, requiring the parent in the constructor has no sense. Also, we avoid the potential error of developers trying to link the the node in its constructor (the origin of the whole issue ).

I agree on the autoConf factories having the same name. I propose emplaceSensor(), emplaceProcessor() and emplaceLandmark() (just emplace() is used by the emplacers implemented in the base classes).

About the old factories, I also agree on removing them (not urgent). It doesn't apply to the Landmark factory, right?

Just a minor comment, regarding the emplacing with a nullptr: make_shared is not equivalent to emplace with nullptr because in order to create a processor or sensor we must first create the params object. Therefore, simulating emplace with a nullptr via make_shared would be something like:

auto params = std::make_shared<ProcessorParamsOdom2D>(_unique_name, _server);
auto prc_ptr = std::make_shared<ProcessorOdom2D>(params);

There is also this other possibility, used many times in gtests:

auto params = std::make_shared<ProcessorParamsOdom2D>();
params->field1 = so so;
params->field2 = so what;
auto prc_ptr = std::make_shared<ProcessorOdom2D>(params);

OK we'll have to agree on this. @joanvallve we already gave the reasons for creating nodes. I think we all now understand each other's arguments so the thing now is to get to an agreement.

For the time being, I am about to merge the following related MR (PEASE CONSIDER THAT YOUR APPLICATIONS, DEMOS, ROS NODES, MAY STOP WORKING DUE TO SOME YAML CHANGES -- SEE BELOW):

• core: 248-processor-constructors-and-creators-requiring-a-sensor-pointer
• other plugins: remove-sensor-from-processor-creators

there's a lot of work done there and I do not want these branches to live separated from devel for long, otherwise we'll have a lot of conflicts in the near future.

These do the following in core and plugins:

• remove SensorXxxPtr from every constructor of ProcessorXxx.
• Implement the (sometimes missing) constructors SensorXx(const VectorXs&, IntrinsicsXxxPtr) and ProcessorXx(ProcessorParamsXxxPtr)
• Implement macros WOLF_SENSOR_CREATE and WOLF_PROCESSOR_CREATE which implement the create and createAutoconf factory methods.
• Remove all explicit definitions of create and createAutoconf and use the macro instead
• Make YAML parsers through parser or through ParamsFactory consistent with the same YAML structure
• Make all YAML parameters use underscore and not space as word separator.
• Write some missing gtests related to the above.

After this merge, changing the whole thing to emplace will be easy as all the code now is more centralized and uniform, and easier to find.

Before merging this MR, however, it'd be nice to have a little improvement we discussed yesterday. It relates to the error messages displayed when something goes wrong with YAML parsing.

@jcasals if you think this feature can be done today, I'll wait a few hours. I do not say this to give pressure, don't worry.

If this YAML error messaging is not ready today, then I want to make this biig merge today, because now I take a few days off and I'll forget everything at my return.

@joanvallve About LandmarkFactories, I have not thought about it. What is the special issue, if any, in the case of landmarks?