-
Notifications
You must be signed in to change notification settings - Fork 0
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
[feat] 3D and testing and matrixxd update #50
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Utku Melemetci <[email protected]>
…r large rotations/translations. Need improved nearest neighbor matching (KD-Tree/soft assignments) and better initial alignment (PCA, FPFH, manual registration).
b99dcd9
to
5762eaa
Compare
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.
Great work! Honestly just minor things to address, everything seems fine to me. It would be cool to get this merged, and we can do further refactors later.
CMakeLists.txt
Outdated
${PCL_INCLUDE_DIRS} | ||
) | ||
target_link_libraries(cevicp ${PCL_LIBRARIES}) |
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.
Do we need to link PCL for the main library? If not, can you please remove this?
CMakeLists.txt
Outdated
@@ -30,14 +34,18 @@ set(LIB_SOURCES | |||
lib/icp/impl/vanilla.cpp | |||
lib/icp/impl/trimmed.cpp | |||
lib/icp/impl/feature_aware.cpp | |||
lib/icp/impl/vanilla_3d.cpp | |||
common/parse_ply.cpp # Add the PLY parser source file |
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.
Does the main library use PCL?
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.
we may need it
CMakeLists.txt
Outdated
set(TEST_SOURCES_3D tests/test3d.cpp) | ||
add_executable(test_suite_3d ${TEST_SOURCES_3D}) | ||
target_link_libraries(test_suite_3d | ||
cevicp | ||
) | ||
target_include_directories(test_suite_3d | ||
PRIVATE | ||
${CMAKE_CURRENT_SOURCE_DIR}/include | ||
/usr/local/include/simple_test | ||
${PCL_INCLUDE_DIRS} | ||
) | ||
target_compile_definitions(test_suite_3d PRIVATE TEST) |
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.
Can we make this a part of the main test executable? By just writing another function in test.cpp
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.
Also, does this test need PCL? It would be good to not link with PCL unless we need 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.
i deleted the link to PCL. i don't really think it's appropriate to put it in test.cpp since it's aim for testing 2d
std::cout << "[4]Result Transform Translation X: " << result.transform.translation.x() | ||
<< std::endl; | ||
std::cout << "[4]Result Transform Translation Y: " << result.transform.translation.y() | ||
<< std::endl; | ||
std::cout << "[4]Result Iteration Count: " << result.iteration_count << std::endl; |
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.
Do we need these print statements? Or are they just for debugging
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.
for debugging only, should i keep them only on the testing branch?
std::vector<icp::Vector> a = {icp::Vector(0, 0)}; | ||
std::vector<icp::Vector> b = {icp::Vector(100, 0)}; | ||
auto result = driver.converge(a, b, icp::RBTransform()); | ||
// Test case 1: Single point translation |
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.
If this test case is unusable we can remove it
test
Outdated
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.
What is this file?
No description provided.