WOLF API change: REMOVING add...() and add...By() functions

The wolf API requires too much functions to be called in order to properly create and link a node in the wolf tree. Now, the procedure has 2 or 3 steps:

1. Calling the constructor via make_shared
2. Adding it to its parent node
3. (only constraints) adding it to its "other" linked nodes.

This information is known at the time of constructing the node. However, calling shared_from_this is not allowed until the constructor ends, so the points 2 and 3 can't be performed inside the constructor.

Yesterdary with @jsola we found out that it is possible using a trick: https://stackoverflow.com/questions/33069674/is-this-trick-to-make-calling-shared-from-this-in-the-constructor-just-work

I've created gtest_shared_from_this to test and show how it works.

Using it, we could implement a new wolf API to do all the steps inside the constructor of the corresponding base classe and there would be no add functions (nor addConstrainedBy()).

What do you thing? @jsola @asantamaria, @artivis, @AtDinesh

changed the description

Well, as the title of this article states it, it is a trick, a tricky one.
First of all the object isn't fully formed so you cannot legally invoke any virtual methods doesn't sound so good. Maybe are we only 'linking' things together for now which means we are not immediately endangered by this caveat, but we may very be in the future. And by that time, we won't remember this particular trick and chase a nasty bug deep down.
Just like one of he answer to the SO question, I would rather prefer a factory or anything that is very explicit and would not jeopardize future developments.

Yes, indeed. Actually, the same guy says that it isn't a "trick" it is a-typical.

The point is, this "trick" will be made at base class level: CaptureBase, for example.

Our approach would be just calling _frame_ptr->addCapture(shared_from_this()). The restriction about not invoking any virtual methods applies just to FrameBase::addCapture(). So, we propose to shield all add functions as public virtual and final in the base classes in order to assure that nobody is deriving them and inside them there is only pointer storing (capture_list_.push_back(_capture_ptr)).

I think it is not jeopardizing future developments.

On the other hand, a factory for constraints, for example, seems impossible to me.

There are several ways to create nodes and link them in wolf:

1. Direct constructor (with make_shared) + addNode() + addConstrainedBy()
2. Use createXxx() in Processors, + addNode() + addConstrainedBy()
3. Use emplaceXxx() in Processors. In most cases this adds all the links
4. Use installXxx() in Problem (only for Sensors and Processors)
5. Use Problem::create(...) (only for Problem)
6. JoanV's trick

Only 4. uses a true factory.

However, 2. and 3. can be considered pseudo-factory methods in the sense that the API is common and the derived class has to implement the final code.

In methods 1. to 3. , having to remember to type in addNode() and addConstrainedBy(), especially this second one, is a bad design: they are by no means obvious, especially the second. Besides, many constraints require more than one ConstrainedBy, so the problem is serious.

Method 5. is a better way to proceed: we make the constructor private, forbid copy and default constructors, and then type in a static method create(...) which will call the constructor and then add the links. If we extended to all nodes, this method create would need to be coded alongside the constructor in all derived Node classes, since the API of the nodes is so diverse.

Joan Vallve's trick 6. is a new way of doing it, similar to 1. but including the links. We found it interesting, but also noticed that the trick is dangerous. Some comments on this:

• During the discussion yesterday, I proposed to go the 5. way, but then we found this trick and tried it to success.

• In our implementation the tricky code is placed in the base class, plus the linking methods addXxx() are declared final. In any case, the pointers to link are always pointers to the base class, so by the time the base class constructor is done, this pointer exists. With this and the final mark on the ONLY methods that will be executed during construction time, it seems to us (maybe we need to think more about this) that there is no possibility for the end user to call any virtual methods before abandoning the call to the constructor.

In conclusion, 6. is tricky and I tend to agree with Jeremie, although we have some safety margin as explained above. Also, 5. is appealing to me, and perfectly safe, at the price of having to write a constructor and a creator for each derived Node class.

How would we force derived classes to have private constructors in 5? How would we assure that the linking is correctly implemented? The aim of 6 is dealing with wolf connections automatically and transparent to the end user, right?

BTW many classes already have the method create. However, this method has been thought for the Factories that already exist (Sensors, Processors, Params, Frames and Landmarks), and at this time is only calling the constructor, and not linking.

Should we require linking too, we have two possibilities:

1. Create another method emplace() which calls create() and then links.
2. rename create --> emplace, add the linking, and then re-think the role of the factories to act as creator+linker instead of the present role of simple creators.

Method 1., as you can imagine, means having for each class: a constructor, a creator, and an emplacer. I think this is too much.

Method 2., demands re-thinking over an old design. There might be hidden implications --> headache and 1e16 bugs to solve.

JoanV, regarding your questions:

• How would we force derived classes to have private constructors in 5?

This needs to be documented and the user has to comply with some rules. the design pattern is that of Problem. We could also create a Macro, maybe.

• How would we assure that the linking is correctly implemented?

I imagine the linking could be made with a call to some function in the base class, so just do link(void) and go. See example code in https://gitlab.iri.upc.edu/mobile_robotics/wolf/issues/166#note_3038

• The aim of 6 is dealing with wolf connections automatically and transparent to the end user, right?

YES. We are interested in methods that do not demand extra care by the end user.

Indeed, the trick 6. does not need anything from the user, just provide all the necessary pointers in the constructor so that all links can be established. This is obvious, and so there is not much to document.

Question: could we come up with a safe design that is simple and accomplishes this? (and not resorting to an external library)?

Finally, there is a final way to proceed. It demands no extra code in the derived classes, but requires the call to two functions:

1. Instead of only one pointer for other nodes, have lists of pointers to allow any number of them (this is a related issue that we also discussed yesterday).

2. add this method to base nodes:

virtual BaseNode::link()
{
  parent->addNode(shared_from_this()); 
  for (auto sensor : sensorOtherList)
    sensor->addConstrinaedBy(shared_from_this());
  for (auto frame : frameOtherList)
    frame->addConstrinaedBy(shared_from_this());
  for (auto capture : caputreOtherList)
    capture->addConstrinaedBy(shared_from_this());
  for (auto feature : featureOtherList)
    feature->addConstrinaedBy(shared_from_this());
  for (auto landmark : landmarkOtherList)
    landmark->addConstrinaedBy(shared_from_this());
}

3. Each time a node is constructed, call DerivedNode::link() just afterwards. Grouping these two calls is actually what we are hoping to do, but at least this way the linking code is in the base class and valid for all nodes (that's why the lists to be 100% generic).

Ouf my brain... OK there is still another way:

1. Have Parent::addNode(Child) establish all the links, also those requiring ConstrainedBy:

ChildBasePtr BaseNode::addNode(ChildBasePtr child)
{
  child_list.push_back(child);
  // child->setCapturePtr(shared_from_this()); // this should be done in child's constructor
  child->setProblem(getProblem());

  // only if child is constraint:
  for (auto sensor : child->sensorOtherList)
    sensor->addConstrinaedBy(child);
  for (auto frame : child->frameOtherList)
    frame->addConstrinaedBy(child);
  for (auto capture : child->caputreOtherList)
    capture->addConstrinaedBy(child);
  for (auto feature : child->featureOtherList)
    feature->addConstrinaedBy(child);
  for (auto landmark : child->landmarkOtherList)
    landmark->addConstrinaedBy(child);

  return child;
}

2. Use ChildDerived's constructor + Parent->addNode(), but forget about the addConstrainedBy() calls.

This is indeed the same method as above, just that link() is named addNode(). The important difference is that link() belongs to Child, while addNode() belongs to Parent. This is minor. But the advantage of this last design is that it matches the current WOLF logic and reuses the code, so the implementation is muuuuch easier --> less recoding.

This is only the constraint case.. I don't know why the the ConstrainedBy functions should be called in FeatureBase::addConstraint() instead of in either ConstraintBase::link() or the constructor ConstraintBase::ConstraintBase() (depeding on which solution we take).

I am only providing alternatives to think them over.

And yes, well, if it were not for the constraints case, would we be discussing this at all? The other cases are trivial and solved with a simple addNode(), which OK needs two calls constructor+addNode(), but they seem logical, easy to explain and difficult to forget. The issue with constraints is that it is easy to forget all calls to ConstrainedBy(), because they are not obvious.

OK @joanvallve , I think the trick is OK and can be implemented and pushed. It provides a lot of advantages and a drawback that we have identified and protected.

@artivis , would you want to give a last word or suggestion?

Nothing more to add .

OK May I raise a second topic related to this issue? May I open a new issue?

It's this: are we ever going to disconnect a node and reconnect it somewhere else? Are we ever going to modify the pointers once a node is inserted in WOLF?

For example when receiving a KF callback, maybe we need to move some things. Maybe an existing Capture needs to be associated to a KF, maybe.

If so, then we need to keep the possibility to get/set the pointers, that is, keep all getters and setters public.

@joanvallve we raised this topic briefly yesterday, and said the setters should be avoided or at least make them private/protected. Have you thought about it?

I think it can be easily done by creating a new node and removing the other. It may not be the easy way to do it but I think it will be hardly the case in our applications... So, if avoiding this functionality the code gets significantly simpler (which is the case I think), it is worth avoiding it.

assigned to @jcasals

It is being implemented !256 (merged)

closed

changed milestone to %Emplace API

reopened

added ~209 label and removed Opinions wanted label

split in #193 (closed) and #191 (closed)

closed