Replies: 9 comments 14 replies
-
Heyo, thanks for the inclusion in the original list! Have you considered instead of having individual functions to set up everything just pass the data is via structs? So instead of bool BeginPlot(const char* title_id,
const char* x_label = NULL,
const char* y_label = NULL,
const ImVec2& size = ImVec2(-1,0),
ImPlotFlags flags = ImPlotFlags_None,
ImPlotAxisFlags x_flags = ImPlotAxisFlags_None,
ImPlotAxisFlags y_flags = ImPlotAxisFlags_None,
ImPlotAxisFlags y2_flags = ImPlotAxisFlags_NoGridLines,
ImPlotAxisFlags y3_flags = ImPlotAxisFlags_NoGridLines,
const char* y2_label = NULL,
const char* y3_label = NULL); we could have an API like bool BeginPlot(const char* title_id,
const ImVec2& size = ImVec2(-1,0),
ImPlotLabelStruct labels = ImPlotLabels(),
ImPlotFlags flags = ImPlotFlags_None,
ImPlotAxisFlagsStruct axis_flags = ImPlotAxisFlagsStruct()); with struct ImPlotLabelStruct
{
const char* x_label = NULL,
const char* y_label = NULL,
const char* y2_label = NULL,
const char* y3_label = NULL,
}
struct ImPlotAxisFlagsStruct
{
ImPlotAxisFlags x_flags = ImPlotAxisFlags_None;
ImPlotAxisFlags y_flags = ImPlotAxisFlags_None;
ImPlotAxisFlags y2_flags = ImPlotAxisFlags_NoGridLines;
ImPlotAxisFlags y3_flags = ImPlotAxisFlags_NoGridLines;
}
Obviously, the names can be changed to what ever is needed, but it is an additional option to not make any massive changes and reduce the size of the Begin API. Or I may have missed the main issue of the discussion! |
Beta Was this translation helpful? Give feedback.
-
I don't know ImPlot intimately enough to have a worthy opinion, but it looks like you've explored this enough so my gut feeling is that the SetupXXX systems like Tables API would be good. It is also nice that it mimics an existing API. Note that a byproduct of having more function calls is you can consider stacking more arguments to them, so if you feel there are strong "common use" contenders for a The other advantage of those multiple Setup functions is if some API needs to change/break again in the future, it'll probably easier to work on that transition for one specific function than for a big BeginXXX function. @AdrianoParisi23 is right that using structures with a default constructor is also good. I don't use this pattern in Dear ImGui show, but I've not nothing against it. It's more future proof but makes basic usage more verbose.
Sounds very much out of question! The more you have to hold on the more code and maintenance it'll be, and it's also likely going to be slower. |
Beta Was this translation helpful? Give feedback.
-
First of all thanks for taking my opinion into consideration! 😄 Let me start by saying that I love this proposal for a few reasons. The main reason is that as a user of ImPlot I often found myself having to constantly look up the function signature of Finally, another reason why I love this is performance: C and C++ do not have a limit on function parameters, so a function can have as many as you like, but there's a catch: after a certain amount of parameters (basically when you run out of registers), parameters will be passed through the stack, which means there will be lots of The Order of
|
Beta Was this translation helpful? Give feedback.
-
To me, this problem looks like a perfect fit for the builder pattern. You'd make a separate type responsible for accumulating various configuration options, and when you are ready, you'd call a // the basics
if (ImPlot::BeginPlot("Example 2")) {
ImPlot::SetupAxis(ImAxis_X1, "My X-Axis", ImPlotAxisFlags_Time);
ImPlot::SetupAxis(ImAxis_Y1, "My Y-Axis", ImPlotAxisFlags_Invert);
ImPlot::PlotLine("data",x_data,y_data,2);
ImPlot::EndPlot();
} would turn into something like PlotBuilder plot;
plot.Title("Example 2")
.SetupAxis(ImAxis_X1, "My X-Axis", ImPlotAxisFlags_Time)
.SetupAxis(ImAxis_Y1, "My Y-Axis", ImPlotAxisFlags_Invert);
if (ImPlot::BeginPlot(plot)) {
ImPlot::PlotLine("data",x_data,y_data,2);
ImPlot::EndPlot();
} The benefit is that the builder just accumulates options via its public API, which you can enrich as you add new features. There's also no order that these calls have to stick to. Calling |
Beta Was this translation helpful? Give feedback.
-
I also appreciate being tagged in this post. I like the idea of treating the individual components of the plot as such. I'm not very familiar with implot's internals, so I'm not sure if this is viable but may help with idea generation. if (ImPlot::BeginPlot("Example 3",ImVec2(-1,-1), ImPlotFlags_NoTitle)) {
ImPlot::BeginAxis(ImAxis_X, "My X-Axis 1", "%g V");
ImPlot::SetAxisLimits(-10, 10);
ImPlot::EndAxis();
if(ImPlot::BeginAxis(ImAxis_X, "My X-Axis 2", "%.3f mA"))
{
// returns "true" if axis is clicked for a custom context menu
}
ImPlot::SetAxisTicks(0,1,5);
ImPlot::EndAxis();
ImPlot::BeginAxis(ImAxis_Y, "My Y-Axis 1", ImPlotAxisFlags_Opposite);
ImPlot::EndAxis();
ImPlot::BeginAxis(ImAxis_Y, "My Y-Axis 2", ImPlotAxisFlags_Opposite, MyFormatCallback);
ImPlot::SetAxisLimits(0, 10000);
ImPlot::EndAxis();
if(ImPlot::BeginLegend(ImPlotLocation_North, ImPlotLegendFlags_Outside|ImPlotLegendFlags_Horizontal))
{
// returns "true" if legend is clicked for a custom context menu
}
ImPlot::SetAxes(0, 0); // first x and y axes
if(ImPlot::PlotLine("data22",x_data,y_data,2))
{
// returns "true" if legend entry is clicked for a custom context menu
}
ImPlot::EndPlot();
} and the simple case: if (ImPlot::BeginPlot("Example 2")) {
ImPlot::BeginAxis(ImAxis_X, "My X-Axis", ImPlotAxisFlags_Time);
ImPlot::EndAxis();
ImPlot::BeginAxis(ImAxis_Y, "My Y-Axis", ImPlotAxisFlags_Invert);
ImPlot::EndAxis();
ImPlot::PlotLine("data",x_data,y_data,2);
ImPlot::EndPlot();
} |
Beta Was this translation helpful? Give feedback.
-
Thanks for showing interest in my opinion. My 2 cents:
|
Beta Was this translation helpful? Give feedback.
-
Another thing to potentially consider: |
Beta Was this translation helpful? Give feedback.
-
Thanks for the additional feedback everyone. My gut feeling is to press forward with the original proposal for the public API in Hoping to work more on this soon -- life and work have put a hold on progress lately. |
Beta Was this translation helpful? Give feedback.
-
FYI, #294 added the proposed Setup API to the public header (among many other changes -- see #168 (comment)). There is still more work to be done to enable the objective APIs suggested here, but it is much closer than before. @ocornut , where is the "dear bindings" you mentioned currently located. I'm curious if ImPlot's API is conformal as is, or will need some tweaks. |
Beta Was this translation helpful? Give feedback.
-
SEEKING FEEDBACK! IF YOU ARE TAGGED BELOW OR STUMBLED UPON THIS, PLEASE SHARE YOUR THOUGHTS :)
@ocornut , @rokups , @marcizhu, @bear24rw, @PeterJohnson , @hoffstadt. @ozlb, @sergeyn, @mindv0rtex, @JoelLinn, @sonoro1234. @jpieper, @JaapSuter , @ChiefyChief23, @leeoniya
The State of Things
ImPlot continues to evolve, but it has become increasingly apparent that the current API is not flexible enough to accommodate many features that have been requested (#82,#173,#235,#246,#248,#251,#165,#197,#245,#56,#98) or things that I would like to eventually do (e.g. mutli x-axis). Our main culprit is of course
BeginPlot
, which is now a Frankenstein monster of what it started out as:Ugh, it pains me to look at this! Here are the biggest issues:
ImPlotFlags_YAxis2/3
set (should just be automatic)Our second issue is the assortment of
SetNextPlotX
functions that we have, e.g.:I started this pattern because I felt it was analogous to
ImGui::SetNextWindowSize()
and because we need to know such details during the call toBeginPlot
, which sets up the plot render layout and transformations. We now have about a dozen similar functions for setting up various plot/axis parameters. In hindsight, this was a bad decision because 1) we end up with a confusing assortment of API calls that must happen beforeBeginPlot
or afterBeginPlot
, and 2) the pattern does not scale well because each new feature requires managing the lifetime of more state up until the next call toBeginPlot
(i.e.NextPlotData
gets bigger).These two things have bugged me for quite a while, and I have been slow to move on adding new features because both have been somewhat restrictive. Continuing to either add more args to
BeginPlot
or moreSetNextPlotX
functions always seemed like dead ends to me.New
Setup
API ProposalTaking a page out of
ImGui Tables'
book, I am proposing that we reduce the number of arguments required forBeginPlot
, and provide a variety of optionalSetupX
functions that can be called immediately after. The latter would replace the currentSetNextPlotX
API. Here's what this new API may look like:And a few examples:
A Few Gotchas
While I feel pretty strongly that this is the path forward, there are a few things I haven't yet worked out:
The Order of
Setup
CallsPreviously,
BeginPlot
would do all plot render layout (e.g. calculate padding sizes based on axis ranges, labels, titles, etc) so that the bounding rect for the plot region and coordinate transformations were know before we entered anyPlotX
functions. Because we had adopted theSetNextPlotX
pattern,BeginPlot
had all the information it needed to do its job. That's not true with the proposed API, since some calls toSetupX
may necessitate an adjustment to the layout.My current strategy is something similar to ImGui Tables:
BeginPlot
SetupX
calls as they likeFinishSetup
FinishSetup
calculates layout and padding, and locks the plot from anymoreSetupX
calls (runtime assert), essentially doing most of the workBeginPlot
previously didPlotX
FinishSetup
, then the first call to ANY function that requires the layout to be complete (e.g.PlotX
, but also util functions such asPlotToPixels
) will call it automaticallyEndPlot
@ocornut , your feedback here would be super appreciated. This seems like the best option, but you may have a more clever idea! FWIW, another option might be to delay all render until
EndPlot
, but this would require caching an enormous amount of data including the user's plot data. Maybe this is something that could be resolved withImDrawListSplitter
? I don't know...Breaking User Code
Changing
BeginPlot
of course is going to break a lot of people's existing code. Although I have been saying for sometime that anything goes untilv1.0
, I want to make sure that this transition wouldn't be TOO bad for folks. Therefore, we could keep the current version ofBeginPlot
around as a wrapper of the new API, and hang on toSetNextPlotX
API for a while. I would likely place these under#infdef IMPLOT_DISABLE_OBSOLETE_FUNCTIONS
, and remove them when we hit v1.0.Prototype
A really early and quite buggy prototype of this API can found on the
features/setup
branch. Working on this will be my main priority for the next few weeks, and other work such asbackends
GPU rendering will go on hold until this is done.Please share your thoughts and concerns. I'm all ears and would be happy to discuss alternative solutions. Thank you!
Beta Was this translation helpful? Give feedback.
All reactions