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.

Sign up for GitHub

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

Jump to bottom

RS-Viewer : metadata scrollbar added #11637

Merged
merged 4 commits into from May 2, 2023
Merged

RS-Viewer : metadata scrollbar added #11637

merged 4 commits into from May 2, 2023

Conversation

Tamir91
Copy link
Contributor

@Tamir91 Tamir91 commented Mar 30, 2023

Tracked on [LRS-714]

The scrollbar was created for metadata view mode.

@Tamir91 Tamir91 requested a review from Nir-Az March 30, 2023 13:33
@Nir-Az Nir-Az changed the title scrollbar created RS-Viewer : metadata scrollbar added Mar 30, 2023
Nir-Az
Nir-Az reviewed Apr 2, 2023
View reviewed changes
common/stream-model.cpp Outdated Show resolved Hide resolved
@Nir-Az
Copy link
Collaborator

Nir-Az commented Apr 2, 2023

Please also rebase development so we can see LibCI works

@Nir-Az
Copy link
Collaborator

Nir-Az commented Apr 2, 2023

Please also see a diff between key height and value height
image

@Tamir91 Tamir91 force-pushed the scrolling-option-for-metadata-in-rs-viewer branch from e523dc8 to 6015559 Compare April 3, 2023 10:35
Nir-Az
Nir-Az reviewed Apr 9, 2023
View reviewed changes
common/stream-model.cpp Outdated Show resolved Hide resolved
Nir-Az
Nir-Az reviewed Apr 9, 2023
View reviewed changes
common/stream-model.cpp Outdated Show resolved Hide resolved
Nir-Az
Nir-Az reviewed Apr 9, 2023
View reviewed changes
common/stream-model.cpp Outdated Show resolved Hide resolved
Nir-Az
Nir-Az reviewed Apr 9, 2023
View reviewed changes
common/stream-model.cpp Outdated Show resolved Hide resolved
Nir-Az
Nir-Az reviewed Apr 9, 2023
View reviewed changes
common/stream-model.cpp
@@ -1510,7 +1536,7 @@ namespace rs2
void stream_model::show_frame(const rect& stream_rect, const mouse_info& g, std::string& error_message)
{
auto zoom_val = 1.f;
if (stream_rect.contains(g.cursor))
if (stream_rect.contains(g.cursor) && !show_metadata)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a comment please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Collaborator

Choose a reason for hiding this comment

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

Where?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Now I see, you placed a comment 500 lines above.
Please add a comment here.
And the main reason I wanted a comment is to explain why we only calculate the mouse scroll for zoom when we don't have metadata.
Something like:

`We allow scrolling for zoom in/out when no metadata is on.
If we have metadata on it will scroll the metadata

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, my bad. Those lines look a little similar...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Still missing comment.
Allow mouse scrolling for zoom when not displaying scrollable metadata

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The comment was added.

Nir-Az
Nir-Az requested changes Apr 9, 2023
View reviewed changes
Copy link
Collaborator

@Nir-Az Nir-Az left a comment

Choose a reason for hiding this comment

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

Overall it looks great.
Just added a few questions and comment that may help the code to be more readable

@Tamir91
Copy link
Contributor Author

Tamir91 commented Apr 13, 2023

Please also see a diff between key height and value height image

fixed

Nir-Az
Nir-Az reviewed Apr 13, 2023
View reviewed changes
common/stream-model.cpp Outdated Show resolved Hide resolved
@Nir-Az
Copy link
Collaborator

Nir-Az commented Apr 13, 2023

OK a few things:

  1. Please fix compilation warning:
    stream-model.cpp(777,48): warning C4244: 'argument': conversion from 'double' to 'const __int64', possible loss of data
    from stream_model::draw_stream_metadata(...)
  2. Please use the reference code I send by mail for how to draw a scrollable background and restore the old UI behavior including the smooth rectangle
            // Draw a smooth rectangle background for each label
            // When we draw multiple rectangles with a shorter pixel on w & h is darker the inside rectangle
            for (auto i = 3; i > 0; i--)
                ImGui::GetWindowDrawList()->AddRectFilled(
                    { screen_pos.x + 10 - i, line_y - i },
                    { screen_pos.x + 10 + size.x + i, line_y + size.y + i },
                    ImColor( alpha( sensor_bg, 0.1f ) ) );
  1. use the same background for text, values, and no md labels as it was before.

See running example:
https://user-images.githubusercontent.com/64067618/231841869-5c55df78-a5d7-492b-b840-d36f51803c34.mp4

@Tamir91 Tamir91 requested a review from Nir-Az April 20, 2023 08:24
Nir-Az
Nir-Az reviewed Apr 23, 2023
View reviewed changes
common/stream-model.cpp Outdated Show resolved Hide resolved
@Tamir91 Tamir91 requested a review from Nir-Az April 27, 2023 08:15
Nir-Az
Nir-Az reviewed Apr 27, 2023
View reviewed changes
common/stream-model.cpp
rsutils::string::from() << std::setprecision(2) << std::fixed << view_fps.get_fps(),
"Viewer FPS captures how many frames the application manages to render.\n"
"Frame drops can occur for variety of reasons." });
const std::string no_md = "no md";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please verify that the use case of not having metadata looks OK too.
You can use @remibettan help to remove the metadata from the registry and verify it looks OK as it was on an old viewer code

Nir-Az
Nir-Az reviewed Apr 27, 2023
View reviewed changes
common/stream-model.cpp
@@ -959,6 +1018,7 @@ namespace rs2
|| (profile.stream_type() == RS2_STREAM_GPIO)
|| (profile.stream_type() == RS2_STREAM_POSE);

// This scope contains a logic behavior of mouse on viewport when metadata not visible for user.
if (stream_rect.contains(mouse.cursor) && !non_visual_stream && !show_metadata)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Curser behavior on visual stream with no metadata on

@Tamir91
Copy link
Contributor Author

Tamir91 commented May 1, 2023

image
Fixed with if (at.name == "") continue; line.

@Tamir91 Tamir91 closed this May 1, 2023
@Tamir91 Tamir91 force-pushed the scrolling-option-for-metadata-in-rs-viewer branch from 0c4cc7c to 27eb5d7 Compare May 1, 2023 11:04
@Nir-Az Nir-Az reopened this May 1, 2023
@Nir-Az Nir-Az merged commit 259f4bf into IntelRealSense:development May 2, 2023
16 checks passed
@Tamir91 Tamir91 added the GUI label May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Nir-Az Nir-Az Nir-Az approved these changes

Assignees
No one assigned
Labels
GUI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Tamir91 @Nir-Az