-
Notifications
You must be signed in to change notification settings - Fork 530
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
Support for other dataset types #104
Comments
Tadd, first thanks for the compliments and suggestions! I agree, neither approach is perfect. However, we do use templates internally per @ocornut's suggestion. For example: implot.h void PlotLine(const char* label_id, const float* values, int count, int offset = 0, int stride = sizeof(float));
void PlotLine(const char* label_id, const double* values, int count, int offset = 0, int stride = sizeof(double)); implot_items.cpp template <typename Getter>
inline void PlotEx(const char* label_id, Getter getter) {
// a whole bunch of templated function calls inside of here
...
}
void PlotLine(const char* label_id, const float* values, int count, int offset, int stride) {
GetterYs<float> getter(values,count,offset,stride);
PlotEx(label_id, getter);
}
void PlotLine(const char* label_id, const double* values, int count, int offset, int stride) {
GetterYs<double> getter(values,count,offset,stride);
PlotEx(label_id, getter);
} Supporting int16 should be trivial. As long as the type can cast to a void PlotLine(const char* label_id, const short* values, int count, int offset, int stride) {
GetterYs<short> getter(values,count,offset,stride);
PlotEx(label_id, getter);
} As far as supporting integral types in implot.h, I'm just not sure of the best way to proceed here. There are far to many possibilities to explicitly add them all. I suppose we could add an implot.inl file for the template code, but I'm not sure what complications this would bring about for our wrapper maintainers and how it would mesh with the ImGui philosophy. ImGui does something like this for types that are not bool DragScalar(const char* label, ImGuiDataType data_type, void* p_data, float v_speed, const void* p_min = NULL, const void* p_max = NULL, const char* format = NULL, float power = 1.0f); where: enum ImGuiDataType_
{
ImGuiDataType_S8, // signed char / char (with sensible compilers)
ImGuiDataType_U8, // unsigned char
ImGuiDataType_S16, // short
ImGuiDataType_U16, // unsigned short
ImGuiDataType_S32, // int
ImGuiDataType_U32, // unsigned int
ImGuiDataType_S64, // long long / __int64
ImGuiDataType_U64, // unsigned long long / unsigned __int64
ImGuiDataType_Float, // float
ImGuiDataType_Double, // double
ImGuiDataType_COUNT
}; We could potentially do something similar, and leverage this enum. I'm happy to start a discussion about how we can make this more generalized and usable for everyone! Let me know your thoughts, and if @ocornut wants to chime in, that'd be really helpful too! |
Also, @electromaggot, note that you have void PlotLine(const char* label_id, ImPlotPoint (*getter)(void* data, int idx), void* data, int count, int offset = 0); at your disposal. You can write a custom getter function that will convert your data to the |
As a proof of concept using void PlotLine(const char* label_id, const void* values, int count, ImGuiDataType type, int offset, int stride) {
if (type == ImGuiDataType_S8) { GetterYs<ImS8 > getter((ImS8 *)values,count,offset,stride); PlotEx(label_id, getter); }
else if (type == ImGuiDataType_U8) { GetterYs<ImU8 > getter((ImU8 *)values,count,offset,stride); PlotEx(label_id, getter); }
else if (type == ImGuiDataType_S16) { GetterYs<ImS16 > getter((ImS16 *)values,count,offset,stride); PlotEx(label_id, getter); }
else if (type == ImGuiDataType_U16) { GetterYs<ImU16 > getter((ImU16 *)values,count,offset,stride); PlotEx(label_id, getter); }
else if (type == ImGuiDataType_S32) { GetterYs<ImS32 > getter((ImS32 *)values,count,offset,stride); PlotEx(label_id, getter); }
else if (type == ImGuiDataType_U32) { GetterYs<ImU32 > getter((ImU32 *)values,count,offset,stride); PlotEx(label_id, getter); }
else if (type == ImGuiDataType_S64) { GetterYs<ImS64 > getter((ImS64 *)values,count,offset,stride); PlotEx(label_id, getter); }
else if (type == ImGuiDataType_U64) { GetterYs<ImU64 > getter((ImU64 *)values,count,offset,stride); PlotEx(label_id, getter); }
else if (type == ImGuiDataType_Float) { GetterYs<float > getter((float *)values,count,offset,stride); PlotEx(label_id, getter); }
else if (type == ImGuiDataType_Double) { GetterYs<double> getter((double*)values,count,offset,stride); PlotEx(label_id, getter); }
} It goes without saying that this will increase the overall compile size considerably once each plotting function is done this way. |
If we go this route, then each function in our public API may expose:
The alternative is to expose the template code in an inline file, and provide an entirely template API. Again, I don't know what issues this could create for our wrapper maintainers and ports. The public headers of both ImPlot and ImGui do expose templated objects (e.g. ImVector), but in ImGui none of the GUI functions themselves are templated, so my preference would be to do the same. |
Wow, @epezent! Excellent, helpful response and thanks for setting me straight (I should have dug-in more before posting, your patience Evan is appreciated). |
No problem! Let's leave this issue open and see if anyone else has input. I'm generally curious what data types people are using or wanting to use ImPlot for. |
Another possibility is this: implot.h template <typename T>
void PlotLineT(const char* label_id, const T* xs, const T* ys, int count, int offset = 0, int stride = sizeof(T)); implot_items.cpp template <typename T>
void PlotLineT(const char* label_id, const T* xs, const T* ys, int count, int offset, int stride) {
GetterXsYs<T> getter(xs,ys,count,offset,stride);
return PlotLineEx(label_id, getter);
}
template void PlotLineT<ImS8>(const char* label_id, const ImS8* xs, const ImS8* ys, int count, int offset, int stride);
template void PlotLineT<ImU8>(const char* label_id, const ImU8* xs, const ImU8* ys, int count, int offset, int stride);
template void PlotLineT<ImS16>(const char* label_id, const ImS16* xs, const ImS16* ys, int count, int offset, int stride);
template void PlotLineT<ImU16>(const char* label_id, const ImU16* xs, const ImU16* ys, int count, int offset, int stride);
template void PlotLineT<ImS32>(const char* label_id, const ImS32* xs, const ImS32* ys, int count, int offset, int stride);
template void PlotLineT<ImU32>(const char* label_id, const ImU32* xs, const ImU32* ys, int count, int offset, int stride);
template void PlotLineT<ImS64>(const char* label_id, const ImS64* xs, const ImS64* ys, int count, int offset, int stride);
template void PlotLineT<ImU64>(const char* label_id, const ImU64* xs, const ImU64* ys, int count, int offset, int stride);
template void PlotLineT<float>(const char* label_id, const float* xs, const float* ys, int count, int offset, int stride);
template void PlotLineT<double>(const char* label_id, const double* xs, const double* ys, int count, int offset, int stride); It compiles and links under both GCC and MSVC for me. I can call I honestly like this approach the most -- it's more terse, keeps the header clean of template implementation and redundant declarations, and the avoids branching seen above. My only reservations are due to increased compile size (meh, not a big deal) and introducing a templated API (well, sort of). I don't have any qualms with it personally, but as someone who only deals in C++, I'm not familiar with what issues this could bring about for binding maintainers. @sonoro1234 and @ocornut, your insight here would be greatly appreciated! |
@hoffstadt, can you comment as well and let us know how this would impact Dear PyGui |
Thanks for mention. This would not impact Dear PyGui. Although we are not restricted to using floats, we've done so out of consistency with Dear ImGui. Ideally we would like to begin using doubles and ints to avoid all the casting taking place from: Python -> Python C API -> ImGui/ImPlot. I personally think templates in this case would be a good idea and I like the the approach taken in your comment above. Omar may have a more insightful view on potential issues for his users. |
I can convert a LuaJIT function to It happens with LuaJIT but I think it is a general C ffi issue: a C function can return a UDT* type but cant return a UDT type |
@sonoro1234 More specifically I was wondering how having a template based plotting API would impact cimplot (see my last comment with code). |
The generic |
May I ask how much work and would you be willing to implement it? The templated approach would be ideal for most users on our side. |
If it means generating CIMGUI_API void ImPlot_PlotLineT_ImS8(const char* label_id, const ImS8* xs, const ImS8* ys, int count, int offset, int stride)
{
return ImPlot::PlotLineT<ImS8>(label_id, xs,ys, count, offset, stride);
} and similars, it seems quite doable. But explicit instantiation template void PlotLineT<ImS8>(const char* label_id, const ImS8* xs, const ImS8* ys, int count, int offset, int stride); goes in .cpp file Could we have something related in header file? (I am only using header files for parsing) That should be the ideal but if it is not possible then we will need to provide the typenames desired as parameters for the generator |
To avoid cluttering the header, would a persistent comma-separated comment suffice? // Supported Data Types: double, float, ImS32, ImU32, ... Might also be possible with a macro in Ideally every plotting function would adopt this templated approach, so you could assume that each one will support all the data types listed. |
@electromaggot , I've converted about half the plotting API to the proposed template approach. You can check it out in the https://github.com/epezent/implot/tree/templates @sonoro1234 , do you want to give it a look as well and let me know what you think? As expected, the compile size increased. Depending on the compiler, I see about a 1.5x to 3x increase in the I'm not certain what issues this would have if ImPlot were linked as a shared library. ImGui recommends not doing this, and so I don't think it's a real concern. Most users will add the source files directly to their own anyway. However, I'm still undecided if this is a good design or a bad design... |
This are the generations for https://github.com/cimgui/cimplot/tree/templates CIMGUI_API void ImPlot_PlotLinedoublePtrInt(const char* label_id,const double* values,int count,int offset,int stride)
{
return ImPlot::PlotLine(label_id,values,count,offset,stride);
}
CIMGUI_API void ImPlot_PlotLineFloatPtrInt(const char* label_id,const float* values,int count,int offset,int stride)
{
return ImPlot::PlotLine(label_id,values,count,offset,stride);
}
CIMGUI_API void ImPlot_PlotLineS32PtrInt(const char* label_id,const ImS32* values,int count,int offset,int stride)
{
return ImPlot::PlotLine(label_id,values,count,offset,stride);
}
CIMGUI_API void ImPlot_PlotLineU32PtrInt(const char* label_id,const ImU32* values,int count,int offset,int stride)
{
return ImPlot::PlotLine(label_id,values,count,offset,stride);
}
CIMGUI_API void ImPlot_PlotLinedoublePtrdoublePtr(const char* label_id,const double* xs,const double* ys,int count,int offset,int stride)
{
return ImPlot::PlotLine(label_id,xs,ys,count,offset,stride);
}
CIMGUI_API void ImPlot_PlotLineFloatPtrFloatPtr(const char* label_id,const float* xs,const float* ys,int count,int offset,int stride)
{
return ImPlot::PlotLine(label_id,xs,ys,count,offset,stride);
}
CIMGUI_API void ImPlot_PlotLineS32PtrS32Ptr(const char* label_id,const ImS32* xs,const ImS32* ys,int count,int offset,int stride)
{
return ImPlot::PlotLine(label_id,xs,ys,count,offset,stride);
}
CIMGUI_API void ImPlot_PlotLineU32PtrU32Ptr(const char* label_id,const ImU32* xs,const ImU32* ys,int count,int offset,int stride)
{
return ImPlot::PlotLine(label_id,xs,ys,count,offset,stride);
} |
Your suggestion here: I agree people using non-C/C++ backends are likely to not mind the size increase, but C/C++ crowd may, in particular people using dear imgui on the web with emscripten, it also tends to have an effect on compilation and link time, if it can be avoided it would be a nice bonus doing so. |
This needs cimplot generator to be manually customized adding --this list will expand all templated functions
parser.ftemplate_list = {"float", "double", "ImS8", "ImU8", "ImS16", "ImU16", "ImS32", "ImU32", "ImS64", "ImU64"} Getting this info from the header in any standarized way would be a completely automatic solution. But I am not being able to find such a standarized way!! (it seems that instantiations must live in cpp files) Also would avoid all templated functions expanded by the same ftemplate_list. But this could be solved by each templated type having its own list: --this list will expand all templated functions
parser.ftemplate_list = {}
parser.ftemplate_list.T = {"float", "double", "ImS8", "ImU8", "ImS16", "ImU16", "ImS32", "ImU32", "ImS64", "ImU64"}
parser.ftemplate_list.T2 = {"float", "double"} |
Yes, I should have mentioned that API functions can be called without the explicit template parameter, which is a nice bonus.
Correct, there's no way to put them in the header without exposing the rest of the template code.
I think this may be ok, since I plan to make each plotting function accept all 10 types. However, we could add a comment for each function with the listed supported types. @ocornut :
Actually, it's nasty template code all the way down. In my initial testing, it seemed that we gained an appreciable FPS boost from a template based design. Unfortunately that means the compiler is generating a bunch of functions/objects like this: RenderPrimitives<ShadedRenderer<GetterXsYs<ImS32>,GetterXsYRef<ImS32,double>,TransformerLinLin>>(...) Perhaps I went a little overboard, but as I said, it seemed to pay off (see #41 (comment), which compares basic ImGui Here are some numbers for compile size and time. Each compiler is set to Release mode with CMake. Beyond that I don't know how each is optimizing the build.
Regarding compile time: because the template instantiations are in the |
Not to derail the topic here, but @ocornut you may find this interesting. Here's a comparison a few different rendering methods using our new benchmark tool #102 : Run 1 - ImPlot's custom template based rendering pipeline (no AA is done) |
It I look at your first message in the thread it seems like the only template variations required are due to the fact that the getter return a strong type, if the getter wrote to an opaque pointer wouldn't it fix it?
Out of curiosity I would suggest also comparing unoptimized builds. If you optimize for Release you only optimize for the best-case scenario, whereas the real-world is also interested in the daily-scenario which may not be full-optimization on.
That's great! I hope your automation framework in imgui_dev could benefit from some of those plotting later. Do you know where are the gains from? if there is something we could report to ->AddLine? |
I've completed templetizing the library on the
Unless there are any major objections, I'm going to merge this later tonight after a few more checks with different compilers and also trying to export the functions in a shared library. Overall this is a big step in the right direction to making ImPlot usable for everyone! @ocornut, I'll respond to your questions in an issue on the ImGui repo. |
This work is completed and merged to master. I did a few final checks, verifying multiple compilers and that ImPlot can be compiled and used as DLL with the template API. I half expected there to be export issues, but surprisingly there were none (save the typical MSVC __declspec nonsense, which is solved with a good ole @sonoro1234 , feel free to open a separate issue where we can discuss what modifications are necessary for cimplot. @hoffstadt , let me know if you have any issues with Python. |
@epezent Thanks for all the great work man (If I see you in Houston, I'll buy you a beer). I plan on finishing the wrapping within a week or so! |
Greetings again Evan, this is all such excellent work - you are prolific! I did implement ImPlotPoint getters as you originally suggested, resulting in some beautiful audio-data graphs. Then I fell down a rabbit hole trying to profile/trace through assembly, seeing how much of the function call overhead the compiler might optimize away (turns out not much, at least for clang). Anyway from my "user perspective" I'm grateful for your continued valiant effort. Your design seems sound, your code tidy considering the increased flexibility, and your consideration of issues like compile times (developer's daily achilles heel) versus lib/executable sizes (and templates' "you only pay for what you use" (my exact words in a deleted comment) although lib-wise handled differently per build system) all extremely important! Yesterday I tried to convert my I'm struggling with this a bit, because I guess I should use the version of By the way here's a pic of the particular graph I'm attempting. My old Thanks again for everything, Evan! |
If I understand your problem, you want to use the single array version of PlotLine (i.e. We could potentially add I was also thinking you could generate a static array of equally spaced times (e.g at 48 kHz), say up to 10 minutes, and use it for every one of your audio plots (assuming they all have the same sample rate). However, I see now the issue is that with the current template design you can't have floating point Xs if your Ys are Out of curiosity, do you see a performance gain between the function pointer version |
I'll think on the possibility of exposing a stripped down header file with fully templated plotting functions. It should be possible, I just have to figure out the best way to decouple it from |
Thanks for your thoughts Evan and especially for troubling to consider my noobish requests! First of all,
Correct, but may I add "in appearance" to your last clause. X-axis can be an index, say in terms of granularity, but what would be really nice is if there was some kind of conversion I could apply to that index before it displays on axis or even hover. In a way, this seems like something that However, on my "wish list," if I may be so bold, would be for this conversion to somehow be flexible enough to display a format like
Line#1: Yes, exactly! Line#2: This would absolutely solve my problem, which is that I'm having to end my plot prematurely due to the discontinuity in my circular buffer. Before you suggested this, I was even thinking I could call
While I think your static array suggestion is intriguing (and doable), what you're saying here about non-homogeneous data, in my opinion, isn't necessary. It overcomplicates, when "simplicity is beautiful." I say your current approach is perfectly ideal, not "mixing worlds" between ints and floats when most devs want to handle those worlds on fully separate terms at their appropriate times. The compromise, I think, is in allowing axes units/formats, for instance, to be re-labelled... if someone wants that mix for display purposes. So I think your current templates are more than adequate! Great work Evan. |
Are you aware of the new time formatted axes? #48 (comment) It's not exactly what you'd want in this case (because it uses a UNIX timestamp and has a date associated with the time), but it might be useful.
It won't resume at the last index, but the two lines will be grouped together in the Legend.
I agree. We can add the scaling and offset to these functions. Regarding support for custom labels programatically, I don't have an immediate answer for you. A formatting callback comes to mind, but that would seem to violate the ImGui philosophy. What would be the ideal interface for this, in your opinion? |
This makes complete sense and seems, as I see now, consistent with ImGui.
Re: time formatted axes: great feature! Although in my case, all I really need is what you have already provided: Using I'm thinking that with your addition of Again, Evan, thanks! This is all a lot of fun. |
@electromaggot , check the latest version of template <typename T> IMPLOT_API void PlotLine(const char* label_id, const T* values, int count, double xscale=1, double x0=0, int offset=0, int stride=sizeof(T)); |
Ha ha INCREDIBLE! Now you check this out! (sorry for crappy capture quality and non-realtime speed) You rock, Evan! I owe you a zillion thanks... and a second beer should I make it to Houston. I will keep looking at your code and hopefully at some point I can contribute to this awesome project. Meantime, keep rockin' and thanks again! (edit: By the way, the top graph is real unadulterated Spotify SP_SAMPLETYPE_INT16_NATIVE_ENDIAN data. I hit "Play" in my app, that data starts streaming and fills a 4-second buffer as it plots to the right. The audio device consumes the data on the left, which plays out the speakers right about the time the plot hits the left edge. It's easy see how fast the stream buffers-ahead and outpaces the playback. This buffer is circular, so when it wraps, I do two |
Your work is amazing Evan! Thanks for doing all this :) |
@electromaggot, that's awesome man! I'm feeling inspired to experiment with audio plotting now. Happy to hear that the new functions are working for you. @ocornut, thank you! That means a lot. I'm just trying to maintain the bar you've already set with ImGui! (PS: still going to post an issue on ImGui regarding line optimizations. Just busy). |
I wish I could use ImPlot for audio data, which e.g. Spotify delivers as
Int16
(signed) interleaved samples, which feed to the audio device as same. My FFTs yield integer ranges as well. I'd rather avoid converting entire blocks of ints to floats for trivial display, simply to skirt a library inflexibility.ImPlot's core piece - the data it operates on and that data's type - being limited to floating point is a fundamental inconvenience. @ocornut's suggestion to templatize the datatype internally, then expose type-specific APIs, seemed to be summarily dismissed. #36 (comment)
@epezent you mentioned there that templates "make the header a mess" but the dual "float then double" repeated function prototypes don't seem elegant either.
Does anyone else have need for different dataset types? Surely someone has. I'm considering a fork to investigate a templated approach but would rather not repeat someone else's failed attempt at wheel reinvention. :-)
By the way, @epezent, nice work on this! It looks great.
The text was updated successfully, but these errors were encountered: