Add ImPlotSpec and remove existing plot item styling mechanisms

[epezent]

This PR implements the new item styling proposal in #330, i.e. using structs passed to ImPlot::PlotX functions:

ImPlotSpec

1. New enum and structs:

enum ImProp_ {
    ImProp_LineColor,  // line color (applies to lines, bar edges, marker edges); IMPLOT_AUTO_COL will use next Colormap color or current item color
    ImProp_LineWeight, // line weight in pixels (applies to lines, bar edges, marker edges)
    ImProp_FillColor,  // fill color (applies to shaded regions, bar faces, marker faces); IMPLOT_AUTO_COL will use next Colormap color or current item color
    ImProp_FillAlpha,  // alpha multiplier (applies to FillColor)
    ImProp_Marker,     // marker type; specify ImPlotMarker_Auto to use the next unused marker
    ImProp_Size,       // size of markers (radius), error bar whiskers (width or height), and digital bars (height) *in pixels*
    ImProp_Offset,     // data index offset
    ImProp_Stride,     // data stride in bytes; IMPLOT_AUTO will result in sizeof(T) where T is the type passed to PlotX
    ImProp_Flags       // optional item flags; can be composed from common ImPlotItemFlags and/or specialized ImPlotXFlags
};

...

struct ImPlotSpec {
    ImVec4          LineColor  = IMPLOT_AUTO_COL;       // line color (applies to lines, bar edges, marker edges); IMPLOT_AUTO_COL will use next Colormap color or current item color
    float           LineWeight = 1.0f;                  // line weight in pixels (applies to lines, bar edges, marker edges)
    ImVec4          FillColor  = IMPLOT_AUTO_COL;       // fill color (applies to shaded regions, bar faces, marker faces); IMPLOT_AUTO_COL will use next Colormap color or current item color
    float           FillAlpha  = 1.0f;                  // alpha multiplier (applies to FillColor)
    ImPlotMarker    Marker     = ImPlotMarker_None;     // marker type; specify ImPlotMarker_Auto to use the next unused marker
    float           Size       = 4;                     // size of markers (radius), error bar whiskers (width or height), and digital bars (height) *in pixels*
    int             Offset     = 0;                     // data index offset
    int             Stride     = IMPLOT_AUTO;           // data stride in bytes; IMPLOT_AUTO will result in sizeof(T) where T is the type passed to PlotX
    ImPlotItemFlags Flags      = ImPlotItemFlags_None;  // optional item flags; can be composed from common ImPlotItemFlags and/or specific ImPlotXFlags where X corresponds
     ...
}

2. Usage Option 1 -- By declaring and defining a struct instance:

ImPlotSpec spec;
spec.LineColor = ImVec4(1,1,0,1);
spec.LineWeight = 1.0f;
spec.FillColor = ImVec4(1,0.5f,0,1);
spec.FillAlpha = 0.5f;
spec.Marker = ImPlotMarker_Square;
spec.Size = 6;
spec.Stride = sizeof(ImVec2);
spec.Flags = ImPlotItemFlags_NoLegend | ImPlotLineFlags_Shaded;
ImPlot::PlotLine("Line 1", &data1[0].x, &data1[0].y, 20, spec);

3. Usage Option 2 -- Inline using ImProp,value pairs (order does NOT matter):

ImPlot::PlotLine("Line 2", &data2[0].x, &data2[0].y, 20, {
    ImProp_LineColor, ImVec4(0,1,1,1),
    ImProp_LineWeight, 1.0f,
    ImProp_FillColor, ImVec4(0,0,1,1),
    ImProp_FillAlpha, 0.5f,
    ImProp_Marker, ImPlotMarker_Diamond,
    ImProp_Size, 6,
    ImProp_Stride, sizeof(ImVec2),
    ImProp_Flags, ImPlotItemFlags_NoLegend | ImPlotLineFlags_Shaded
});

4. Result:

Other API Changes

• SetNextLineStyle, SetNextFillStyle, SetNextMarkerStyle, and SetNextErrorBarStyle have been removed; pass styling variables directly to PlotX functions now with ImPlotSpec
• ImPlotCol_Line, ImPlotCol_Fill, ImPlotCol_MarkerOutline, ImPlotCol_MarkerFill, ImPlotCol_ErrorBar have been removed and thus are no longer supported by PushStyleColor. You can use a common ImPlotSpec instance across multiple PlotX calls to emulate PushStyleColor behavior.
• ImPlotStyleVar_LineWeight, ImPlotStyleVar_Marker, ImPlotStyleVar_MarkerSize, ImPlotStyleVar_MarkerWeight, ImPlotStyleVar_FillAlpha, ImPlotStyleVar_ErrorBarSize, and ImPlotStyleVar_ErrorBarWeight have been removed and thus are no longer supported by PushStyleVar. You can use a common ImPlotSpec instance across multiple PlotX calls to emulate PushStyleVar behavior.
• ImPlotStyle::/ImPlotStyleVar_ DigitalBitGap as renamed to DigitalSpacing; DigitalBitHeight was removed (use ImPlotSpec::Size); DigitalPadding was added for padding from bottom.
• PlotX offset, stride, and flags parameters are now incorporated into ImPlotSpec; specify these variables in the ImPlotSpec passed to PlotX.
• ImPlotSpec collapses styling parameters what were previously distinct, i.e. LineWeight applies to lines, markers, and bar edge size; FillColor applies to marker fills, shaded regions, bar faces; etc. This means that in some cases the user will not have independent control of e.g. shaded region fill and marker fill (as in the example above). This can be overcome by using multiple PlotX function calls (e.g. the example above could be a PlotLine + PlotShaded + PlotMarkers for full control). I did this to keep the size of ImPlotSpec minimal. TBD if this was the right call; willing to consider adding additional variables to ImPlotSpec for finer control sometime in the future (e.g. MarkerFillColor).

These changes will be frustrating to some users, but I cannot reasonably support three different styling paths (SetNext, PushStyleVar, and now ImPlotSpec). ImPlotSpec is easier to use, conforms with other plot libraries, and will be simpler to extend in the future.

Test Plan

• Updated implot_demo.cpp to use new ImPlotSpec throughout. Confirmed that all demo examples still function and look as before.

Future Plans

• Support per-index color (and maybe size) via ImPlotSpec as was demonstrated in add SetNextColorsData to give per point colors #332 by @niavok

[epezent]

@PeterJohnson, @sergeyn, @sonoro1234, @rokups, @marcizhu , @ocornut, @bear24rw, @ozlb -- I would appreciate some eyes on this. Please feel free to leave comments or critiques.

[PeterJohnson]

I like the new API. Just wanted to provide some code comments.

[ozlb]

Thanks for considering me!
Most annoying issue for me so far, is that DigitalPadding and DigitalSpacing are now defined in style and so common for all plots.
Beside this note, refactoring was straight forward.

I’m using Usage Option 1

[ocornut]

I'm not equipped to provide overall feedback, but some small bits:

• Assuming this is a breaking change, I would suggest providing a couple of explicit example of "before" vs "after" code.
• The naming ImProp feels a bit too globally could it be ImPlotProp ? (if dear imgui ends up in a similar direction in the future we'd always have a change to merge the enums/types later)
• Good to spend a bit of time considering how non-C++ language wrapping would interact with the API (maybe there's no problem here?), and if there's be an decent path for e.g. Dear Bindings to generate the corresponding metadata for implot.
• Wouldn't Designated Initializers solve Option 2 more naturally? (never used them in C++, just wondering)
• That's a personal committing guideline I use and you may not care about, but on large refactor I like to manually push all chunks for shallow parts (typo fixes, constexpr bits, comments) in a separate commit, so the big commit ends up as noise-less as possible. It also makes it easier to review PRs.

[PeterJohnson]

Most annoying issue for me so far, is that DigitalPadding and DigitalSpacing are now defined in style and so common for all plots.

Somehow I missed that on my first read-through. That's a problem for me as well, as it sounds like this means it's no longer possible to arbitrarily vertically position individual digital plots on a per-line basis? Or is there a "new" way to do this?

[epezent]

@ocornut

The naming ImProp feels a bit too globally could it be ImPlotProp?

Yea, I was torn on this one for the same reasons. I leaned more toward ImProp since e.g. ImPlotProp_LineColor is a bit much for something that I intend people to use frequently. I'm open to change if it if folks here don't mind the extra 4 characters, or if there's a more concise name anyone can think of.

Relatedly, I experimented with compile time conversion of string literal to ImProp, such that this was possible:

ImPlot::PlotLine("Line 2", &data2[0].x, &data2[0].y, 20, {
    "LineColor", ImVec4(0,1,1,1),
    "LineWeight", 1.0f,
    "FillColor", ImVec4(0,0,1,1),
    "FillAlpha", 0.5f,
    "Marker", ImPlotMarker_Diamond,
    "Size", 6,
    "Stride", sizeof(ImVec2),
    "Flags", ImPlotItemFlags_NoLegend | ImPlotLineFlags_Shaded
});

I like it because it feels more like other plotting libraries (MATLAB/matplotlib), but I dislike it because it's unlike anything else in ImGui. Anyway, I abandoned it because also couldn't find a way to guarantee compile time evaluation in all scenarios. Could spend more time on it if folks like it. Thoughts?

Good to spend a bit of time considering how non-C++ language wrapping would interact with the API

With the default struct method, I think we are fine? @sonoro1234 what do you make of this?

Wouldn't Designated Initializers solve Option 2 more naturally?

It's a C++20 feature, unfortunately.

@ozlb , @PeterJohnson

sounds like this means it's no longer possible to arbitrarily vertically position individual digital plots on a per-line basis?

It's still possible to change the padding from the bottom and the spacing between digital plots with PushStyleVar(ImPlotStyleVar_DigitalPadding) and PushStyleVar(ImPlotStyleVar_DigitalSpacing), respecively. My thinking was that these wouldn't require frequent modification from users, whereas the height of each digital plot might (which is controlled via ImPlotSpec::Size). I don't use PlotDigital much myself, so let me know if this is problematic. What I do want to avoid is adding digital plot specific variables to ImPlotSpec, for now (hence my decision to collapsing DigitalBitHeight into ImPlotSpec::Size).

[sonoro1234]

With the default struct method, I think we are fine? @sonoro1234 what do you make of this?

Yes, I guess that with default struct method is ok.

[PeterJohnson]

I'm open to change if it if folks here don't mind the extra 4 characters, or if there's a more concise name anyone can think of.

I'm fine with a slightly longer name to avoid potential naming conflicts.

Could spend more time on it if folks like it. Thoughts? [Designated initializers] are a C++20 feature, unfortunately.

I personally wouldn't use either the string or pairs approach, as I'm using C++20. Strings sound not particularly performant.

Speaking of performance, one thing to note about large structs initialized through a default arguments (or via designated initializers) is that with many compilers this actually causes codegen to occur at each call site to fill out the struct, even though it's actually constant (the compiler won't generate a common constant struct in .rodata and just pass a pointer). It looks like ImPlotSpec could get quite large and be passed as a default lots of places, so that may cause a surprising amount of codegen. There's a complex reason for this (https://reddit.com/r/cpp/s/ezrziiv9NM has some discussion), but the workaround I found was to declare a constexpr MyStruct kDefaultStruct; and pass kDefaultStruct as the default argument value instead of MyStruct().

I don't use PlotDigital much myself, so let me know if this is problematic.

As long as it's still possible to set separately for each series via PushStyle, that works for me.

[rokups]

Struct approach is good, this is what i would use as well. With variadic args approach and enums for parameters we still do not get hints about what type such parameter accepts unless we compile. With strings we do not get autocompletion for parameters too. Simplest approach is the best and that would be structs, with designated initializers, whenever we graduate to appropriately high standard.

[ozlb]

@ocornut

The naming ImProp feels a bit too globally could it be ImPlotProp?

Yea, I was torn on this one for the same reasons. I leaned more toward ImProp since e.g. ImPlotProp_LineColor is a bit much for something that I intend people to use frequently. I'm open to change if it if folks here don't mind the extra 4 characters, or if there's a more concise name anyone can think of.

Probably bothImAxis and ImProp (and related enums) are now eligible to ImPlot prefix

[ocornut]

There’s ImGuiAxis in the main codebase you could use that or decide its more consistent for users to see ImPlotAxis

[PeterJohnson]

I don't use PlotDigital much myself, so let me know if this is problematic. What I do want to avoid is adding digital plot specific variables to ImPlotSpec, for now (hence my decision to collapsing DigitalBitHeight into ImPlotSpec::Size).

It might make sense to create a ImPlotDigitalSpec struct to hold digital-specific settings (it could even be derived from ImPlotSpec so there's still only a single parameter to PlotDigital)? The digital padding/spacing/height thing is kind of an oddity right now in that it affects behavior across multiple series and is done in pixel coordinates. I think spacing is really only useful if you're doing a lot of digital signals in one plot, and even then, the user could relatively easily compute this themselves? It might be less confusing to just remove the padding feature entirely and just have a ImPlotDigitalSpec struct with a Y-axis start value? This could also be a good time to think about whether pixel coordinates make sense for the start and height, or whether changing to something like % of plot height would be better.

A more generic solution overall could be #321 (comment) (in which case digital plots could always be full scale), but that major of a change is probably better done as a separate development effort.

[marcizhu]

First of all, thanks for having me into consideration! And sorry for the super late reply, I've been meaning to reply to this for ages, but I haven't had the time until now.

I think the most important points have been raised already, but just to touch on a few things I'd like to point out that the second API feels more natural (kind of like what MATLAB, Python or other languages do, as you mentioned) while the first option is probably more performant, due to not having to iterate over the initializer list. Also, I haven't looked at the implementation for the first API, but if it takes in an rvalue reference it'd allow to use designated initializers, just like @ocornut mentioned, which is a big plus for people using C++20 and newer.

About the naming convention, I too would prefer ImPlotProp over ImProp, as the former is clearly part of ImPlot while the latter could perfectly be part of ImGUI or any other third-party library running on top of ImGUI. I have the strong opinion that nowadays, where everyone is using IDEs and code assistants and alike, a few more characters doesn't make a big difference since the autocomplete will still save you more characters than you have to type.

Another topic I see mentioned quite a few times is the digital plot and its style spec. Since the digital plot is completely different to a line/bar/pie chart, maybe it does make sense to have its style in a different struct. However, I have no strong opinions on this, so if this remains as-is it's fine by me. I still remember some conversations a while ago about doing some rework on the digital plot (issue #321 iirc) so I guess this change could also be made as part of that enhancement, since it would probably add tons of new styling options which might better justify having on its own struct.

That's pretty much all I wanted to say. Congrats on this PR! I think it's a nice step forward, and I'm very happy to see progress still being made on this awesome project! 😄

[Nahor]

(I commented in the associated discussion thread but I don't know how much visibility that discussion has)

What about using C++ types instead of an enum (example)? It's a bit more complicated/verbose on the implot side, but I think it's easier on the application side (less error prone, and more IDE friendly).

[Nahor]

Here is another option. It's similar to my previous proposal, but the "spec argument builder" are fully separated from the ImPlotSpec. This makes ImPlotSpec independent from the "builder functions" and allow an app to have its own "builder functions" if desired (e.g. to dynamically compute the color of a plot, or the line thickness), basically the equivalent of ImPlotGetter for ImPlotSpec. This also allows to have a generic "builder function" template and simplify the addition of new fields in ImPlotSpec.

Examples:

• Defining a new field using the generic template:

  typedef ImPlotSpec_Generic<ImVec4, offsetof(ImPlotSpec, LineColor)> ImPlotSpec_LineColor;

• Defining a custom builder function:

  struct ImPlotSpec_AutoLineWeight {
     static constexpr void assign(ImPlotSpec &spec, ImPlotSpec_LineWeight v) {
          spec.LineWeight = ImPlot::IsPlotHovered() ? 3.0f : 1.0f;
      }
  };

  which can be used like this:

  ImPlot::PlotLine(..., {ImPlotSpec_AutoLineWeight{}});

  which I think is nicer than:

  ImPlot::PlotLine(..., {ImProp_LineWeight, ComputeLineWeight()});