-
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
Additional Viewer Enhancements #6423
Conversation
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.
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;
}
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.
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(); |
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.
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; |
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.
Default is missing
|
||
ss << "GL_" << error.c_str() << " - " << file << ":" << line << "\n"; | ||
err = glGetError(); | ||
has_errors = 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.
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?
common/opengl3.cpp
Outdated
{ | ||
err = glGetError(); | ||
} | ||
} |
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.
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 |
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.
Should it be leaved in place?
@@ -0,0 +1,114 @@ | |||
#include "skybox.h" |
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.
License label is missing
common/viewer.cpp
Outdated
|
||
ImGui::PushStyleColor(ImGuiCol_Text, light_grey); | ||
ImGui::PushStyleColor(ImGuiCol_TextSelectedBg, white); | ||
// active = show_help_screen; |
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 the commented code be removed?
|
||
glMatrixMode(GL_PROJECTION); | ||
glPushMatrix(); | ||
gluPerspective(45, viewer_rect.w / win.framebuf_height(), 0.001f, 100.0f); |
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.
In case of T265 100 meter won't be enough imho as you may want to see the whole path
common/viewer.cpp
Outdated
{ | ||
if (distance < 0.01f) | ||
{ | ||
label = to_string() << std::fixed << std::setprecision(3) << distance * 1000.f << " mm"; |
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.
Nice
return source.allocate_composite_frame(results); | ||
if (results.size()) | ||
return source.allocate_composite_frame(results); | ||
else return rs2::frame{}; |
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.
Why not to pass through the input instead of an empty frame?
Addressed code review comments and added some extra functionality -
|
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.
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)
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 reviewed the latter part and it is good to go, imho.
The FW version conflict should be resolved.
Adding simple distance measurement ability to the 3D view of the Viewer: