Skip to content
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 long & long double, add macro INSTANTIATE_FOR_NUMERIC_TYPES (Fix #319) #397

Merged
merged 6 commits into from
Sep 11, 2022

Conversation

pthom
Copy link
Contributor

@pthom pthom commented Sep 7, 2022

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

…ix epezent#319)

- INSTANTIATE_FOR_NUMERIC_TYPES is a macro which instantiates templated plotting functions for numeric types.
This macro helps reduce some boilerplate code for template functions instantiations.

- Added optional support for more numeric types (long and long double)
The numeric type list does not include "long", "unsigned long" and "long double".
Most of the time, it is not an issue when linking statically.
However, when linking dynamically, issues related to undefined functions can arise:
although those types might have the same size, they are considered separate.

define IMPLOT_INSTANTIATE_ALL_NUMERIC_TYPES) in order to define versions for those types

In this case, the compilation time for this specific file will be 33% longer

- implot_internal.h / ImMean and ImStdDev: added cast  to double
(suppress MSVC warning about downcasting)

- Notes about numeric types "synonyms":
  Even if "long double" and "double" might occupy the same size,
they are not complete synonyms, and it is legal to define overloads for both double and long double.
  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)
@pthom pthom mentioned this pull request Sep 7, 2022
@pthom
Copy link
Contributor Author

pthom commented Sep 10, 2022

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
Copy link
Owner

epezent commented Sep 10, 2022

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:

  1. change instantiate_PlotLine to INSTANTIATE_PLOT_LINE (personal preference)
  2. 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
Copy link
Contributor Author

pthom commented Sep 10, 2022

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
Copy link
Owner

epezent commented Sep 10, 2022

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
Copy link
Contributor Author

pthom commented Sep 11, 2022

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?)

@epezent epezent merged commit 49db527 into epezent:master Sep 11, 2022
@ocornut
Copy link
Collaborator

ocornut commented Sep 11, 2022

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

@epezent
Copy link
Owner

epezent commented Sep 11, 2022

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
Copy link
Collaborator

ocornut commented Sep 11, 2022

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
Copy link
Contributor Author

pthom commented Sep 11, 2022

Edit: See new related PR : #399

@pthom pthom deleted the pthom/add_num_types branch September 13, 2022 12:06
pthom added a commit to pthom/imgui_bundle that referenced this pull request Sep 20, 2022
See merged implot PRs epezent/implot#402 and epezent/implot#397 which were provided during the development of these bindings
Ben1138 pushed a commit to Ben1138/implot that referenced this pull request Oct 2, 2024
 epezent#319) (epezent#397)

* implot_items: INSTANTIATE_FOR_NUMERIC_TYPES / add long & long double (Fix epezent#319)

- INSTANTIATE_FOR_NUMERIC_TYPES is a macro which instantiates templated plotting functions for numeric types.
This macro helps reduce some boilerplate code for template functions instantiations.

- Added optional support for more numeric types (long and long double)
The numeric type list does not include "long", "unsigned long" and "long double".
Most of the time, it is not an issue when linking statically.
However, when linking dynamically, issues related to undefined functions can arise:
although those types might have the same size, they are considered separate.

define IMPLOT_INSTANTIATE_ALL_NUMERIC_TYPES) in order to define versions for those types

In this case, the compilation time for this specific file will be 33% longer

- implot_internal.h / ImMean and ImStdDev: added cast  to double
(suppress MSVC warning about downcasting)

- Notes about numeric types "synonyms":
  Even if "long double" and "double" might occupy the same size,
they are not complete synonyms, and it is legal to define overloads for both double and long double.
  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)

* IMPLOT_INSTANTIATE_ALL_NUMERIC_TYPES: disabled by default

* uppercase template instantiatation macros & group them

* implot_items.cpp: reword comments on IMPLOT_INSTANTIATE_ALL_NUMERIC_TYPES

* README.md: mention compile-time option IMPLOT_INSTANTIATE_ALL_NUMERIC_TYPES

* Github CI: IMPLOT_INSTANTIATE_ALL_NUMERIC_TYPES=1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants