-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Voxel hashing GUI #3363
Voxel hashing GUI #3363
Conversation
…to wei/voxelhashing
Thanks for submitting this pull request! The maintainers of this repository would appreciate if you could update the CHANGELOG.md based on your changes. |
…/voxelhashing-gui
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.
Reviewed 27 of 28 files at r1.
Reviewable status: 27 of 28 files reviewed, 4 unresolved discussions (waiting on @prewettg, @reyanshsolis, and @theNded)
examples/cpp/VoxelHashingGUI.cpp, line 14 at r1 (raw file):
static const Eigen::Vector3d kTangoSkyBlueDark(0.125, 0.290, 0.529); std::shared_ptr<geometry::LineSet> CreateCameraFrustum(
Most of this function could be replaced with LineSet::CreateCameraVisualization
examples/cpp/VoxelHashingGUI.cpp, line 774 at r1 (raw file):
int main(int argc, char** argv) { if (argc < 2) { utility::LogError("Expected a dataset path as input.");
This should print a usage message and quit (return) gracefully.
examples/cpp/VoxelHashingGUI.cpp, line 779 at r1 (raw file):
std::string dataset_path = argv[1]; if (!utility::filesystem::DirectoryExists(dataset_path)) { utility::LogError("Expected an existing directory {}", dataset_path);
LogError
throws an exception that is uncaught which results in a crash. Since this is an expected possible error condition and we are in main I don't think it should crash the program. I recommend using LogWarning
plus a return -1
or some other return value.
examples/cpp/VoxelHashingGUI.cpp, line 787 at r1 (raw file):
utility::GetProgramOptionAsString(argc, argv, "--device", "CUDA:0"); if (device != "CPU:0" && device != "CUDA:0") { utility::LogError("Unrecognized device {}. Expecting CPU:0 or CUDA:0.",
Please see previous comment.
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.
Reviewed 1 of 28 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @prewettg, @reyanshsolis, and @theNded)
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.
Reviewable status: 23 of 30 files reviewed, 4 unresolved discussions (waiting on @errissa, @prewettg, and @reyanshsolis)
examples/cpp/VoxelHashingGUI.cpp, line 14 at r1 (raw file):
Previously, errissa (Rene) wrote…
Most of this function could be replaced with
LineSet::CreateCameraVisualization
Done.
examples/cpp/VoxelHashingGUI.cpp, line 774 at r1 (raw file):
Previously, errissa (Rene) wrote…
This should print a usage message and quit (return) gracefully.
Done.
examples/cpp/VoxelHashingGUI.cpp, line 779 at r1 (raw file):
Previously, errissa (Rene) wrote…
LogError
throws an exception that is uncaught which results in a crash. Since this is an expected possible error condition and we are in main I don't think it should crash the program. I recommend usingLogWarning
plus areturn -1
or some other return value.
Done.
examples/cpp/VoxelHashingGUI.cpp, line 787 at r1 (raw file):
Previously, errissa (Rene) wrote…
Please see previous comment.
Done.
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.
Reviewed 7 of 7 files at r2.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @prewettg and @reyanshsolis)
This PR introduces the VoxelHashing's application. Depending on #3323.
To be fixed before merging:
Camera intrinsic's path and dataset file structureFeatures:
TBD in the future:
This change is![Reviewable](https://camo.githubusercontent.com/23b05f5fb48215c989e92cc44cf6512512d083132bd3daf689867c8d9d386888/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)