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

ImPlot v0.1 Discussions #1

Closed
epezent opened this issue Apr 27, 2020 · 44 comments
Closed

ImPlot v0.1 Discussions #1

epezent opened this issue Apr 27, 2020 · 44 comments
Labels
help wanted Extra attention is needed

Comments

@epezent
Copy link
Owner

epezent commented Apr 27, 2020

The issue serves to continue on-going discussions from the original plotting widget that was found in mahi-gui.

@epezent
Copy link
Owner Author

epezent commented Apr 27, 2020

Thanks. Actually i need some time in order to get familiar with new API, it's totally different...
As already mentioned, my wondering features are:

  • digital channels (as recently introduced but now gone...)
  • multiple y axis and drag/drop channels over y axis
  • cursors to analyze and display data: delta time, peak min max values, average
  • save as "screenshot"
  • import/export data and settings
    Are you interested on?

PS: about huge data handling, maybe I'm ignoring something, but have you ever cosidered to handle in different way the "sub pixeling"? I mean, during points for loop cycle, util x "pixel" point will remain the same (due to x wide zoom), instead to plot multiple overlapping lines, while x coorinate is the same consider the y most deviating point. In this way you reduce the total amount of lines and you don't loose spikes of y in wide zoom mode.

  • Understood. The new API is more user friendly, but as a result the backend is a bit more complicated. Let me know if you have questions.
  • Yes, I want to include your Digital Signals! I haven't myself ported it to the new API, but it should be trivial if you want to take a stab at it.
  • I think multi-axis is a good idea, but probably should be saved for later when the rest of the library is more mature. I have given it some thought, and haven't quite come up with a great solution yet. The main issue is knowing which axis the user wants their data to be plotted against. That is, when the call ImGui::Plot("Plot",xs,ys) is made, which axis should ys be transformed to? One solution would be to have a separate ImGui::BeginMultiAxisPlot() function, and then allow the user to push the current axis, e.g. ImGui::PushYAxis(1) would indicate that following y data should be mapped to the secondary y-axis.
  • Generic plot statistics such as average, min/max values is a good idea as well. PyQtGraph does some of these, and perhaps we should look to there for inspiration.
  • Saving screenshots can be accomplished simply through the OS (e.g. PrtSc button on Windows). If ImGui provided functions to record screenshots, then I would be for it. However, it does not and this is inherently a platform/backend dependent feature, so I'm hesitant to try and support all possibilities.
  • What might be more useful, anyhow, is to instead export the current plot data and axis information to a file format that can be opened with professional plotting libraries (e.g. MATLAB *.m or matplotlib *.py files). The plot could then be manipulated for, say, publication or presentation.
  • It would be nice to be able to save plot settings (e.g. axis ranges, scales, etc.) to a ini file, so that they are retained between sessions. I'm not sure if we can us imgui.ini for this, or if we'd need a separate implot.ini. Like the other points here, I think this is best for future work when the API is rock-solid.
  • I really like your "down-sampling" idea, by checking against the previous pixel position. I will look into this soon!

@ozlb
Copy link
Contributor

ozlb commented Apr 27, 2020

  • digital signals: if you want I can try to work on in order to understand better the new API
  • "down-sampling": my idea is to create an inner while loop that updates p1.y with MAX(p1.y, p2.y) until p1.x == p2.x . can it works?
  • plot statistics/analysis: something like follwing screenshot i think could be a good starting point
    a
  • x axis with date/time: this is another wondering feature

I agree with you about all other points.

@epezent
Copy link
Owner Author

epezent commented Apr 27, 2020

  • Yes, please make a first pass a PlotDigital(...). I was a little confused about its inner workings last time. It seemed strange that they were plotted in pixel units and not plot units. Is there a way to make the API work with plot units? Otherwise, perhaps it should be a completely separate entity.
  • The down sampling may work, but values may have to be cast/round to int first since they are float and unlikely to be exactly equal.
  • I see what you mean now regarding analytic tools. ImGui will soon provide an improved Tables API, and so if this is your vision, then we should investigate how the two can be integrated seamlessly.
  • a data/time axis format seems reasonable. We would need to make grid divisions/subdivisions more intelligent/robust so as to not cause label overlapping

@ozlb
Copy link
Contributor

ozlb commented Apr 27, 2020

  • plot statistics/analysis: draw and move cursors and somehow give access to data between is something that can be done regardless how to present it (as your suggestion in tables or file/export)

@epezent
Copy link
Owner Author

epezent commented Apr 28, 2020

That is a fair point. I guess we only really need to expose the ability to do basic queries, and then let the end user decide how to process the query. I've added a feature that allows a query range to be drawn (middle drag or Ctrl + right drag). Use the Alt and Shift keys to expand the range. The range can then be retrieved with IsPlotQueried() and GetPlotQuery(). This is just a start. We can continue to expand upon this. Let me know what you think!

ezgif-4-d232dbdff286

@ozlb
Copy link
Contributor

ozlb commented Apr 28, 2020

IsPlotQueried() and GetPlotQuery() looks a very good starting point. I would prefere sort of permanent "query range" not to hide on mouse release but instead on a new selection and configurable options in parallel to IO.KeyShift or IO.KeyAlt handling

@epezent
Copy link
Owner Author

epezent commented Apr 28, 2020

I agree that a "set and forget" mode for query ranges would be useful. I will need to decouple it a bit more from box selection to make it work, but it should be doable. Do you have any ideas for what the input method would be?

@ozlb
Copy link
Contributor

ozlb commented Apr 28, 2020

What you did is excellent, but i think it's annoying that selection disappears and you cannot refine the query context.
What about the idea of selection implemented by https://github.com/soulthreads/imgui-plot?
It's similar, only that is missing the persistence and eventually the range selection editing

@ozlb
Copy link
Contributor

ozlb commented Apr 28, 2020

.. or cursors #1 (comment)

@ozlb
Copy link
Contributor

ozlb commented Apr 28, 2020

  • Digital channels WIP: what do you think?
    Peek 2020-04-28 23-31

I have a doubt about gp.BB_Grid: what is exactly!? I'm using gp.PixelRange instead

@epezent
Copy link
Owner Author

epezent commented Apr 28, 2020

gp.BB_grid is the regular bounding box of the plot area/grid in pixels. gp.PixelRange is the same, but the min/max values get swapped when the axes are flipped/inverted.

I think the digital signals look great! My only concern is that they don't scroll with everything else. I would like to see them work similar to the bar plots function. The way I would do it, is let the user specify a y location and height, both in plot units.

What is the function signature? How is digital data represented? (bool* ?)

@epezent
Copy link
Owner Author

epezent commented Apr 28, 2020

I should be more specific...I find it strange that they don't scroll in the Y direction.

@ozlb
Copy link
Contributor

ozlb commented Apr 28, 2020

To be honest the vertical scrolling and zooming is what i do not want at all for digital signals; the real scenario is mixing logic events (digital) with control signals (analog), and if you zoom vertically the digital signals you will loose the analysis context.

About the function signature actually it's derived from Plot() so it's still float* and so signals value are 0.0f or 1.0f. Obviously it's not good, but i was more afraid about new API familiarization.

@epezent
Copy link
Owner Author

epezent commented Apr 28, 2020

Is there a reason that the digital signals couldn't be plotted on a separate plot?

@ozlb
Copy link
Contributor

ozlb commented Apr 28, 2020

they can be plotted on separate plot as well, but imagine that you want to check at rising edge of digital signal the value of another analog signal? it's easy to vertical scroll the analog signal and the point of intersection with digital positive edge is the value at that specific moment. It's very usefull

@epezent
Copy link
Owner Author

epezent commented Apr 28, 2020

Ah, I see. I suppose you also would need to link their x-axis somehow. I understand the issue now.

We can keep it as is until we come up with a better solution. Submit a PR when you are ready. Can you also add it to the demo under a new header (and possibly to Realtime Plots as well?

@ozlb
Copy link
Contributor

ozlb commented Apr 28, 2020

Yes, correct.
Anyhow, link x-axis between multiple plots is something that should be done as compensation of absence of multiple y axis in a single plot.

I will work to cleanup a little bit the code and try to handle digital data as bool*. If you have a tip about this topic, you are welcome.

@epezent
Copy link
Owner Author

epezent commented Apr 28, 2020

actually, the minimum amount of information needed to render the plot is 1) the initial state (low/high), and then a sequence of times ( or x values) where the state flips. so:

PlotDigital(const char* label, const float* x_flip, bool intial_value)

or, if you prefer:

PlotDigital(const char* label, const float* xs, const bool* ys)

@epezent
Copy link
Owner Author

epezent commented Apr 28, 2020

The second may be more user friendly.

@epezent
Copy link
Owner Author

epezent commented Apr 29, 2020

I pushed a much improved query mode per your recommendations. This took a while to get just right, but I'm very happy with how it turned out. Checkout the Query and Views demos!

I couldn't decide if queries should be pixel based, or plot unit based. They both seemed useful, so I added both. The mode can be toggled with the ImPlotFlags_PixelQuery flag. You can see the difference that it makes in the following GIFs:

query
views
rt_query

@epezent
Copy link
Owner Author

epezent commented Apr 29, 2020

I just need to add a few functions that allow for query ranges to be set through code, and then I think this is good enough for now. Any thing else you'd like to see from this?

@ozlb
Copy link
Contributor

ozlb commented Apr 29, 2020

Great job!
Pixel query is my favorite, but have the opportunity to hanlde as option is great.
The only "request" could be to activte the CTRL+SHIFT selection programmaticaly, so a function that create this "scenario" by code, so we will have practically the "show cursors" request that i described initially in my wonder features list.

@ozlb
Copy link
Contributor

ozlb commented Apr 29, 2020

actually, the minimum amount of information needed to render the plot is 1) the initial state (low/high), and then a sequence of times ( or x values) where the state flips. so:

PlotDigital(const char* label, const float* x_flip, bool intial_value)

or, if you prefer:

PlotDigital(const char* label, const float* xs, const bool* ys)

Change ImPlotGetterData like so or am I totally wrong?

struct ImPlotGetterData
{
    const float* Xs;
    const float* Ys;
    const bool* YDs;
    const float* ErrNeg;
    const float* ErrPos;
    int Stride;
    float XShift;
    float YShift;
    ImPlotGetterData(const float* xs, const float* ys, int stride, 
                    float x_shift = 0, float y_shift = 0, 
                    const float* err_neg = NULL, const float* err_pos = NULL) {
        Xs = xs;
        Ys = ys;
        Stride = stride;
        XShift = x_shift;
        YShift = y_shift;
        ErrNeg = err_neg;
        ErrPos = err_pos;
    }
    ImPlotGetterData(const float* xs, const bool* ys, int stride, 
                    float x_shift = 0, float y_shift = 0, 
                    const float* err_neg = NULL, const float* err_pos = NULL) {
        Xs = xs;
        YDs = ys;
        Stride = stride;
        XShift = x_shift;
        YShift = y_shift;
        ErrNeg = err_neg;
        ErrPos = err_pos;
    }
};

@epezent
Copy link
Owner Author

epezent commented Apr 29, 2020

I would probably just make a new ImGetterDigitalData. This one has too many fields that won't be useful, and doesn't have a bool* member.

@ozlb
Copy link
Contributor

ozlb commented Apr 29, 2020

I try but i failed so many times. I will leave in float because I'm loosing to much time.

@epezent
Copy link
Owner Author

epezent commented Apr 29, 2020

No worries, I'll add it when I review/merge your PR. Thanks for working on this!

@epezent
Copy link
Owner Author

epezent commented Apr 30, 2020

It was never that way. It's only ever been that you could double click the plot area to fit both axes. However, axes that are locked aren't fit.

Fitting by double clicking the axis labels seems like a good idea. I'll add that now.

@epezent
Copy link
Owner Author

epezent commented Apr 30, 2020

You can fit individual axes now!

I also added the original mahi-gui benchmark to the demo. I get around 110 FPS on my machine, which is about 3x slower than it used to be. The slow down is probably due to 1) the conditional in the point transformation function (gp.ToPixels) now that checks for log scale, and 2) the use of the "getter" function pointer, which the compiler likely can't optimize. 1) is an easy fix, 2) may be harder to work around.

@ozlb
Copy link
Contributor

ozlb commented Apr 30, 2020

Thanks!
About performances (with VSYNC disable) both functions are time comsuming, but mostly 2)

@epezent
Copy link
Owner Author

epezent commented Apr 30, 2020

Right. I am going to make the Plot(...,float* xs, float* ys,...) version bypass the getter version and then move the linear/log check outside of the loop so it isn't repeated for every data point. I'll need to refactor parts of the code to avoid duplication. May be a day or two, but at least it's fixable.

@ozlb
Copy link
Contributor

ozlb commented May 1, 2020

I think that pointer to circular buffer is the most efficient way to put and get points, but i did some trials with void PlotP(const char* label_id, const float* xs, const float* ys, int count, int offset) that do not use getter and performances are pratically the same...

I'm thinking that probably there are other optimizations that could improve performances at least for "continuous plot" and i mean the condition when x data buffer is a "monotone ascending sequence":
a) pixel density: while points will be plot to same pixels, amount of lines to plot can be optimized. We already discuss about this topic. This will have benefits in any condition. You can "select" next valid p2 where x plotting point will be greater
b) points outside plot range: while x values of p1 and p2 are less than x.min of current plot range (that depends on "zoom" factor) they can be totally skipped (continue in for loop i mean) and when p1 and p2 are greater than x.max of current plot range you can break the loop. Sort of culled preconditions.

Eventually this "optimization" can be enabled by a flag "x monotone ascending sequence".

Obviously those considerations and improves are not related to benchmark since the aim is to evaluate global code efficiency.

@ozlb

This comment has been minimized.

@epezent
Copy link
Owner Author

epezent commented May 2, 2020

Awesome! I've also been working on some optimizations (moved linear/log scaling conditionals to the outside of the loop, and using templates instead of function points to get a compile time advantage). I'll integrate your code with my changes (expect a push soon).

However, I discovered what the real issue is. There's a dramatic decrease in performance when more than one ImGui window is present. This is because each window has it's own DrawList, and apparently swapping the DrawLists given to OpenGL has a significant overhead.

The issue has only presented itself now because I started using a BeginChild/EndChild region inside of BeginPlot/EndPlot to capture mouse scroll (so that the window doesn't scroll when the plot is scrolled). With my optimizations above and disabling the child region, my FPS goes from 110 to 360. So, I think it's worth investigating other ways to capture mouse scroll.

@ozlb
Copy link
Contributor

ozlb commented May 3, 2020

This is a great. I guess that @ocornut can help you about this topic with proper advise.

@ozlb
Copy link
Contributor

ozlb commented May 4, 2020

About #6 (comment) (where i'm not authorized to reply directly), Cursors are actually sort of "XQuery". Query are very nice and usefull but you need to draw and drag, while "XQuery" they just appear and you move where you need for your data analysis.

Inspired by https://www.youtube.com/watch?v=9HWBc_oHB-k

@epezent
Copy link
Owner Author

epezent commented May 4, 2020

What if instead we made the edges and corners of Query dragable/resizable after it is created? If the query was initially created using the Shift key (to expand vertical), then it would be the exact same behavior as far as I can tell. It also has the added benefit of working in the Y direction when needed. I'd just rather not have two similar tools to manage and upkeep.

epezent pushed a commit that referenced this issue May 5, 2020
@jpieper
Copy link
Contributor

jpieper commented May 9, 2020

Hi... random interloper here. I'm interested in implementing multiple y axis support as a more general version of the digital signal handling (and I need it for more generally referencing signals with disparate y ranges).

Question for @epezent : Your initial comment in this thread implied that maybe the old version in mahi-gui had support for multiple y axes? I looked prior to 2c957b5 and didn't see anything, so I'm guessing that was just me mis-reading the thread?

Second question: You mused about an API of the form ImGui::PushYAxis that required a preceeding ImGui::BeginMultiAxisPlot. Any qualms with something more like ImGui::SetYAxis(n) (i.e. it doesn't need to be popped) and have the multi-axis nature be inferred if SetYAxis is ever called with a value greater than 0 (i.e. it could be omitted for Y axis 0 only plotting and there would be no need for a different plot creation call).

Any other thoughts on the topic?

@epezent
Copy link
Owner Author

epezent commented May 9, 2020

Hi @jpieper! Thanks for stopping by.

No, mahi-gui never had multiple y axes. It's only ever been a pipe dream, and no one has started working on it as far as I know.

I'm in favor of a more generic API as you suggest, that just works with the existing BeginPlot / EndPlot. However, the issue I foresaw with something like this is that a lot of sizing and rendering is done in BeginPlot. In particular, the plot area bounding box is determined here (based on the presence of titles, labels, tick labels, etc), and then the grid, ticks, and labels are rendered immediately so that all following plot items can be rendered over them. Therefore, BeginPlot needs to be aware of how many labeled axes to size for and render. Adding axes in between BeginPlot/EndPlot seems infeasible with the current rendering pipe-line. That is why I suggested a separate ImGui::BeginMultiAxisPlot or similar.

If we are to keep it generic, then either we'd need to make the number of visible axes a parameter of BeginPlot, or add a helper function like SetNextPlotAxesCount. I don't have a preference here.

The most extreme approach would be to delay all rendering until EndPlot. However, the issue I saw with this is that 1) we'd have to hold on to the user's data pointer, which may expire by the time EndPlot is reached, which means that 2) we'd have to copy their data into a buffer which was a big no-no for me

It's a challenging problem, so I gladly accept your support and any and all contributions.

@jpieper
Copy link
Contributor

jpieper commented May 9, 2020

So, there's still a fair amount of work left, but how does the general approach in

look?

You can only drag the first Y axis, fitting doesn't work, and auxiliary axes have no ticks or visual rendering at all yet.

I'm happy to take feedback on all levels, including API, style, and implementation.

@jpieper
Copy link
Contributor

jpieper commented May 9, 2020

I should say, I have not implemented it yet, but in prior systems I have set it up so that holding down the 1, 2, 3, etc keys would select translation and zooming of solely those axes, where if nothing is pressed all Y axes move in sync. Seem OK?

@epezent
Copy link
Owner Author

epezent commented May 9, 2020

I should say, I have not implemented it yet, but in prior systems I have set it up so that holding down the 1, 2, 3, etc keys would select translation and zooming of solely those axes, where if nothing is pressed all Y axes move in sync. Seem OK?

That sounds reasonable to me for when input is done while hovering the main plot area. For input done over the axes labels, I think each should have an independent hit box.

A couple comments/questions that come to mind (stream of consciousness, apologies):

  • Are we going to support multiple x-axes? My hunch is that it's fairly uncommon and probably not worth it, but in may be good to design this system as abstractly as possible so that we can add it on later if really needed.
  • Three axes max seems reasonable to me. How common is it to use more than two, though?
  • I like the use of ImPlotFlags to specify multi-axis, but I'm curious what happens if ImPlotFlags_Y3Axis is passed without ImPlotFlags_Y2Axis? Is there a sleeker way to handle this?
  • Which axis does the bottom right indicator map to? Can the user change it?
  • Which axis does the query system map to? Can the user change it?
  • Generally, any API that returns information related to the plot range has similar issues (e.g. GetPlotRange , PixelsToPlot, etc.). Probably the easiest thing is just to add a int y_axis = 0 parameter to these functions.
  • The signature of BeginPlot is a bit more obnoxious than it already was, but I guess it's not a big deal if all the parameters have default values. Maybe there's a better way to handle flag setting.
  • Often, one y-axis will be on the left, and the other will be on the right (e.g. MATLAB). How should we render the axes if three are supported? All-left? Will this get cluttered? Can users specify multiple axis labels? Is this reason alone to only support two?

In general, it looks like you did a pretty good job finding all the places that code needed to be changed or variables needed to be added. Kudos. Please keep posting updates as you make progress. Also, GIFs are always appreciated!

@jpieper
Copy link
Contributor

jpieper commented May 9, 2020

That sounds reasonable to me for when input is done while hovering the main plot area. For input done over the axes labels, I think each should have an independent hit box.

Agreed.

A couple comments/questions that come to mind (stream of consciousness, apologies):

  • Are we going to support multiple x-axes? My hunch is that it's fairly uncommon and probably not worth it, but in may be good to design this system as abstractly as possible so that we can add it on later if really needed.

I wasn't planning on it, and haven't seen it supported in other tools, like matplotlib. In any event, I don't think this work would preclude later generalization to more than one x axis.

  • Three axes max seems reasonable to me. How common is it to use more than two, though?

Heh. I use 3 pretty often, and have actually used 4 on occasion, although I can usually live without with more contortions. Granted, more than 2 (and more than 1) can also be somewhat emulated by having multiple separate plots with linked x axes. That just makes it harder sometimes to see the relations if you can't squish things around to be appropriately close together.

  • I like the use of ImPlotFlags to specify multi-axis, but I'm curious what happens if ImPlotFlags_Y3Axis is passed without ImPlotFlags_Y2Axis? Is there a sleeker way to handle this?

I was planning on just having them enable independently. In that regime, it would be admittedly somewhat silly to select y3 without y2. I'm not sure any alternatives I can think of are any better:

  • Throw an error if y3 is enabled without y2
  • Make the flags be mutually exclusive and mean "2 axes" and "3 axes"
  • Add yet another parameter to BeginPlot with number of y axes
  • Which axis does the bottom right indicator map to? Can the user change it?

I was planning to have it display all enabled axes simultaneously, like "%f,%f,%f,%f" if all 3 axes were enabled.

  • Which axis does the query system map to? Can the user change it?

I admit to not knowing the rationale for the query system, and was planning on leaving it stuck to the first y axis at the moment.

  • Generally, any API that returns information related to the plot range has similar issues (e.g. GetPlotRange , PixelsToPlot, etc.). Probably the easiest thing is just to add a int y_axis = 0 parameter to these functions.

PixelsToPlot in this patch uses the axis as currently selected by SetPlotYAxis. I haven't yet gotten around to fixing all the APIs, like GetPlotRange yet. Do you think it would be useful to be able to switch the queried axis independently of the SetPlotYAxis setting?

  • The signature of BeginPlot is a bit more obnoxious than it already was, but I guess it's not a big deal if all the parameters have default values. Maybe there's a better way to handle flag setting.

Yeah, I don't have great ideas there at the moment.

  • Often, one y-axis will be on the left, and the other will be on the right (e.g. MATLAB). How should we render the axes if three are supported? All-left? Will this get cluttered? Can users specify multiple axis labels? Is this reason alone to only support two?

I was planning on having the second and third y axis be hard coded to the right hand side for now. If you have 3 and are rendering the values, it will be cluttered, but if you need 3, then I figure it doesn't matter that much if it is ugly?

In general, it looks like you did a pretty good job finding all the places that code needed to be changed or variables needed to be added. Kudos. Please keep posting updates as you make progress. Also, GIFs are always appreciated!

Thanks for the feedback!

Would you prefer this discussion be pulled out into a separate issue to keep this more general one uncluttered?

@epezent
Copy link
Owner Author

epezent commented May 9, 2020

Yep, let's move this discussion. I replied to your comments here: #20

@epezent epezent added the help wanted Extra attention is needed label May 9, 2020
@epezent
Copy link
Owner Author

epezent commented May 11, 2020

Closing. I may start a v0.2 thread later.

@epezent epezent closed this as completed May 11, 2020
epezent pushed a commit that referenced this issue Aug 24, 2020
Draw query rect only when ImPlotFlags_Query is set.
epezent pushed a commit that referenced this issue Feb 3, 2021
DatePicker separate disabled color
Ben1138 pushed a commit to Ben1138/implot that referenced this issue Oct 2, 2024
Ben1138 pushed a commit to Ben1138/implot that referenced this issue Oct 2, 2024
Draw query rect only when ImPlotFlags_Query is set.
Ben1138 pushed a commit to Ben1138/implot that referenced this issue Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants