-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
PLY export #5110
PLY export #5110
Conversation
… save_to_ply callback
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 tested it and it looks good.
Several minor suggestions to enhance user experience
ImGui::NewLine(); | ||
ImGui::SetCursorScreenPos({ (float)(x0 + 15), (float)(y0 + 90) }); | ||
ImGui::Separator(); | ||
if (ImGui::Checkbox("Meshing", &mesh)) |
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.
The checkbox explanation and caption are not complete, especially what will be the output if button left unchecked. The button would be used as an addon "Generate Mesh" to generate Polygons in addition to Points (that always present)
common/notifications.cpp
Outdated
ImGui::PushStyleColor(ImGuiCol_Text, alpha(light_grey, 1. - t)); | ||
|
||
std::string s = to_string() << "Saving 3D view " | ||
<< (get_manager().get_data().is<rs2::points>() ? "without texture to " : "to ") << |
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.
Not clear what "without texture to.. means - also couldn't reproduce this flow
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.
You are right, this is redundant today (remains from early versions where you could have pointcloud without texture)
common/ux-window.cpp
Outdated
@@ -46,6 +46,10 @@ namespace rs2 | |||
config_file::instance().set_default(configurations::performance::show_fps, false); | |||
config_file::instance().set_default(configurations::performance::vsync, true); | |||
|
|||
config_file::instance().set_default(configurations::ply::mesh, true); | |||
config_file::instance().set_default(configurations::ply::use_normals, false); | |||
config_file::instance().set_default(configurations::ply::encoding, 1); |
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.
pls replace int with enum for maintainability
@@ -81,76 +111,174 @@ namespace rs2 | |||
auto width = profile.width(), height = profile.height(); | |||
static const auto threshold = 0.05f; |
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.
Is it possible to add the threshold to the list of PLY options ? This will allow for better fine-tune of resulted models
Thanks @ev-mp |
@ev-mp - updated, please approve |
register_simple_option(OPTION_PLY_MESH, option_range{ 0, 1, 1, 1 }); | ||
register_simple_option(OPTION_PLY_NORMALS, option_range{ 0, 1, 0, 1 }); | ||
register_simple_option(OPTION_PLY_BINARY, option_range{ 0, 1, 1, 1 }); | ||
register_simple_option(OPTION_PLY_THRESHOLD, option_range{ 0, 1, 0.05f, 0 }); |
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.
adjust the default 0.05 as in original
@@ -68,89 +100,187 @@ namespace rs2 | |||
fabs(verts[i].z) >= min_distance) | |||
{ | |||
idx_map[i] = new_verts.size(); | |||
new_verts.push_back(verts[i]); | |||
new_verts.push_back({ verts[i].x, -1 * verts[i].y, -1 * verts[i].z }); |
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 you add a comment that explain RLS->PLY coordinate system adjustment?
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.
Looks good - I'm not sure whether the added option should propagated into the wrappers. If not -disregard those comments
@@ -2,6 +2,9 @@ | |||
classdef save_to_ply < realsense.filter | |||
properties (Constant=true) | |||
option_ignore_color = realsense.option.count + 1; | |||
option_ply_mesh = realsense.option.count + 2; | |||
option_ply_binary = realsense.option.count + 3; | |||
option_ply_normals = realsense.option.count + 4; |
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.
Add OPTION_PLY_THRESHOLD
# Set options to the desired values | ||
# In this example we'll generate a textual PLY with normals (mesh is already created by default) | ||
ply.set_option(rs.save_to_ply.option_ply_binary, False) | ||
ply.set_option(rs.save_to_ply.option_ply_normals, True) |
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.
OPTION_PLY_THRESHOLD Optional here - can be added for tutoring
wrappers/python/python.cpp
Outdated
.def_readonly_static("option_ignore_color", &rs2::save_to_ply::OPTION_IGNORE_COLOR) | ||
.def_readonly_static("option_ply_mesh", &rs2::save_to_ply::OPTION_PLY_MESH) | ||
.def_readonly_static("option_ply_binary", &rs2::save_to_ply::OPTION_PLY_BINARY) | ||
.def_readonly_static("option_ply_normals", &rs2::save_to_ply::OPTION_PLY_NORMALS); |
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.
OPTION_PLY_THRESHOLD
const rs2_option save_to_ply::OPTION_IGNORE_COLOR; | ||
const rs2_option save_to_ply::OPTION_PLY_MESH; | ||
const rs2_option save_to_ply::OPTION_PLY_BINARY; | ||
const rs2_option save_to_ply::OPTION_PLY_NORMALS; |
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.
Is it required here - OPTION_PLY_THRESHOLD ?
new_tex.push_back(rgb); | ||
} | ||
} | ||
} | ||
|
||
auto profile = p.get_profile().as<video_stream_profile>(); | ||
auto width = profile.width(), height = profile.height(); | ||
static const auto threshold = 0.05f; | ||
static const auto threshold = get_option(OPTION_PLY_THRESHOLD); |
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.
Probably should be queried per invocation
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.
Looks good
Continuation of #4906 by @AnnaRomanov