Disambiguate KeyFrame --> EstimatedFrame to allow for estimated non-keyframes

added To Do (release) label

According to the aim of this issue explained by @jsola, I wonder if we have 2 different aspects (key/non-key and estimated/non-estimated) or if we have just 3 different types of frames:

• key & estimated
• non-key & estimated
• non-key & non-estimated

Since key & non-estimated is nonsense..?

I would say non-estimated keyframe has no sense in practice...

Ok, then, I think that we can keep the FrameType enum but redefining it in these 3 types. They could be named:

• KEY_ESTIMATED
• ESTIMATED
• NON_ESTIMATED

Also, we will have to provide the machinery for checking this type. I would say that, now, isKey() and setKey() are used in the sense of isEstimated() and setEstimated(), respectively. I propose to first rename them accordingly everywere in the code, and afterwards we can add new functions isKey() and setKey(), to be used whenever we decide to differentiate between key and non-key frames.

Yes, I agree to rename to isEstimated etc, then code the new isKey etc.

Analogously, the rest of code related with key frames should be renamed to estimated. It means:

In Problem:

• getLastKeyFrame()
• closestKeyFrameToTimeStamp()
• permitKeyFrame()
• keyFrameCallback()
• getLastKeyFrameCovariance()

In TrajectoryBase:

• getLastKeyFrame()
• findLastKeyFrame()
• closestKeyFrameToTimeStamp()
• setLastKeyFrame()

In ProcessorBase and derived ones:

• class PackKeyFrame
• voteForKeyFrame()
• permittedKeyFrame()
• keyFrameCallback()

Maybe we should use EstFrame instead of EstimatedFrame in these functions?

mentioned in merge request !266 (merged)

added ~209 label and removed To Do (release) label

assigned to @joanvallve

After a discussion with @jsola, some conclusions:

• From the Processor perspective, key frames will continue being key frames (important frames that will remain and synchronized with other processors). Estimated frames are some auxiliar frames also estimated that helps some processors to estimate the "recent past".
• From the rest of wolf architecture, now key frames is used mostly as a synonym of estimated frame.

Then, this is not that straightforward. For now, it will be better not to use KEY to be clear. So, FrameType enum could be renamed:

• IMPORTANT
• ESTIMATED
• NON_ESTIMATED

For processors, new IMPORTANT frames require synchronization with all processors while new ESTIMATED frames require only synchronization with ProcessorMotion. So, current Key functions will be renamed as Important and analogous functions for Estimated will be implemented.

Ok, finally, I have pushed some work in !266 (merged). The proposed changes are the following:

FrameType enum contains 3 types, the first two are estimated, meaning, notified to the solver, part of the problem (fixed or unfixed):

• KEY
• AUXILIARY
• NON_ESTIMATED

In FrameBase there are the following new methods (along with the existing isKey() and setKey()):

• bool isAuxiliary() const
• bool isEstimated() const it means it is either KEY or AUXILIARY
• void setAuxiliary()

In Problem there are the following new methods:

• FrameBasePtr getLastEstimatedFrame( ) const analogous to the existing getLastKeyFrame()
• FrameBasePtr closestEstimatedFrameToTimeStamp(const TimeStamp& _ts) const analogous to the existing closestKeyFrameToTimeStamp(_ts)
• bool permitAuxFrame(ProcessorBasePtr _processor_ptr) analogous to the existing permitKeyFrame(_processor_ptr), it just returns true
• void auxFrameCallback(FrameBasePtr _frame_ptr, ProcessorBasePtr _processor_ptr, const Scalar& _time_tolerance) analogous to the existing keyFrameCallback(_frame_ptr, _processor_ptr, _time_tolerance) it notifies that a new AUXILIARY frame has been created but just to the Problem::processor_motion_ (the first ProcessorMotion that has been installed).
• bool getLastEstimatedFrameCovariance(Eigen::MatrixXs& _covariance) analogous to the existing getLastKeyFrameCovariance(_covariance)

The new parameter voting_aux_active has been added to the ProcessorParamsBase and added to its constructor without breaking the API:

ProcessorParamsBase(bool _voting_active, Scalar _time_tolerance, bool _voting_aux_active = false)

In ProcessorBase, new methods have been added:

• bool isVotingAuxActive() const getter
• void setVotingAuxActive(bool voting_active) setter
• virtual bool voteForAuxFrame() which returns false (to be inherited by whichever processor that wants to vote for AUXILIARY frames)
• virtual bool permittedAuxFrame() final analogous to the existing permittedKeyFrame()

As @jsola pointed out, using the word "Estimated" to refer to both AUXILIARY and KEY frames may be a little misleading. If the frame is fixed, it wont be actually being estimated by the solver.

Have you an alternative, I will replace the corresponding functions names.

One solution is siply to NOT IMPLEMENT isEstimated(), since it's just a isKey() || isAuxiliary() it can be left as is and require users to always call both isXx().

Another solution is to implement precisely isKeyOrAuxiliary(), with an obvious implementation, acting just as a shortcut.

Finally, being this method just exactly this, I do not see why a different name would be required. It would just require more documentation and lead to confusion. So solution 2 is perfect from my view.

The complement for these functions in the case of Estimated / non estimated is:

isFixed()

It comes from an intricate implementation sarting at getStatus(). Maybe these little methods could be improved to make their implementation cinsistent with the changes in this issue.

The isEstimated() is not a problem. Indeed, it can be just not implemented.

However, FrameBasePtr getLastEstimatedFrame( ) , FrameBasePtr closestEstimatedFrameToTimeStamp(_ts) and bool getLastEstimatedFrameCovariance(_cov) have sense. They refer to the last "estimated" frame (or the closest to a timestamp), you do not care about if it's KEY or AUXILIARY...

Maybe, FrameBasePtr getLastKeyOrAuxFrame( ) , FrameBasePtr closestKeyOrAuxFrameToTimeStamp(_ts) and bool getLastKeyOrAuxFrameCovariance(_cov) is a good alternative.

I don't understand your second comment..

MR !266 (merged) is ready to be merged with the proposed changes.

closed via merge request !266 (merged)

mentioned in commit 13b3b9e8