Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Intended logic in updateKeyframes() #8

Closed
juliangaal opened this issue Jun 24, 2023 · 3 comments
Closed

Intended logic in updateKeyframes() #8

juliangaal opened this issue Jun 24, 2023 · 3 comments

Comments

@juliangaal
Copy link
Contributor

juliangaal commented Jun 24, 2023

I have some questions about this section of updateKeyframes():

bool newKeyframe = false;
// spaciousness keyframing
if (abs(dd) > this->keyframe_thresh_dist_ || abs(theta_deg) > this->keyframe_thresh_rot_) {
newKeyframe = true;
}
// rotational exploration keyframing
if (abs(dd) <= this->keyframe_thresh_dist_ && abs(theta_deg) > this->keyframe_thresh_rot_ && num_nearby <= 1) {
newKeyframe = true;
}
// check for nearby keyframes
if (abs(dd) <= this->keyframe_thresh_dist_) {
newKeyframe = false;
} else if (abs(dd) <= 0.5) {
newKeyframe = false;
}

To my understanding, there are some possible issues here:

rotational exploration keyframing

// rotational exploration keyframing 
if (abs(dd) <= this->keyframe_thresh_dist_ && abs(theta_deg) > this->keyframe_thresh_rot_ && num_nearby <= 1) { 
  newKeyframe = true; 
} 

is always overwritten by

 if (abs(dd) <= this->keyframe_thresh_dist_) { 
   newKeyframe = false;
}

num_nearby has no effect

If this statement

 if (... || abs(theta_deg) > this->keyframe_thresh_rot_) { 
   newKeyframe = true; 
 } 

is true (from the second condition),

// rotational exploration keyframing 
if (abs(dd) <= this->keyframe_thresh_dist_ && abs(theta_deg) > this->keyframe_thresh_rot_ && num_nearby <= 1) { 
  newKeyframe = true; 
} 

this statement doesn't change newKeyFrame, even if num_nearby is larger than 1. At this point, abs(dd) <= this->keyframe_thresh_dist_ is always true, given that abs(dd) > this->keyframe_thresh_dist_ was false in the condition before.

Intended logic

What is the intended logic here? I couldn't find much about this in your papers, except the adaptive thresholding distance to account for tight spaces.

Without the rotational exploration keyframing, the logic is clear to me (roughly):

if abs(dd) <= this->keyframe_thresh_dist or abs(dd) > 0.5 -> return false
else if abs(dd) > this->keyframe_thresh_dist -> return true

Now, I don't quite understand why the keyframe are updated when a large rotation was registered. In a scenario where, let's say, the robot stays in the same spot, but turns 180deg. Why do I have to update keyframes? The map is rotation invariant, so to speak, so I have gained no new information about the environment just be turning. I guess this is what was meant with the following statement in the DLO-paper?

Keyframe nodes are commonly dropped using fixed thresholds (e.g., every 1m or 10◦ of translational or rotational change)

Thanks for you help, happy to discuss

@kennyjchen
Copy link
Contributor

Ah -- the entire "check for nearby keyframes" statement should not be there. There were originally some additional checks in that block from our new DLIOM algorithm that I removed when open-sourcing this code (e.g., slippage), but I should have just removed that entire block. I'll push a new update in a bit. Thanks for the catch.

Regarding whether keyframes should be updated after a large rotation -- that largely depends on the FOV of the LiDAR. If the sensor is 360 degrees, then yes, keyframes are rotation-invariant. There are some sensors (i.e., Intel L515 or some older Livox sensors) that have a limited FOV, so keyframes are rotation-dependent. Even with modern mechanical LiDAR, they are only 360 FOV in the x-y direction, so tilting up or down would provide more information (and hence need a new keyframe). If I recall correctly, this exact scenario can be seen in the beginning of one of the LIO-SAM datasets.

@juliangaal
Copy link
Contributor Author

There were originally some additional checks in that block from our new DLIOM algorithm that I removed when open-sourcing this code (e.g., slippage), but I should have just removed that entire block. I'll push a new update in a bit. Thanks for the catch.

Understood. DLIOM being available some day would be awesome :)

There are some sensors (i.e., Intel L515 or some older Livox sensors) that have a limited FOV, so keyframes are rotation-dependent [...]
so tilting up or down would provide more information

Makes perfect sense!

@kennyjchen
Copy link
Contributor

Updated! Thanks again for the catch.

gandres42 pushed a commit to Cardinal-Space-Mining/direct_lidar_inertial_odometry that referenced this issue May 13, 2024
…-workday-3-2

add timestamp param to localization subscribers
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

No branches or pull requests

2 participants