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

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

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.

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?

@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).

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.

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.

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.

@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

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

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.

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! 😄

First, I'd like to thank @epezent for his ongoing work and dedication, which has made ImPlot so successful.
Second, as a user, I would like to offer my opinion on the design decision regarding styling mechanisms.

Option 1, using the pure struct approach, is the most straightforward approach to the problem.
As @ocornut points out, using designated initializers for setting field values is also a sufficient mechanism and does not introduce any C++ features that could hinder porting the functionality to other languages.

I would refactor it slightly to exclude data access parameters and flags to represent the pure styling parameters.
This could also be used as a solution to issue #332, which attempts to extend the Callback versions of plot procedures to allow for modifying the color based on the primitive's index.

I would approach the problem by having the user supply a styling struct upon invoking the plot procedure, then, in the callback, supply a reference to the style struct to be modified by the user if the user wants. This way, any relevant parameter such as color, size, or marker could be altered without having a combinatorial explosion of potential Getters.

struct ImPlotStyle {
    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*
     ...
}

And a use case example:

ImVec2 pos[20] = {...};
float size[20] = {...};

struct Data {
    const ImVec2* pos;
    const float* size;
    ImPlotColormap map;
};

Data data = {
    .pos = pos,
    .map = ImPlotColormap_Plasma,
};

ImPlotLineFlags flags = 0;
ImPlotStyle style = {
    .Size = 10.f,
};

// Plot lines using style
ImPlot::PlotLine("Line 1", &data.pos[0].x, &data.pos[0].y, 20, flags, 0, sizeof(ImVec2), style);

// Example with getter
auto getter = [](int idx, ImPlotStyle& style, void* user_data) -> ImPlotPoint {
    Data* data = (Data*)user_data;
    style.LineColor = ImPlot::GetColormapColor(idx, data->map);
    style.Size = data->size[idx];
    return {data->pos[idx].x, data->pos[idx].y};
};

// Plot lines using getter
ImPlot::PlotLineG("Line 2", getter, data, 20, flags, style);
...

In this design, passing the style parameter as const ImPlotStyle& makes sense, which is set to {} by default.
Then, in the case of PlotLineG, it is internally copied to allow for modification when iterating over items.

I'd really love to know @epezent's take on whether more time has provided clarity on which direction to go, and where #332 fits into this. Styling lines and scatters with different sizes and colors would be a really excellent and powerful feature, especially for something like ImPlot/ImGui and its robust support for realtime interactivity.

I just finished merging the changes from the #332 PR into a local branch of mine so that I can go ahead and use this functionality, but I would really love to contribute functionality to ImPlot in any way I can. Unfortunately, without clear guidance from maintainers such as @epezent / @ocornut, I'm a little apprehensive to go too far down the wrong path only to have a different solution/direction adopted by the main repository.

My main goal is to use this feature to color one line based on the value of another. The approach from #332 has worked so far, coloring the line in the upper right panel based on the distance between the white and red lines in the upper left plot. This is in an effort to write a portable, interactive viewer for weather data used by the NOAA Storm Prediction Center. If in my efforts to write this application, I can provide testing, source code contributions, and ideas, I'd really like to help push this forward if possible -- just point me in the right direction!

@keltonhalbert Thank you for reaching out. I commented on the status of ImPlot recently in #585 (comment) and the situation remains the same -- I am not actively contributing or addressing PRs at this time, but I am openly welcoming any serious contributors/maintainers to take lead. If you are interested in discussing this, please do not hesitate to reach out via email -- Evan

@keltonhalbert Thank you for reaching out. I commented on the status of ImPlot recently in #585 (comment) and the situation remains the same -- I am not actively contributing or addressing PRs at this time, but I am openly welcoming any serious contributors/maintainers to take lead. If you are interested in discussing this, please do not hesitate to reach out via email -- Evan

Thanks Evan! I'll absolutely be reaching out to you... I've only been spinning myself up on ImGui/ImPlot over the last few months, but I am very interested and invested in seeing this library continue to progress forward.