Merge Aux/KeyFrames into Estimated Frames

Meeting agreement

Remove all code related to Auxiliary frames.
2 different enums: KEY/REGULAR, ESTIMATED/NON_ESTIMATED.
Current functionalities (registering, etc) will change to check if frame is estimated (not key).

"Estimated" and "NotEstimated" are quite long for the respective functions (setEstimated(), setNotEstimated(), isEstimated()...). @jsola do you have any alternative?

following this #213 (comment 6887), it seems that this question becomes irrelevant no?

I don't think so. The issue you linked says "key" everywhere meaning "estimated". I think all three agreements listed before are still valid. The second requires a name for "estimated" and "not estimated".

I mean: now, all frames in Trajectory will be estimated, so what is the point of having a marker Estimated/NonEstimated?

If what you mean with setEstimated() is a method to:

Link the Frame which was in the Processor to the Trajectory
Set problem
Register its state blocks

then I would probably go for something like estimate() or addToProblem() or similar semantics, not necessarily setEstimated() which from the outside looks like a simple setter.

This discussion is quite difficult. Maybe we could re-discuss this once the other issue is implemented.

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

mentioned in merge request !403 (merged)

mentioned in issue #366 (closed)

Doing:

removing all related to Auxiliary frames
Rename enum NON_ESTIMATED --> REGULAR
Rename setNonEstimated() --> unsetKey()

The non estimated frames are the ones that are floating (not linked to the WOLF tree), so I think that they are not REGULAR at all..

In the current implementation (since we removed the non-key frames from trajectory), we should rethink the names. There is only floating non-estimated frames and linked estimated frames. The second ones are the typical ones (actually I proposed to avoid floating frames, they are not useful, but anyway...). I would skip the KEY/NON-KEY nomenclature, enum and functionality. The only difference is if the frames is linked to the tree. If you don't want to estimate it, either you fix it or you remove it. Let's discuss it in a meeting...

So every frame in the Trajectory is estimated and is a keyframe. Therefore:

Do additionally:

remove enum
remove setNonEstimated() -- unsetKey()

Work on:

setKey() --> link() to problem, somehow
create non KF / KF --> all the same, but link or not. It's a make_shared so remove completely
emplaceKF --> emplace
isKey() --> isLinked() or isInProblem(), etc --> remove
getLastKeyCapture, getLastKeyFrame, etc --> make names logical with all this new design

assigned to @jsola

added Doing To Do (release) labels and removed Agreement required Idea labels

mentioned in issue #330 (closed)

As per Capture:move(), the condition isKey() has been replaced by getProblem() so that if a frame has no Problem then you cannot move it there. However, I do not fully understand these asserts, they seem to check the opposite than what we want?

void CaptureBase::move(FrameBasePtr _frm_ptr)
{
    WOLF_WARN_COND(this->getFrame() == nullptr, "moving a capture not linked to any frame");
    WOLF_WARN_COND(_frm_ptr == nullptr, "moving a capture to a null FrameBasePtr");

    assert((this->getFrame() == nullptr || this->getFrame()->getProblem()) && "Forbidden: moving a capture already linked to a KF");
    assert((_frm_ptr == nullptr || !_frm_ptr->getProblem()) && "Forbidden: moving a capture to a non-estimated frame");

    // Unlink
    if (this->getFrame())
    {
        // unlink from previous non-key frame
        this->getFrame()->removeCapture(shared_from_this());
        this->setFrame(nullptr);
    }

    // link
    link(_frm_ptr);
}

Because in ProcessorTracker this is how move is used:

// Capture last_ is added to the new keyframe
FrameBasePtr last_old_frame = last_ptr_->getFrame();
last_ptr_->move(pack->key_frame);
last_old_frame->remove();

OK so these asserts were actually completely wrong --- amazing how things worked, probably no-one was testing move() in any of the gtests -- but what about the demos?

Here a TEST of the move() function:

TEST(CaptureBase, move)
{
    ProblemPtr problem = Problem::create("PO", 2);

    auto KF = problem->emplaceFrame(0.0); // dummy F object

    auto KC = CaptureBase::emplace<CaptureBase>(KF, "Dummy", 0.0);

    auto F = std::make_shared<FrameBase>(0.0, nullptr); // dummy F object

    auto C = CaptureBase::emplace<CaptureBase>(F, "Dummy", 0.0);

    ASSERT_EQ(KF->getCaptureList().size(), 1);
    ASSERT_EQ(F->getCaptureList().size(), 1);

    C->move(KF);

    ASSERT_EQ(KF->getCaptureList().size(), 2);
    ASSERT_EQ(F->getCaptureList().size(), 0);
}

which passes only if the asserts in move are written as:

    assert((this->getFrame() == nullptr || !this->getFrame()->getProblem()) && "Forbidden: moving a capture already linked to a KF");
    assert((_frm_ptr == nullptr || _frm_ptr->getProblem()) && "Forbidden: moving a capture to a non-estimated frame");

void CaptureBase::move(FrameBasePtr _frm_ptr)
{
    WOLF_WARN_COND(this->getFrame() == nullptr, "moving a capture not linked to any frame. Consider just linking it with link() instead of move()!");

    assert((this->getFrame() == nullptr || not this->getFrame()->getProblem()) && "Forbidden: moving a capture already linked to a KF");
    assert((_frm_ptr != nullptr) && "Forbidden: moving a capture to a null frame");
    assert((_frm_ptr != nullptr && _frm_ptr->getProblem()) && "Forbidden: moving a capture to a non-estimated frame");

    // Unlink
    if (this->getFrame())
    {
        // unlink from previous non-key frame
        this->getFrame()->removeCapture(shared_from_this());
        this->setFrame(nullptr);
    }

    // link
    link(_frm_ptr);
}

closed with merge request !403 (merged)

mentioned in commit 3c7a47dc

mentioned in commit e879ddc8

Merge Aux/KeyFrames into Estimated Frames

Designs

Child items ...

Activity