Skip to content
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

Merged
merged 25 commits into from Nov 12, 2019
Merged

PLY export #5110

merged 25 commits into from Nov 12, 2019

Conversation

dorodnic
Copy link
Contributor

Continuation of #4906 by @AnnaRomanov

Copy link
Collaborator

@ev-mp ev-mp left a 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))
Copy link
Collaborator

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/viewer.cpp Show resolved Hide resolved
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 ") <<
Copy link
Collaborator

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

Copy link
Contributor Author

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)

@@ -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);
Copy link
Collaborator

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;
Copy link
Collaborator

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

@dorodnic
Copy link
Contributor Author

dorodnic commented Nov 2, 2019

Thanks @ev-mp
All good points
Please do not merge yet, I'll work on it

@dorodnic dorodnic mentioned this pull request Nov 2, 2019
@dorodnic
Copy link
Contributor Author

@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 });
Copy link
Collaborator

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 });
Copy link
Collaborator

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?

Copy link
Collaborator

@ev-mp ev-mp left a 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;
Copy link
Collaborator

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)
Copy link
Collaborator

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

.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);
Copy link
Collaborator

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;
Copy link
Collaborator

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);
Copy link
Collaborator

@ev-mp ev-mp Nov 11, 2019

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

Copy link
Collaborator

@ev-mp ev-mp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@dorodnic dorodnic merged commit 17cd8e2 into IntelRealSense:development Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants