Support long & long double, add macro INSTANTIATE_FOR_NUMERIC_TYPES (Fix #319)

pthom commented 2 years ago

This fixes the issue #319 which stated that long double are not supported.

As a matter of fact, long double as well as unsigned long and signed long are not supported when using dynamic linking: see demo repository for a minimal reproduction here : https://github.com/pthom/tmp_implot_dyn

Commit details

There are two commits:

the first commit introduces a macro INSTANTIATE_FOR_NUMERIC_TYPES that reduces some boilerplate code around template instantiations. It provides a "setting" (IMPLOT_INSTANTIATE_ALL_NUMERIC_TYPES) in order to enable / disable the addition of "long double" & "signed/unsigned long". All the CI tests succeed. The compilation time of implot_items.cpp went up from 3.2 seconds to 4 seconds
the second commit disables this setting (and it can be activated by the user via the compil time define IMPLOT_INSTANTIATE_ALL_NUMERIC_TYPES) All the CI tests succeed.

Notes about numeric types synonyms quasi-synonyms:

On some platforms, "unsigned long" might be the same size as "unsigned long long", but it is nonetheless a separate type: see https://godbolt.org/z/1KWv5re7q (example with GCC 64 bits)

On some other platforms, "long double" might be the same size as "double", but it is nonetheless a separate type: see https://godbolt.org/z/ae71P7rqG (example with MSVC 64 bits)

Context:

I discovered this issue while creating python bindings for imgui and implot. Since numpy uses "long double" as the default float type, and "long" as the default integer type, python code like this would fail under linux 64 bits:

xs = np.array((1, 2, 3, 4))
implot.PlotBars("Bars", xs)  # xs is of type "signed long"

For info, the bindings I'm developing are not ready for production (hopefuly they will soon). However my intent is to write an automatic binding generation tool that takes great care in reproducing the layout and the documentation found in the library header files. See implot.pyi and imgui.pyi. For the gory details, the generated code automatically transcribes between a numpy ndarray and the corresponding C++ buffer, such as here for the plot_line function

pthom commented 2 years ago

I created a minimal repository that show a link error when using ImPlot::PlotLine<unsigned long> in the case of a shared library, here: https://github.com/pthom/tmp_implot_dyn

epezent commented 2 years ago

Today I learned you can pass a macro function to a macro function. This is awsome! I have wanted to do exactly what INSTANTIATE_FOR_NUMERIC_TYPES but never figured out how.

I have a few suggestions:

change instantiate_PlotLine to INSTANTIATE_PLOT_LINE (personal preference)
collapse instantiate_PlotLine, instantiate_PlotLine2, etc. to one macro (feels cleaner to not have numbered macros)

#define INSTANTIATE_PLOT_LINE(T) \
template IMPLOT_API void PlotLine<T> (const char* label_id, const T* values, int count, double xscale, double x0, ImPlotLineFlags flags, int offset, int stride); \
template IMPLOT_API void PlotLine<T>(const char* label_id, const T* xs, const T* ys, int count, ImPlotLineFlags flags, int offset, int stride);

INSTANTIATE_FOR_NUMERIC_TYPES( INSTANTIATE_PLOT_LINE );

pthom commented 2 years ago

Thanks for your review! You are right, the preprocessor is almost a second awkward hidden language inside C++, after the templates ;-)

I pushed your requested changes, they do improve the code.

There is one more thing I'd like to discuss. As a matter of fact, it also fails with static builds (see https://github.com/pthom/tmp_implot_dyn and edit the CMakeList to set set(BUILD_SHARED_LIBS OFF).

Thus, the setting that I added might seem superfluous, since I think "signed long" and "unsigned long" are quite common and should be supported. What do you think?

epezent commented 2 years ago

I think it's ok to require the user to define IMPLOT_INSTANTIATE_ALL_NUMERIC_TYPES for these types (for both static and dynamic linkage). Even though they are common, no one has every requested support for them until now. I'd rather not add these as default types and further extend compile times.

I'd even argue we may want to move some already existing types behind IMPLOT_INSTANTIATE_ALL_NUMERIC_TYPES to further reduce compile times. In #328 I suggested only instantiating int, float, and double by default. But we can make this decision at a later time.

I would suggest adding the following to the end of .github/CMakeLists.txt:

target_compile_definitions(implot PRIVATE IMPLOT_INSTANTIATE_ALL_NUMERIC_TYPES=1)

pthom commented 2 years ago

OK, I added the definition in the CMakeList.txt.

In order to prepare for a merge, I also reworded the comments on one line in implot_items.cpp.

Also, I mentioned of this option in the FAQ part of the Readme (Q: What data types can I plot?)

ocornut commented 2 years ago

Can this setup be used to manually restrict instanciations to seleected types? Eg embedded apps could instanciate only 1-2 types.

epezent commented 2 years ago

Can this setup be used to manually restrict instanciations to seleected types? Eg embedded apps could instanciate only 1-2 types.

I'd be interested in supporting this. Probably we'd want somehing like IMPLOT_INSTANTIATE_FOR_FLOAT, IMPLOT_INSTANTIATE_FOR_DOUBLE, etc. which default to the current behavior unless explicitly defined by the user. @pthom , do you see any way to extend this PR to support something like that? I'm not seeing an immediate solution that wouldn't require multiple preprocessor passes (i.e. we can't just put #if IMPLOT_INSTANTIATE_FOR_FLOAT etc. in the body of INSTANTIATE_FOR_STANDARD_NUMERIC_TYPES)

ocornut commented 2 years ago

I recall there’s some sort of trick you can use to provide a list of items (here types) in a macro and then expand this list for other macros.

pthom commented 2 years ago

Edit: See new related PR : https://github.com/epezent/implot/pull/399

epezent / implot