alejocb / dpptam

DPPTAM: Dense Piecewise Planar Tracking and Mapping from a Monocular Sequence
GNU General Public License v3.0
220 stars 82 forks source link

Question regarding semidense mapping update #26

Closed sunghoon031 closed 8 years ago

sunghoon031 commented 8 years ago

Is there any reason why you keep "semidense_mapper->local_map_points" and "semidense_mapper ->points3D_toprint" separate? I'm referring to the giant for loop you have in line 866~1139, and how push back points_aux to sdm->local_map_points and points_aux2_print to sdm->points3D_toprint

Why not just do something like:

semidense_mapper -> local_map_points = semidense_mapper -> points3D_toprint[num_keyframes].clone()

Then you simply use more accurate 3D map points for tracking, right? I can simply change the code this way, and it will run just as fine. I would like to know if there was any special reason why you made it this way.

sunghoon031 commented 8 years ago

BTW, if I do this, then I can save a lot of time in that giant for loop, bringing down from average 32ms to 8ms on my laptop. I don't know what kinda advantage you gain by separating sdm->local_map_points and sdm->points3D_toprint, but I doesn't really seem it's worth extra 26 ms ...

sunghoon031 commented 8 years ago

But actually it seems like the dense mapping is better when you keep it in the original way... I see, so you're feeding "richer" version of map points to achieve better dense mapping, huh? I'm not too sure how much better the tracking would be by using this "richer, but noisier" local_map_points, but as far as the dense mapping is concerned, this is quite smart.

sunghoon031 commented 8 years ago

Enough monologue. I'm closing this issue.

alejocb commented 8 years ago

Hi Seong, Thank you for your suggestions and bugs!!

Yes, it is only to have a cleaner version of the 3D map.

Best, Alejo El 02/06/2016 20:19, "Seong" notifications@github.com escribió:

Enough monologue. I'm closing this issue.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/alejocb/dpptam/issues/26#issuecomment-223377450, or mute the thread https://github.com/notifications/unsubscribe/ADjYbFlFLOhzrOEAnFNJEmvWzucC4j0-ks5qHx6_gaJpZM4IsycL .

sunghoon031 commented 8 years ago

@alejocb I understand. Thanks for the reply :+1: I was thinking about adding some more flexibility in the way semidense_mapping thread passes map results to semidesne_tracking thread based on the boolean calculate_superpixels. That way you can compute and pass ONLY the "cleaner version" of the map to the tracking thread for some speed gain. I will see how it goes.

BTW any plans for checking my pull requests? XD

sunghoon031 commented 8 years ago

Actually, I just realized that sdm->local_map_points is simply identical to sdm->points3D_toprint[num_keyframes] ...!!

Here is another thing: I understand that the for-loop in line 1012-1080 is for "neighborhood-pixels-saving". The two conditions for saving the 3D points corresponding to this "neighborhood pixels" of inverse-depth-converged-point projections are:

if ( semidense_mapper -> G_expanded.at<float>(round(n_y_ref_aux),round(n_x_ref_aux)) <5)

and

 if (deviation_inv_depth.at<float>(i,0) > deviation_inv_depth_print_th 
  && be_outlier_print.at<float>(i,0)<0.5 && count_inlier_neighbours > 3  
  && camera_translation / point_to_camera > semidense_mapper-> translational_ratio_th_min*0.6 
  && pixel_taken_print.at<float>(n_y_ref_aux,n_x_ref_aux)<0.5)

where the first if condition is ALWAYS TRUE because we are only considering the pixels that are not taken yet. Now on top of this, if the second condition is also true, then we are pushing the same points twice to points_aux2... Is this a mistake or intended?

EDIT 1: Without this second if loop, the algorithm still works fine and yields more or less the same result (it seems so at first sight)

sunghoon031 commented 8 years ago

Ah never mind. The first if-loop handles points_aux2 whereas second if-loop handles points_aux2_print.