-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Unified cloud initializer pipeline for ICP (fixes segfault colored ICP) #6942
Unified cloud initializer pipeline for ICP (fixes segfault colored ICP) #6942
Conversation
Thanks for submitting this pull request! The maintainers of this repository would appreciate if you could update the CHANGELOG.md based on your changes. |
"PointToPlaneICP requires target pointcloud to have normals."); | ||
} | ||
std::shared_ptr<const geometry::PointCloud> source_initialized_c( | ||
&source, [](const geometry::PointCloud *) {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @nicolaloi. Can you explain why the deleter is empty here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benjaminum In some cases, like for PointToPoint and PointToPlane ICP, no actual modification/initialization of the point clouds is required. However, now there is a unified initialization interface across different ICP algorithms, so the initialization function (which is returning shared pointers to const point clouds) is being called in these cases too. Since in these cases the point clouds remain unchanged during the initialization step, creating a costly copy would be unnecessary (eg pc_initialized = make_shared(pc_original)). Instead, the shared pointers are created from the addresses of the original point clouds, effectively creating a const "view" of the originals. The empty deleter ensures that when these shared pointers go out of scope, they don't free the memory owned by the original point clouds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, got it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Type
Motivation and Context
The
RegistrationICP
function can be used to perform point-to-point ICP, point-to-plane ICP, generalized ICP, or colored ICP. Additionally, there are specific functions,RegistrationGeneralizedICP
andRegistrationColoredICP
, for computing generalized ICP and colored ICP only, respectively. These functions are simple wrappers aroundRegistrationICP
: they initialize the point clouds for the specific ICP algorithm (e.g., computing color gradients for colored ICP) and then call theRegistrationICP
function with the modified point clouds.However, this point cloud initialization is skipped if you directly call the
RegistrationICP
function to compute generalized ICP or colored ICP because the initialization is only performed in the wrapper functions. While this is not a significant issue for generalized ICP, it causes a segmentation fault for colored ICP, as the target point cloud must be initialized as an instance ofPointCloudForColoredICP
to compute the additionalcolor_gradients_
values.Checklist:
python util/check_style.py --apply
to apply Open3D code styleto my code.
updated accordingly.
results (e.g. screenshots or numbers) here.
Description
This PR introduces a unified point cloud initialization pipeline. A new virtual function,
InitializePointCloudsForTransformation
, is added to theTransformationEstimation
class. This function is overridden in derived classes to handle the specific initialization required for each specific ICP algorithm, and is being called at the beginning of theRegistrationICP
function.This fixes the segmentation fault that occurs when calling
RegistrationICP
for colored ICP, because now the target point cloud will be correctly initialized with thecolor_gradient_
values. It also allowsRegistrationICP
to compute generalized ICP with point clouds that don't have pre-computed normals. Previously, this was only possible by calling theRegistrationGeneralizedICP
wrapper, which exclusively handled the initialization of the point clouds.With this change the point clouds are correctly and consistently initialized across the different ICP algorithms, regardless of whether the user directly calls
RegistrationICP
or a specialized wrapper function likeRegistrationColoredICP
.Testing
Before
After