Skip to content

[roadmngr] Fixed Position::Delta sometimes flipping dLandId#266

Merged
eknabevcc merged 4 commits intoesmini:masterfrom
brifsttar:fix_delta_flip_laneid
Apr 6, 2022
Merged

[roadmngr] Fixed Position::Delta sometimes flipping dLandId#266
eknabevcc merged 4 commits intoesmini:masterfrom
brifsttar:fix_delta_flip_laneid

Conversation

@brifsttar
Copy link
Contributor

Hi Emil,

This should fix #261. I've taken this opportunity to also make the if condition more readable.

if (path->visited_.size() > 0) {
RoadPath::PathNode* lastNode = path->visited_.back();
RoadPath::PathNode* firstNode = lastNode;
while (firstNode->previous) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand this loop. It seems like a flaw that visited[0] is not the actual first road in the "path".
Still it would be nice to skip the loop. I'll check later if there is another solution (to store FIRST road).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The loop is taken from RoadPath::Calculate, which faces the same problematic of finding the first connection. The method could therefore store that value once found, so that it can be used elsewhere.

while (node)
{
if (node->previous == 0)
{
// This is the first node - inspect whether it is in front or behind start position
bool isPred = node->link == startRoad->GetLink(LinkType::PREDECESSOR);
bool isGTPi2 = abs(startPos_->GetHRelative()) > M_PI_2;
bool isLT3Pi2 = abs(startPos_->GetHRelative()) < 3 * M_PI / 2;
bool isSucc = node->link == startRoad->GetLink(LinkType::SUCCESSOR);
bool isLTPi2 = !isGTPi2;
bool isGT3Pi2 = !isLT3Pi2;
bool isPredAndBack = isPred && isGTPi2 && isLT3Pi2;
bool isSuccAndFront = isSucc && (isLTPi2 || isGT3Pi2);
if (isPredAndBack || isSuccAndFront)
{
direction_ = 1;
}
else
{
direction_ = -1;
}
}
node = node->previous;
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I like the idea to store the already calculated start direction. And then re-use it the Delta method which could be used frequently, hence we want to minimize unnecessary work.

I took the liberty of committing an attempt to achieve this on your branch. If it builds, please test and report how it works.

then re-use it for all Delta calculations (which could be many and often...)
@eknabevcc
Copy link
Collaborator

Good that a test case caught a bad attempt from my side.

I made another commit that seems to work better. But since it included also changes in RoadManager.hpp (which is not changed in your commit) I can't edit it in this PR. I tried to figure out a way to commit to the PR branch, but I had to give up.

Instead I created a branch fix_flip. Could you perhaps merge that branch on your (PR branch)?

As a last resort we could close this PR and I could merge fix_flip on master instead.

@brifsttar
Copy link
Contributor Author

I merged your branch on this PR, and I ran the test that initially failed (see #261), and it worked as expected. So all is ok on my end.

@eknabevcc eknabevcc merged commit 3ad640c into esmini:master Apr 6, 2022
@eknabevcc
Copy link
Collaborator

Excellent, thanks for the fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Position::Delta sometimes return wrong dLaneId

2 participants