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

Fix linking curve plot axes #68

Open
wants to merge 2 commits into
base: devel
Choose a base branch
from
Open

Conversation

dzid26
Copy link
Contributor

@dzid26 dzid26 commented May 19, 2024

Fixes #67

By separating curve subplots from bar subplots, curve plots axis linking can be set without being affected by bar plot axis limits.

@klonyyy
Copy link
Owner

klonyyy commented May 23, 2024

I don't thing its a good solution. If it is possible to use subplots I would do so, and I'd try to solve the problem in a different way (not sure how yet). Your solution causes a scroll bar to appear, which makes the whole plot screen scroll if I simply want to zoom in or out.

ok:
image

not ok:
image

@dzid26
Copy link
Contributor Author

dzid26 commented May 23, 2024

Yes behaviour is slightly different, to what you had before:

const float remainingSpace = (ImGui::GetWindowPos().y + ImGui::GetWindowSize().y) - (ImGui::GetCursorPos().y + initialCursorPos.y);
ImVec2 plotSize(-1, -1);
if (remainingSpace < 300)
plotSize.y = 300;

But I think new behavior is more proper. It introduces scrollbar when sublot is getting too narrow. On old version, if I was to add 10 plots, it would be unusable on FullHD monitor.
image

You are right that scrolling is an issue - I didn't notice this. But this was also present without this change. If I reduce window height, scrollbar appears and scrolling issue is present:
image

I could emulate the old behavior for now, but it doesn't seem like the best solution.

It would be best to fix scrolling issue. Maybe page scrolling could be done only by means of dragging the slider and not by scrollwheel... Also related ocornut/imgui#7235

(cherry picked from commit 422562d364f06fd99441b60d9b3a6bc71efe762d)
@dzid26
Copy link
Contributor Author

dzid26 commented May 24, 2024

I fixed with scroll issue by changing the plot flag.

I think the only regression in this PR is that the bar plots are grouped at the bottom - not the same order as in the plot list.
image

@dzid26
Copy link
Contributor Author

dzid26 commented May 24, 2024

But even in the current version the order can be different if table type is used.

I think matching the plot order with the plot list can be revisited in the future.

I am quite happy with this change and what it enables.

@klonyyy
Copy link
Owner

klonyyy commented May 24, 2024

To be honest I don't really feel the need to show more than 4 or 5 plots at once. First point is that with five plots the acqusition might start to get laggy, the second one is that you can easily create a single plot dedicated to whatever you've got on the five plots you want to display at the moment. The solution you're proposing encourages to leave "garbage" plots on the viewport, thus reducing the app's performance. Moreover the Imgui/implot scrollbar is just not intutive - you have to find it on the right side, and if someone's new to the imgui ecosystem he/she might be lost. I'm really grateful for your input, but I just don't think its worth it - the axis linking is a bug, and I fully admit it, but at the same time it's not such a severe one that should make us introduce these changes (bar plot reordering, scroll bar appearing etc.).

For now I'd wait for other people to give their opinion on this PR.

@dzid26
Copy link
Contributor Author

dzid26 commented May 24, 2024

I am not insisting on the scrollbar behavior. I just think it's a bit better than it was before.
One scenario when scrollbar might usefull is when someone has e.g. 6 plots on 4k monitor, pauses the sim and then drags the window to a FullHD monitor. :)
It can be adjusted to show 5 plots (instead of current 4) before scrollbar appears.


I partially agree with the bar plot ordering.
But similarly to values table, bar plots could be separated (in the end their axis is not time).- This would also open a possibility to display time axis only on the most bottom curve plot to save space.

I am thinking there could be a separator line added on the plot list that indicates plot type and location.
image

After selecting plot type from the drop down, the plot name would jump to a right category (also reflecting its actual location in main window). Additionally then, maybe it would possible to drag plot names on the plot list which could be used to set exact order and even change plot type.

-> That's one idea how this could be solved in terms of the UX.

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.

None yet

2 participants