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

Additional Viewer Enhancements #6423

Merged
merged 68 commits into from
Jun 1, 2020

Conversation

dorodnic
Copy link
Contributor

Adding simple distance measurement ability to the 3D view of the Viewer:
Screenshot from 2020-05-19 06-19-11

dorodnic and others added 30 commits July 27, 2019 13:11
@dorodnic dorodnic requested review from gwen2018 and ev-mp May 19, 2020 10:16
@dorodnic dorodnic added the GUI label May 19, 2020
Copy link
Contributor

@gwen2018 gwen2018 left a comment

Choose a reason for hiding this comment

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

when click on or close to one of the end points of existing segment, it may crash at line 2712 because at that moment selected_points[] may only has one element.
auto dist = selected_points[1].pos - selected_points[0].pos;

may caused by conflict in
if ((pos_2d - win.get_mouse().cursor).length() < 20)
{
dragging_point_index = i;
measurement_point_hovered = true;
if (win.get_mouse().mouse_down && input_ctrl.click_period() > 0.5f)
dragging_measurement_point = true;
}

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.

Great work!

@@ -38,6 +38,7 @@ void vbo::upload(int attribute, const float* xyz, int size, int count, bool dyna
dynamic ? GL_DYNAMIC_DRAW : GL_STATIC_DRAW);
glVertexAttribPointer(attribute, size, GL_FLOAT, GL_FALSE, 0, 0);
_size = count;
check_gl_error();
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is no legal note in the file's header

case GL_INVALID_ENUM: error="INVALID_ENUM"; break;
case GL_INVALID_VALUE: error="INVALID_VALUE"; break;
case GL_OUT_OF_MEMORY: error="OUT_OF_MEMORY"; break;
case GL_INVALID_FRAMEBUFFER_OPERATION: error="INVALID_FRAMEBUFFER_OPERATION"; break;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Default is missing


ss << "GL_" << error.c_str() << " - " << file << ":" << line << "\n";
err = glGetError();
has_errors = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

has_errors will be inconsistent if in the previous line err = GL_NO_ERROR.
Also - all the retrieved values are overwritten by the last one . Is it by design?

{
err = glGetError();
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing EOF

common/opengl3.h Outdated

#define check_gl_error() _check_gl_error(__FILE__,__LINE__)

#define MOUSE_PICK_USE_PBO 1 // use pbo to improve performance, for debug only
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should it be leaved in place?

@@ -0,0 +1,114 @@
#include "skybox.h"
Copy link
Collaborator

Choose a reason for hiding this comment

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

License label is missing


ImGui::PushStyleColor(ImGuiCol_Text, light_grey);
ImGui::PushStyleColor(ImGuiCol_TextSelectedBg, white);
// active = show_help_screen;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can the commented code be removed?


glMatrixMode(GL_PROJECTION);
glPushMatrix();
gluPerspective(45, viewer_rect.w / win.framebuf_height(), 0.001f, 100.0f);
Copy link
Collaborator

Choose a reason for hiding this comment

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

In case of T265 100 meter won't be enough imho as you may want to see the whole path

{
if (distance < 0.01f)
{
label = to_string() << std::fixed << std::setprecision(3) << distance * 1000.f << " mm";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice

return source.allocate_composite_frame(results);
if (results.size())
return source.allocate_composite_frame(results);
else return rs2::frame{};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not to pass through the input instead of an empty frame?

@dorodnic
Copy link
Contributor Author

Addressed code review comments and added some extra functionality -

  1. Finally found and fixed heap corruption on viewer close
  2. SHIFT allows to continue measuring more than one segment
  3. Measuring a closed loop will calculate and display area
  4. Ctrl+Z can be used to undo changes
  5. A bit improved mouse control, to avoid confusion during pan and zoom and highlight selected points & line segments

Copy link
Contributor

@gwen2018 gwen2018 left a comment

Choose a reason for hiding this comment

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

very nice additional features multiple segments and area calculation, real useful.

one more potential issue in measurement.cpp line 347, need to check dragging_point_index, in case a single click onto an endpoints.
if (dragging_measurement_point && dragging_point_index >= 0)

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 reviewed the latter part and it is good to go, imho.
The FW version conflict should be resolved.

@ev-mp ev-mp merged commit 690df61 into IntelRealSense:development Jun 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants