Skip to content

Additional Viewer Enhancements#6423

Merged
ev-mp merged 68 commits into
realsenseai:developmentfrom
dorodnic:mouse_picking2
Jun 1, 2020
Merged

Additional Viewer Enhancements#6423
ev-mp merged 68 commits into
realsenseai:developmentfrom
dorodnic:mouse_picking2

Conversation

@dorodnic

Copy link
Copy Markdown
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 ev-mp and gwen2018 May 19, 2020 10:16
@dorodnic dorodnic added the GUI label May 19, 2020

@gwen2018 gwen2018 left a comment

Copy link
Copy Markdown
Contributor

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;
}

@ev-mp ev-mp left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great work!

Comment thread common/opengl3.cpp
dynamic ? GL_DYNAMIC_DRAW : GL_STATIC_DRAW);
glVertexAttribPointer(attribute, size, GL_FLOAT, GL_FALSE, 0, 0);
_size = count;
check_gl_error();

Copy link
Copy Markdown
Contributor

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

Comment thread common/opengl3.cpp
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
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Default is missing

Comment thread common/opengl3.cpp

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

Copy link
Copy Markdown
Contributor

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?

Comment thread common/opengl3.cpp Outdated
{
err = glGetError();
}
} No newline at end of file

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Missing EOF

Comment thread 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
Copy Markdown
Contributor

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?

Comment thread common/skybox.cpp
@@ -0,0 +1,114 @@
#include "skybox.h"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

License label is missing

Comment thread common/viewer.cpp Outdated

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

Copy link
Copy Markdown
Contributor

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?

Comment thread common/viewer.cpp

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

Copy link
Copy Markdown
Contributor

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

Comment thread common/viewer.cpp Outdated
{
if (distance < 0.01f)
{
label = to_string() << std::fixed << std::setprecision(3) << distance * 1000.f << " mm";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice

Comment thread src/proc/zero-order.cpp
return source.allocate_composite_frame(results);
if (results.size())
return source.allocate_composite_frame(results);
else return rs2::frame{};

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

@gwen2018 gwen2018 left a comment

Copy link
Copy Markdown
Contributor

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)

@ev-mp ev-mp left a comment

Copy link
Copy Markdown
Contributor

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 realsenseai: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