New processors workflow

Last meeting agreements:

In processors:

• Remove ProcessorBase::process(capture)
• Add virtual ProcessorBase::run() = 0. All derived processors current process(capture) should move to run().
• Add virtual bool ProcessorBase::triggerInCapture(capture) = 0 implemented in ProcessorTracker and ProcessorLoopClosure.
• Add virtual bool ProcessorBase::triggerInKF(frame) = 0 implemented in ProcessorTracker and ProcessorLoopClosure.

In SensorBase::process() call processor->captureCallback(capture) instead of processor->process(capture).

changed milestone to %IRI release

added To Do (release) label

created merge request !291 (merged) to address this issue

mentioned in merge request !291 (merged)

closed via merge request !291 (merged)

mentioned in commit b21a3bc5

reopened

added Agreement required ~209 labels and removed To Do (release) label

PROPOSED APPROACH
The MR !291 (merged) contains the following implementation.

There are 2 possible lines of execution, when a capture or a keyframe is notified via callback:
New capture:

1. void captureCallback(CaptureBasePtr) (public): Is the external API (called by SensorBase::process(CaptureBasePtr)). Implemented in ProcessorBase: it calls processCapture(CaptureBasePtr) if triggerInCapture(CaptureBasePtr) == true.
2. virtual bool triggerInCapture(CaptureBasePtr) (protected): Implemented in derived classes. It returns if processCapture(CaptureBasePtr) should be called.
3. virtual void processCapture(CaptureBasePtr) (protected): Implemented in derived classes. For those processors that process all or some captures, the algorithm should be here.

New key-frame:

1. void keyFrameCallback(FrameBasePtr, TimeStamp) (public): Is the external API (called by Problem::keyFrameCallback(...)). Implemented in ProcessorBase: it calls processKeyFrame(FrameBasePtr, TimeStamp) if triggerInKeyFrame(FrameBasePtr, TimeStamp) == true.
2. virtual bool triggerInKeyFrame(FrameBasePtr, TimeStamp) (protected): Implemented in derived classes. It returns if processKeyFrame(FrameBasePtr, TimeStamp) should be called.
3. virtual void processKeyFrame(FrameBasePtr, TimeStamp) (protected): Implemented in derived classes. For those processors that process all or some key-frames, the algorithm should be here.

Changes in existing processors:
Capture-based processors: ProcessorMotion, ProcessorTracker, ProcessorGnssFix, ProcessorGnssSingleDiff changes:

• process(CaptureBasePtr) was renamed to processCapture(CaptureBasePtr)
• new virtual bool triggerInCapture(CaptureBasePtr){return true;}
• new virtual bool triggerInKeyFrame(FrameBasePtr, TimeStamp){return false;}
• new virtual void processKeyFrame(FrameBasePtr, TimeStamp){} (not called)

Key-frame-based processors: ProcessorLoopClosure, ProcessorCaptureHolder changes:

• process(CaptureBasePtr) was modified to processKeyFrame(FrameBasePtr, TimeStamp)
• new virtual bool triggerInCapture(CaptureBasePtr){return false;}
• new virtual bool triggerInKeyFrame(FrameBasePtr, TimeStamp){return true;}
• new virtual void processCapture(CaptureBasePtr){} (not called)

OPEN ISSUES
Everything is working now. However, there is still a thing that I don't like. The implementation of ProcessorBase::captureCallback(CaptureBasePtr) is:

void ProcessorBase::captureCallback(CaptureBasePtr _capture_ptr)
{
    // if trigger, process directly without buffering
    if (triggerInCapture(_capture_ptr))
        processCapture(_capture_ptr);
    else
        buffer_capture_.add(_capture_ptr->getTimeStamp(), _capture_ptr);
}

And the implementation of ProcessorBase::keyFrameCallback(FrameBasePtr _keyframe_ptr, const Scalar& _time_tol_other) is:

void ProcessorBase::keyFrameCallback(FrameBasePtr _keyframe_ptr, const Scalar& _time_tol_other)
{
    // buffering anyway
    buffer_pack_kf_.add(_keyframe_ptr, _time_tol_other);

    // if trigger true -> processKeyFrame
    if (triggerInKeyFrame(_keyframe_ptr, _time_tol_other))
        processKeyFrame(_keyframe_ptr, _time_tol_other);
}

As you can see, they are not equivalent. All key-frames are stored in the buffer in all types of processors but the captures are not if triggerInCapture(...) is true. I imagine some cases in which maybe the storage and the processing of a capture/key-frame are independent choices. Maybe 2 new parameters in ParamsProcessorBase::store_captures and ParamsProcessorBase::store_keyframes would be better?

If I remember well, the meeting agreement was two new virtual functions analogously to triggerInXX:

• virtual bool storeKeyFrame(FrameBasePtr)
• virtual bool storeCapture(CaptureBasePtr)

and call them in the corresponding callback to know if the capture/keyframe should be stored.

removed Agreement required label

In captureCallback(...) if storeCapture(CaptureBasePtr) returns true, should the capture be stored regardless of triggerInCapture(...) returning true?

Yes, they should be independent.

Then, I think we should implement them by default in ProcessorTracker and ProcessorLoopClosure:

ProcessorTracker:

• storeCapture(CaptureBasePtr){ return false;}
• storeKeyFrame(FrameBasePtr){ return true;}

ProcessorLoopClosure:

• storeCapture(CaptureBasePtr){ return true;}
• storeKeyFrame(FrameBasePtr){ return true;}

What should be the values for ProcessorMotion & ProcessorCaptureHolder?

ProcessorMotion:

• storeCapture(CaptureBasePtr){ return false;}
• storeKeyFrame(FrameBasePtr){ return true;}

ProcessorCaptureHolder Actually, I think that this is a processor that implemented the capture buffer, now included in ProcessorBase. I don't know who did it, I am not sure it is still useful: #243 (closed)

• storeCapture(CaptureBasePtr){ return true;}
• storeKeyFrame(FrameBasePtr){ return false;}

The changes have been made in new-processor-workflow. Will be merged in a few days if there are no complaints.

@jcasals what's the status of this? Your last comment says it is ready to merge. If possible, just proceed with the merge.

Merged, closing.

closed