Imgui: Refactor in 1.64 (imgui_widgets.cpp)

[ocornut]

Here's an idea I have and I would happily get your feedback on it.

Aiming to release 1.63 shortly (maybe this week).

Immediately after I will release 1.64 which would _ONLY_ consist in a shuffling code around.

Some of the code in imgui.cpp would be split into imgui_widgets.cpp. As you can guess imgui_widgets.cpp will contains all the widgets specific code, whereas imgui.cpp would contains the core.
In my preliminary tests, about 30-35% of imgui.cpp would move to imgui_widgets.cpp.
(I expect that % to grow as down the line I'd like to remove stb_textedit.h and re-implement a custom version, as much of our InputText() issues are tricky to solve with the current code)

Behavior shouldn't be affected but moving code around will break local forks and make them conflicts when merged. Making a clear documented transition between 1.63->1.64 will facilitate the update for people with non-trivial fork (they will be advised to first update to 1.63, isolate their diffs, then update to 1.64 and reapply).

This would be a change analogous to 1.44 when I extracted imgui_draw.cpp out of imgui.cpp.
This would obviously break the build for project which have an hardcoded list of file instead of imgui*.cpp but the fix should be trivial.

I will also perform various performance tests (e.g. how do non-static functions in a same compilation unit affect optimizations, if any? does anyone has info to share about that for msvc/clang/gcc?) and depending on the result will aim at including the most touched low-level function calls called from widgets into imgui_widgets.cpp if it makes a difference.

While doing that shuffle I will take the occasion to probably re-order many functions in both imgui.cpp and imgui_widgets.cpp, and section some areas with comments. (exact sectioning/ordering criteria unknown, but I'll aim to make it easier for people to locate code. e.g. maybe one of the criteria for imgui_widgets.cpp might be that the function ordering matches the ordering in imgui.h?)

[ocornut]

This task is done and has been released and tagged as 1.64.
Please read https://github.com/ocornut/imgui/releases/tag/v1.64 for details.

[TheLavaBlock]

Good refactoring, but I had to add (in imgui_widgets.cpp):

#include <stdint.h>

because of:

../imgui_widgets.cpp:1235:24: error: unknown type name 'intptr_t'; did you mean '__intptr_t'?
        PushID((void*)(intptr_t)i);

[ocornut]

@TheLavaBlock Thank you, this has been fixed now (I have retagged the release). Please try to always specify your OS and Compiler/Version when submitting compilation errors, it helps deciding for the appropriate fix.

[TheLavaBlock]

Yeah, you are right, will do it next time. Thanks.

[bkaradzic]

I have issue with some functions that are moved (for example RoundScalarWithFormat, and SliderBehaviorCalcRatioFromValue) into imgui_widgets but are used in some user widgets, and included from imgui.cpp:

// Include imgui_user.inl at the end of imgui.cpp to access private data/functions that aren't exposed.
// Prefer just including imgui_internal.h from your code rather than using this define. If a declaration is missing from imgui_internal.h add it or request it on the github.
#ifdef IMGUI_INCLUDE_IMGUI_USER_INL
#include "imgui_user.inl"
#endif

Here is user code that uses those internal functions:
https://github.com/bkaradzic/bgfx/blob/master/3rdparty/dear-imgui/widgets/range_slider.inl

[ocornut]

I have issue with some functions that are moved (for example RoundScalarWithFormat, and SliderBehaviorCalcRatioFromValue)

This is tricky because they are template... I don't really fancy adding large functions implementation in imgui_internal.h.. Let me think about it.,

[ocornut]

My template-foo is lacking but it is looks like I could possible forward declare them in imgui_internal.h:

template<typename TYPE, typename SIGNEDTYPE>
IMGUI_API TYPE          RoundScalarWithFormat(const char* format, ImGuiDataType data_type, TYPE v);

as long as you can calling them with the same template arguments that are instantiated in imgui_widgets.cpp (which you are) it is working here.

Works and links with VS2010, Mingw 8.1 and Mingw 5.1. However Clang is warning:

D:\T-Work\GitHub\imgui\examples\example_null>clang++ -m64 -DIMGUI_DISABLE_WIN32_DEFAULT_CLIPBOARD_FUNCTIONS -DIMGUI_DISABLE_WIN32_DEFAULT_IME_FUNCTIONS -Xclang -flto-visibility-public-std -Weverything -Wno-zero-as-null-pointer-constant -Wno-old-style-cast -Wno-double-promotion -Wno-reserved-id-macro -Wno-float-equal -Wno-c++98-compat-pedantic -Wno-variadic-macros -g *.cpp ../../*.cpp -I../..
main.cpp:18:22: warning: instantiation of function 'ImGui::RoundScalarWithFormat<float, float>' required here, but no
      definition is available [-Wundefined-func-template]
    float f = ImGui::RoundScalarWithFormat<float,float>("%.1f", ImGuiDataType_Float, 1.123f);
                     ^
../..\imgui_internal.h:1249:29: note: forward declaration of template entity is here
    IMGUI_API TYPE          RoundScalarWithFormat(const char* format, ImGuiDataType data_type, TYPE v);
                            ^
main.cpp:18:22: note: add an explicit instantiation declaration to suppress this warning if
      'ImGui::RoundScalarWithFormat<float, float>' is explicitly instantiated in another translation unit
    float f = ImGui::RoundScalarWithFormat<float,float>("%.1f", ImGuiDataType_Float, 1.123f);
                     ^
1 warning generated.

I'm not sure I understand the solution, add an explicit instantiation declaration to suppress this warning if XXX is explicitly instantiated in another translation unit.

If I add in imgui_internal.h

template<>
IMGUI_API float         RoundScalarWithFormat<float, float>(const char* format, ImGuiDataType data_type, float v);

Then the .cpp file compile without warning but it fails linking on all compiler.
Do you know if there is a way to provide this forward declaration guaranteeing the existence of an instantiation while not having to provide a second function implementation?

EDIT
References

• https://github.com/dealii/dealii/issues/3705
• https://www.ibm.com/support/knowledgecenter/en/SSGH3R_13.1.3/com.ibm.xlcpp1313.aix.doc/language_ref/explicit_instantiation.html

PS: Obviously I'm trying to solve it without having to move template function implementations in imgui_internal.h

[ocornut]

@bkaradzic At the moment I don't understand how to solve this problem other than

• Silencing the warning on the caller code
• Providing a second function body specifically for (that would be identical..)
• Creating a non-template function that we'd expose (similarly to how SliderScalar takes void*+ `ImGuiDataType) that's essentially a switch.

I don't mind exposing a solution using some kind of extern/fwd declare mechanism even if it requires C++11 etc. (in which case we can selectively enable those forward declaration only on modern compilers, if that solves your problem).

[thedmd]

@ocornut Did you try to put an extern before template specialisation in header to turn it into declaration?

[ocornut]

If I have this in the .h

template<typename TYPE, typename SIGNEDTYPE>
IMGUI_API TYPE          RoundScalarWithFormat(const char* format, ImGuiDataType data_type, TYPE v);

extern template 
IMGUI_API float         RoundScalarWithFormat<float, float>(const char* format, ImGuiDataType data_type, float v);

Then it doesn't link because it is looking for a definition for that specialization.
However I want to use the regular which _IS_ instantiated via SliderFloat/DragFloat already, not provide a second copy of the function.

Weirdly _without_ that extern declaration it warns but does work/link, so there must be a solution?

[bkaradzic]

This might help: https://stackoverflow.com/a/8131212

[thedmd]

I mean:

template <>
extern IMGUI_API float         RoundScalarWithFormat<float, float>(const char* format, ImGuiDataType data_type, float v);

extern should be after template. Otherwise it will try to reefer to extern templates which were removed from C++.

Edit: this is probably bad advice

[ocornut]

This works with Clang/Mingw:

.h

extern template 
IMGUI_API float         RoundScalarWithFormat<float, float>(const char* format, ImGuiDataType data_type, float v);

.cpp

template
float ImGui::RoundScalarWithFormat<float,float>(const char* format, ImGuiDataType data_type, float v);

in the .cpp file with C+11.

VS2010 fails with:

1>imgui_internal.h(1253): warning C4231: nonstandard extension used : 'extern' before template explicit instantiation
1>imgui_widgets.cpp(1510): error C2929: 'float ImGui::RoundScalarWithFormat<float,float>(const char *,ImGuiDataType,float)' : explicit instantiation; cannot explicitly force and suppress instantiation of template-class member

In spite of https://msdn.microsoft.com/en-us/library/hh567368.aspx saying that Extern Template are supported in VS2010.

VS2012 fails with:

imgui_widgets.cpp(1511): error C2929: 'float ImGui::RoundScalarWithFormat<float,float>(const char *,ImGuiDataType,float)' : explicit instantiation; cannot explicitly force and suppress instantiation of template-class member

Both VS2010/VS2012 errors because imgui_widgets.cpp both sees the extern and the instantiation.. If you add an ifdef block to avoid the extern declaration when compiled from imgui_widgets.cpp it works, but we'd mess up with unity builds?

VS2015 compiles/links OK.

[thedmd]

Found a solution:

imgui_internal.h

template<typename TYPE, typename SIGNEDTYPE>
IMGUI_API inline TYPE RoundScalarWithFormat(const char* format, ImGuiDataType data_type, TYPE v);

template
IMGUI_API float RoundScalarWithFormat<float, float>(const char* format, ImGuiDataType data_type, float v);

imgui_widgets.cpp

template<typename TYPE, typename SIGNEDTYPE>
IMGUI_API inline TYPE RoundScalarWithFormat(const char* format, ImGuiDataType data_type, TYPE v)
{
    const char* fmt_start = ImParseFormatFindStart(format);
    if (fmt_start[0] != '%' || fmt_start[1] == '%') // Don't apply if the value is not visible in the format string
        return v;
    char v_str[64];
    ImFormatString(v_str, IM_ARRAYSIZE(v_str), fmt_start, v);
    const char* p = v_str;
    while (*p == ' ')
        p++;
    if (data_type == ImGuiDataType_Float || data_type == ImGuiDataType_Double)
        v = (TYPE)ImAtof(p);
    else
        ImAtoi(p, (SIGNEDTYPE*)&v);
    return v;
}

// instantiations
template
IMGUI_API float RoundScalarWithFormat<float, float>(const char* format, ImGuiDataType data_type, float v);

This should work. When template <> means specialization, template means instantiation. Today I le-learned.

[ocornut]

With your solution (only adding ImGui:: to the implementations in the .cpp file)

VS2010 is OK

Clang

In file included from main.cpp:5:
../..\imgui_internal.h:1253:29: error: explicit instantiation of undefined function template 'RoundScalarWithFormat'
    IMGUI_API float         RoundScalarWithFormat<float, float>(const char* format, ImGuiDataType data_type, float v);
                            ^
../..\imgui_internal.h:1250:29: note: explicit instantiation refers here
    IMGUI_API TYPE          RoundScalarWithFormat(const char* format, ImGuiDataType data_type, TYPE v);

(from every calls)

Similar stuff from Mingw

In file included from main.cpp:5:
../../imgui_internal.h: In instantiation of 'TYPE ImGui::RoundScalarWithFormat(const char*, ImGuiDataType, TYPE) [with TYPE = float; SIGNEDTYPE = float; ImGuiDataType = int]':
../../imgui_internal.h:1253:117:   required from here
../../imgui_internal.h:1253:117: error: explicit instantiation of 'TYPE ImGui::RoundScalarWithFormat(const char*, ImGuiDataType, TYPE) [with TYPE = float; SIGNEDTYPE = float; ImGuiDataType = int]' but no definition available [-fpermissive]
     IMGUI_API float         RoundScalarWithFormat<float, float>(const char* format, ImGuiDataType data_type, float v);
                                                                                                                     ^
../../imgui_internal.h: In instantiation of 'TYPE ImGui::RoundScalarWithFormat(const char*, ImGuiDataType, TYPE) [with TYPE = float; SIGNEDTYPE = float; ImGuiDataType = int]':
../../imgui_internal.h:1253:117:   required from here
../../imgui_internal.h:1253:117: error: explicit instantiation of 'TYPE ImGui::RoundScalarWithFormat(const char*, ImGuiDataType, TYPE) [with TYPE = float; SIGNEDTYPE = float; ImGuiDataType = int]' but no definition available [-fpermissive]
../../imgui_internal.h:1250:29: warning: inline function 'TYPE ImGui::RoundScalarWithFormat(const char*, ImGuiDataType, TYPE) [with TYPE = float; SIGNEDTYPE = float; ImGuiDataType = int]' used but never defined
     IMGUI_API inline TYPE   RoundScalarWithFormat(const char* format, ImGuiDataType data_type, TYPE v);
                             ^~~~~~~~~~~~~~~~~~~~~
I

[bkaradzic]

Oh C++ templates... Keep jiggling it, it's going to work eventually... :)

[thedmd]

Could you try to remove instantiation from header?
imgui_internal.h

template<typename TYPE, typename SIGNEDTYPE>
IMGUI_API inline TYPE RoundScalarWithFormat(const char* format, ImGuiDataType data_type, TYPE v);

VS swallowed that.

[jdumas]

Ok so you actually don't need any extern or inline keyword, and should be able to separate the implementation if you use explicit template instantiation (which is different from explicit template specialization, but the syntax can be confusing).

Minimal example

foo.h

#pragma once

// Function declaration
template<typename T> void show(T x);

foo.cpp

#include "foo.h"
#include <iostream>

// Function definition
template<typename T> void show(T x) {
    std::cout << x << std::endl;
}

// Explicit template instantiation
template void show<int>(int);

main.cpp

#include "foo.h"

int main(void) {
    show((int) 1);
    return 0;
}

Compilation

clang++ -c foo.cpp
clang++ -c main.cpp
clang++ main.o foo.o 

--> No warning whatsoever.

In any case, if you have a generic implementation of a templated function that you want to make available to everyone, but want to keep separation between header files and implementation, I recommend to 1) put the (forward) declaration of the templated function in imgui_internal.h, 2) put the definition (implementation) of the templated function in a separate file imgui_internal.tpp, and 3) #include "imgui_internal.tpp" at the end of imgui_internal.h.

PS: It's actually template-fu (as in kung-fu), not template-foo, but as a programmer one can argue about the legitimity of this second form :D

[ocornut]

@thedmd

Could you try to remove instantiation from header?

That's equivalent to my initial problem. Works with VS 10/12/15, Clang warns.

---

@jdumas

Ok so you actually don't need any extern or inline keyword, and should be able to separate the implementation if you use explicit template instantiation (which is different from explicit template specialization, but the syntax can be confusing).
No warning whatsoever

Minus the Explicit template instantiation (which is not strictly required in this case since there is an instantiation with the same type, but I'll leave it, better be explicit) - this is what my first/current version is doing, but Clang warns with -Weverything. The warning happens on the calling site, so we can't even easily disable them.

---

Perhaps I'm missing something but at this point I don't have an ideal solution.
If some of you want to try it, it would be good to test with at least VS2010, VS2015 and Clang with high-warning settings.

The worry if that we have a weak point here that would trigger warning/error on esoteric compilers.

To clarify a few points:

(Clang)
.h

template<typename TYPE, typename SIGNEDTYPE>
IMGUI_API TYPE          RoundScalarWithFormat(const char* format, ImGuiDataType data_type, TYPE v);

main.cpp:28:22: warning: instantiation of function 'ImGui::RoundScalarWithFormat<float, float>' required here, but no
      definition is available [-Wundefined-func-template]
    float f = ImGui::RoundScalarWithFormat<float,float>("%.1f", ImGuiDataType_Float, 1.123f);



md5-ecd7945f1c055372ebf44fde4ccb5b40



template<typename TYPE, typename SIGNEDTYPE>
IMGUI_API TYPE   RoundScalarWithFormat(const char* format, ImGuiDataType data_type, TYPE v);

template
float   RoundScalarWithFormat<float, float>(const char* format, ImGuiDataType data_type, float v);



md5-4f1dd350ccfb02506d6eed88d7ce2cc7



imgui_internal.h:1254:13: error: explicit instantiation of undefined function template 'RoundScalarWithFormat'
    float   RoundScalarWithFormat<float, float>(const char* format, ImGuiDataType data_type, float v);



md5-06268f9e2aa91963142c801ac8b08396



template<typename TYPE, typename SIGNEDTYPE>
IMGUI_API TYPE  RoundScalarWithFormat(const char* format, ImGuiDataType data_type, TYPE v);

template<>
float RoundScalarWithFormat<float, float>(const char* format, ImGuiDataType data_type, float v);



md5-4f1dd350ccfb02506d6eed88d7ce2cc7



imgui_widgets-b6fa7c.o : error LNK2001: unresolved external symbol "float __cdecl ImGui::RoundScalarWithFormat<float,float>(char const *,int,float)" (??$RoundScalarWithFormat@MM@ImGui@@YAMPBDHM@Z)
a.exe : fatal error LNK1120: 1 unresolved externals



md5-6c8e0a5494e0bf64c744f3dc86e62652



extern template
IMGUI_API float         RoundScalarWithFormat<float, float>(const char* format, ImGuiDataType data_type, float v);



md5-5758b7b59bb7ffb3ea3bc18108a205a8



template
float ImGui::RoundScalarWithFormat<float, float>(const char* format, ImGuiDataType data_type, float v);



md5-4f1dd350ccfb02506d6eed88d7ce2cc7



1>..\..\imgui_widgets.cpp(1510): error C2929: 'float ImGui::RoundScalarWithFormat<float,float>(const char *,ImGuiDataType,float)' : explicit instantiation; cannot explicitly force and suppress instantiation of template-class member

Which can be worked out with some define/ifdef but that could break the possibility of using unity builds?

---

TL;DR;

For now I can offer to _declare_ the templates in imgui_internal.h.
The user will need to use an extern template in their .cpp file in order to silence Clang warning, or silence clang warnings.

[ocornut]

@bkaradzic As stated above I have pushed changes to _declare_ the template function in imgui_internal.h
You'll need an extern template statement in your range_slider code to use them.

I also renamed them:

Renamed RoundScalarWithFormat<> -> RoundScalarWithFormatT<>
Renamed SliderBehaviorCalcRatioFromValue<> -> SliderCalcRatioFromValueT<>

Let me know if that solves your issue!

[jdumas]

Hmm if the warning comes from clang's -Weverything then I vote for ignoring it. -Weverything enables really all warnings, even those that are considered experimental, undesirable, or lead to false positive. It does not necessarily makes sense to heed those warning all the time, and there's a good reason not all warnings make it to -Wall and -Wextra. Now, in this case, I see this message as a way to warn against potential linker errors for someone that doesn't know what they're doing and forget to instantiate or define the necessary template function in other parts of the code. But this really defeats the purpose imho and sounds like a false positive you can safely ignore.

Honestly template function are just like macros that the compiler will expand and implement depending on what type is used. If the function body is available, the compiler will compile the function in every translation unit where it is used. But if only the declaration is available, then it will issue a standard function call and stuff will be resolved at linking time. It's just like any other function, which do not need an available definition to be called properly.

My recommendation: don't mess with extern qualifiers, it's probably not needed here, especially if it's only to make clang's -Weverything happy. I'll also throw a modified version of my example above that makes clang's -Weverything happy, without any extern keyword:

foo.h

#pragma once

// Declaration of the generic template function
template<typename T> void show(T x);

// Declaration of the specialized function
template<> void show(int x);

foo.cpp

#include "foo.h"
#include <iostream>

namespace {

template<typename T> void show_gen(T x) {
    std::cout << x << std::endl;
}

}

// Definition of the specialized function
template<> void show<int>(int x) {
    show_gen(x);
}

main.cpp

#include "foo.h"
#include <iostream>

int main(void) {
    show(int(1));
    return 0;
}

As you can see, it's mostly doing some gymnastic to avoid a warning that shouldn't be there in the first place.

[ocornut]

Then I vote for ignoring it.

I agree but as stated I cannot ignore it as the warning happens on the call site, so it's up to @bkaradzic there.

[bkaradzic]

@ocornut it builds with GCC, still waiting other compilers to finish:

https://github.com/bkaradzic/bgfx/blob/2904b42dafdcae37dfbda260dff543fb53e9faa0/3rdparty/dear-imgui/widgets/range_slider.inl#L7

[bkaradzic]

GCC 5 linker only complains about SliderCalcRatioFromValueT but not RoundScalarWithFormatT even thought both have internal instantiation.

../../linux64_gcc/bin/libexample-commonRelease.a(imgui.o): In function `ImGui::RangeSliderBehavior(ImRect const&, unsigned int, float*, float*, float, float, float, int, int)':
/home/travis/build/bkaradzic/bgfx/.build/projects/gmake-linux/../../../3rdparty/dear-imgui/widgets/range_slider.inl:117: undefined reference to `float ImGui::SliderCalcRatioFromValueT<float, float>(int, float, float, float, float, float)'
/home/travis/build/bkaradzic/bgfx/.build/projects/gmake-linux/../../../3rdparty/dear-imgui/widgets/range_slider.inl:131: undefined reference to `float ImGui::SliderCalcRatioFromValueT<float, float>(int, float, float, float, float, float)'
/home/travis/build/bkaradzic/bgfx/.build/projects/gmake-linux/../../../3rdparty/dear-imgui/widgets/range_slider.inl:131: undefined reference to `float ImGui::SliderCalcRatioFromValueT<float, float>(int, float, float, float, float, float)'

[ocornut]

@bkaradzic I'm not sure I understand why, sorry. Copying your extern template" and making a dummy call toSliderCalcRatioFromValueT()works for me with Mingw 8.1 and 5.1 (they both link properly). Because of howSliderBehavior()` is setup with a switch statement on all data type the instantiation should always happen.

[bkaradzic]

Ok it's wasn't GCC 5 specific (it happens with GCC 7 too), just in release build.

I had to do this for now... Until I figure out why it's getting stripped:
https://github.com/bkaradzic/bgfx/blob/350e5d45c3639c98566b033a8c8ca967a8d4ff84/3rdparty/dear-imgui/widgets/range_slider.inl#L13

[ocornut]

Yeah that looks strange to me (looking at https://github.com/bkaradzic/bgfx/commit/350e5d45c3639c98566b033a8c8ca967a8d4ff84#diff-3307f41d1c9cdf8632e3d8fdb3fa320f)

Closing this now, looks like it's solvable in user land with those declaration.
It's worth thinking about those issues for then I'll put more serious work into improving imgui_internal.h to allow building custom widgets.

[bkaradzic]

Also I wouldn't mind if you implement that range slider as part of ImGui. :)

[ocornut]

Will do, #76 is still open :)