Skip to content

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)

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)


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


auto profile = p.get_profile().as<video_stream_profile>();
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

{
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

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 ?

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.

3 participants