search

klonyyy / STMViewer

Real-time STM32 variable & trace viewer
GNU General Public License v3.0
536 stars 59 forks source link

Fix linking curve plot axes #68

Open dzid26 opened 1 month ago

dzid26 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

Yes behaviour is slightly different, to what you had before: https://github.com/klonyyy/STMViewer/blob/3676a8ef7c4cf7af082bb73ae6435f68ed4c4fb6/src/Gui/GuiPlots.cpp#L39-L42

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 https://github.com/ocornut/imgui/issues/7235

dzid26 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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.