Variadic Styling Arguments

[epezent]

ImPlot has always attempted to mimic ImGui's API design as much as possible. I generally think that's been a good decision, but one area where I feel we may have taken this too far is in styling plot items. We currently offer two patterns lifted from ImGui:

ImPlot::PushStyleColor(ImPlotCol_Line, red);
ImPlot::PlotLine("Red Line",x,y);
ImPlot::PopStyleColor();

or

ImPlot::SetNextLineStyle(green);
ImPlot::PlotLine("Green Line",x,y);

Not only is that a bit verbose and strange if you're coming from other plotting libraries, but it is currently limiting the extent to which items can be customized with additional arguments (see #298, #321, #185, #248). In particular, there are some cases where item styling should be based on buffers of data (e.g. marker sizes or colors #298), and I don't think either API above lends itself well to that need (e.g. ImPlot::SetNextMarkerStyle(const float* sizes, const ImVec4* colors) is a bit smelly).

I would love to just put all styling arguments in the function call to PlotX, but having a bunch of default args or overloads isn't very attractive either. Passing a styling struct is of course an easy solution, but like I noted in the Setup API proposal (#272), this is IMHO not the ImGui way.

I think the best design would be one that directly mimics MATLAB and/or matplotlib, where styling arguments can be passed in any order or quantity:

plot(x,y,'Color',[1 0 0 1],'Marker','x','MarkerFaceColor',[1 1 0 1],'MarkerSize',5) % MATLAB

plot(x,y,color='red',marker='X',markerfacecolor='yellow',markersize=5) # matplotlib

MATLAB's style can be emulated in C++ using varadic template args and recursion:

typedef int ImPlotSpec;

enum ImPlotSpec_ {
    ImPlotSpec_LineWeight,
    ImPlotSpec_LineColor,
    ImPlotSpec_FaceColor,
    ImPlotSpec_EdgeColor,
    ImPlotSpec_Marker,
    ImPlotSpec_MarkerSize,
    ImPlotSpec_MarkerFaceColor,
    ImPlotSpec_MarkerEdgeColor,
    ImPlotSpec_Stride,
    ImPlotSpec_Offset,
};

struct ImPlotSpecData {
    float LineWeight = 1.0f;
    ImVec4 LineColor = IMPLOT_AUTO_COL;
    ImVec4 FaceColor = IMPLOT_AUTO_COL;
    ImVec4 EdgeColor = IMPLOT_AUTO_COL;
    ImPlotMarker Marker = ImPlotMarker_None;
    float MarkerSize = 4.0f;
    ImVec4 MarkerFaceColor = IMPLOT_AUTO_COL;
    ImVec4 MarkerEdgeColor = IMPLOT_AUTO_COL;
    int    Stride = 1;
    int    Offset = 0;
};

void BuildSpec(ImPlotSpecData& sd, ImPlotSpec spec, float v) {
    switch (spec) {
        case ImPlotSpec_LineWeight : sd.LineWeight = v; break;
        case ImPlotSpec_MarkerSize : sd.MarkerSize = v; break;
    }
}

void BuildSpec(ImPlotSpecData& sd, ImPlotSpec spec, int v) {
  ...
}

void BuildSpec(ImPlotSpecData& sd, ImPlotSpec spec, ImVec4 v) {
  ...
}

template <typename Arg, typename ...Args>
void BuildSpec(ImPlotSpecData& sd, ImPlotSpec spec, Arg arg, Args... args) {
    BuildSpec(sd, spec, arg);
    BuildSpec(sd, args...);
}

template <typename T>
void PlotLine(const char* label, float* xs, float *ys, int count, const ImPlotSpecData& sd);

template <typename T, typename ...Args>
void PlotLine(const char* label, const T* xs, const T* ys, int count, Args... args) {
    ImPlotSpecData S;
    BuildSpec(S, args...);
    PlotLine(label, xs, ys, count, S);
}

Finally, we can do the following:

PlotLine("MyLine", x, y, 10, ImPlotSpec_LineColor, ImVec4(1,0,0,1), ImPlotSpec_Marker, ImPlotMarker_Cross, ImPlotSpec_MarkerFaceColor, ImVec4(1,1,0,1), ImPlotSpec_MarkerSize, 5.0f);

And with a little bit more effort using compile time string hashing, we could even do the following wihtout any runtime cost:

PlotLine("MyLine", x, y, 10, "LineColor", ImVec4(1,0,0,1), "Marker", ImPlotMarker_Cross, "MarkerFaceColor", ImVec4(1,1,0,1), "MarkerSize", 5.0f);

You may notice we needed a struct argument in the end after all. That's not necessarily bad, since it enables those who prefer it and would be needed for C binding generators.

I'm not 100% sold on this idea yet, but perhaps somewhere in all this is a better solution than what we currently have. I'd love to get the community's feedback on this, or hear other ideas.

Cheers!

Looping in the usual suspects: @marcizhu @ocornut @rokups @bear24rw @PeterJohnson @hoffstadt @ozlb @sergeyn @mindv0rtex @JoelLinn @sonoro1234 @JaapSuter @ChiefyChief23 @leeoniya

[sonoro1234]

My only concern is related to #126, so that variadic arguments in C cant be given as a variadic in C++ without va_
Another concern is related to template instantiation which must be done before C wrapping, where the number of instantiations could be huge.

[epezent]

I could not find an elegant way to make C varadic arguments work here without explicitly specifying the number of styling args (either via int count or a printf like specifier string) or using a sentinal value (e.g. ...,arg,arg,NULL); ).

I think the best option for cimplot would be to ignore the fancy varadic API and only wrap the struct version:

template <typename T>
void PlotLine(const char* label, T* xs, T*ys, int count, const ImPlotSpecData& sd);

Thoughts?

[sergeyn]

IMHO with struct as you put it is a must-have interface. Fancy api can be added later or on individual basis, either in terms of overloading PlotLine or extending ImPlotSpecData - you choose, many won't care :).

[sonoro1234]

with just the struct version this can be done from LuaJIT:

PlotLine("MyLine", x, y, 10, {LineColor = ig.ImVec4(1,0,0,1), Marker = ig.lib.ImPlotMarker_Cross, MarkerFaceColor = ig.ImVec4(1,1,0,1), MarkerSize = 5.0});

So that variadic templates could be skipped. I dont know what happens with other languajes.

[sergeyn]

I would like to have struct variant available as I want to remember all the styling info in ONE variable and be able to shove it into any plot. It's nice If that struct has a fancy constructor with variadic template but only if it doesn't introduce hit on the build times.

[marcizhu]

I feel like the proposed API is really good: on one hand, it finally adds the struct parameter that many users were asking for so long. This simple addition might allow some users to cache or reuse a ImPlotSpecData object for many plots, which I think it's pretty cool. On the other hand, this adds a flexible API similar to MATLAB's or matplotlib's while only using a minimal amount of template functions (and thus keeping compile times low as well). I like the idea of being able to supply either ImPlotSpec_* or strings for the name of the argument, but I understand that means maintaining two interfaces for the same functionality and might not be worth it, so in the end if we have to choose between strings or ImPlotSpec_* constants, I'd have no issue and personally either approach is good for me.

However I'd like to propose making all the BuildSpec() functions constexpr, so that the ImPlotSpecData object can be built at compile time. It should be a fairly simple thing to add but it might improve the speed of the library a bit. Besides that, I have no other concerns and I personally believe this change will add lots of possibilities to the library. Good job! 😄

[PeterJohnson]

For several of my use cases, it's important all of the styling parameters be customizable at runtime, so I'll likely only ever use the struct variant and will likely not use the varargs one.

[rokups]

Variadic template arguments hide types. IDEs are unable to provide contextual information in these cases. It would force users unfamiliar with codebase to visit a documentation site to get information that normally would be available in auto-complete. I personally would just use ImPlotSpecData struct directly and fill it with required parameters. Dumb, unexciting code that would be easy to comprehend by other people.

[JoelLinn]

I like the c++ api and think it would map well to python, but requires a struct api to do so because I can't instantiate all the template combinations 🙈. But I can parse the python argument list and build up a struct.

I partially agree with @rokups but that is a general c++ template problem. C++ beginners struggle with templates but with good annotation/comments in source/headers its easy to use for intermediates and up

[rokups]

I bumped into a vaguely relevant issue just now. I am working on a thing written in python so i wrapped implot with SWIG. Everything seemed to work initially, but eventually application started crashing for no apparent reason. I tracked issue down to use of freed memory buffers. Turns out implot often expects that strings passed to APIs like BeginPlot/SetupAxes/SetupAxisFormat/PlotBars should be persistent, which of course is not the case with other languages that may use a different internal string format and need to convert each string before passing it to API and free temporary memory afterwards. Maybe spec builder API could take this usecase into consideration and remove dependency of static persistent data as much as possible. Better yet if core APIs evolved this way as well. Dear ImGui does not depend on buffer persistence across the calls so this would be a nice consistency between two projects.

Edit:
Seems like i misinterpreted what i saw and implot does not actually expect these strings to be persistent, which is terrific! Turns out issue was something else which i fixed and submitted PR (#365) to redeem myself for confusion and spam. Still, extra thought into accommodating scripting languages would be definitely useful 🙏

[epezent]

I have implemented this functionality in #519. Would appreciate some eyes on this before landing (@PeterJohnson, @sergeyn, @sonoro1234, @rokups, @marcizhu , @ocornut)

Cheers!