mosra / magnum-integration

Integration libraries for the Magnum C++11 graphics engine
https://magnum.graphics/
Other
99 stars 44 forks source link

Imgui_internal.h IMGUI_API inclusion #94

Closed jvannugteren closed 2 years ago

jvannugteren commented 2 years ago

When I try to compile implot with magnum imgui integration under windows. I'm getting unresolved external symbol warnings for "ImPool". This is a class located in imgui_internal.h. I can fix the error by replacing current line 638 struct IMGUI_API ImPool to struct __declspec( dllexport ) ImPool. This means that magnum's visibility.h file is likely not included for the imgui_internal header, even though there is IMGUI_API macros there. Unfortunately, I could not figure out how magnum's cmake system does this. Thanks for looking at this.

pezcode commented 2 years ago

The IMGUI_API macros are set as interface compile definitions on the ImGui::ImGui target. Linking ImGui::ImGui to implot (if that's a CMake target in your setup) should bring in the correct macros. The source files are added as interface sources, I'm not sure if that means you'll get duplicate symbols 🤔

jvannugteren commented 2 years ago

My CMakeLists.txt is something like this:

set(WITH_IMGUI ON CACHE BOOL "" FORCE)
set(IMGUI_DIR ${CMAKE_CURRENT_SOURCE_DIR}/imgui)
add_subdirectory(magnum-integration EXCLUDE_FROM_ALL)
find_package(MagnumIntegration REQUIRED ImGui)
add_library(implot SHARED
    implot/implot.cpp
    implot/implot_demo.cpp
    implot/implot_items.cpp
)
target_link_libraries(implot PUBLIC MagnumIntegration::ImGui)

I think I'm doing as you say. Note that the IMGUI_API works fine for all imgui.h functions/classes. Its really only that ImPool function in imgui_internal.h.

pezcode commented 2 years ago

You need to directly link to ImGui::ImGui for the interface compile definitions to apply to the implot target. Which may or may not break because that means imgui is being compiled twice, once in MagnumIntegration and once in implot.

Alternatively, try this:

set_property(TARGET implot APPEND PROPERTY COMPILE_DEFINITIONS
            "IMGUI_USER_CONFIG=\"Magnum/ImGuiIntegration/visibility.h\"")

possibly needs an include dir too, so it finds that.

jvannugteren commented 2 years ago

Added this.

find_package(ImGui REQUIRED Sources)
target_link_libraries(implot PUBLIC 
    MagnumIntegration::ImGui
    ImGui::ImGui
)

Had to also put find_package as the ImGui::ImGui was not directly available to me. but same error(s):

error LNK2019: unresolved external symbol "__declspec(dllimport) public: struct ImPlotItem * __cdecl ImPool<struct ImPlotItem>::GetOrAddByKey(unsigned int)" (__imp_?GetOrAddByKey@?$ImPool@UImPlotItem@@@@QEAAPEAUImPlotItem@@I@Z) referenced in function "public: struct ImPlotItem * __cdecl ImPlotItemGroup::GetOrAddItem(unsigned int)" (?GetOrAddItem@ImPlotItemGroup@@QEAAPEAUImPlotItem@@I@Z)
jvannugteren commented 2 years ago

I'll try your second suggestion now.

jvannugteren commented 2 years ago

No effect. I think because there is no IMGUI_USER_CONFIG in imgui_internal.h. It does conditionally include imgui.h but only if some version number doesn't exist.

jvannugteren commented 2 years ago

I think this might be more of an imgui/implot issue than magnum-integration maybe?

pezcode commented 2 years ago

No effect. I think because there is no IMGUI_USER_CONFIG in imgui_internal.h. It does conditionally include imgui.h but only if some version number doesn't exist.

That should still work 🤔 implot.cpp includes:

  1. implot.h -> includes imgui.h -> includes IMGUI_USER_CONFIG
  2. implot_internal.h -> includes imgui_internal.h -> uses already defined IMGUI_API

Not really sure what's going wrong here

jvannugteren commented 2 years ago

Nvm previous message. What you say seems logical.

jvannugteren commented 2 years ago

I'll try to make a minimalist example for this problem.

jvannugteren commented 2 years ago

As promised here is a minimalist example.

pezcode commented 2 years ago

Thanks for the repro ☺️ I believe this is an issue in imgui. My working theory:

Adding IMGUI_API to a bare template won't actually export it in a shared library (unless you add it to an explicit instantiation with a type). The code using the template however expects the shared library to export an explicit instantiation because it knows the type at that point. Hence the undefined symbol errors. Removing the IMGUI_API altogether makes this work for me.

Maybe a language expert can pitch in 🤠

jvannugteren commented 2 years ago

Interesting. So when compiling with msvc implot.cpp, which includes implot_internal.h, which in turn includes imgui_internal.h, the ImPool template class definition: template<typename T> struct ImPool works, template<typename T> struct __declspec( dllexport ) ImPool also works, but template<typename T> struct IMGUI_API ImPool where magnum sets IMGUI_API to __declspec( dllexport ) does not work. Shall I make an issue with ImGui and refer to this one?

pezcode commented 2 years ago

Well, magnum sets it to dllimport inside the implot target which makes the compiler try to find it in ImGuiIntegration. In ImGuiIntegration magnum set it to dllexport but since it's on a template (as opposed to an explicit instantiation - like template IMGUI_API struct ImPool<int>) it didn't actually get exported.

Roughly related question on stackoverflow: https://stackoverflow.com/questions/362822/how-do-i-export-templated-classes-from-a-dll-without-explicit-specification

So yes, I would file an issue with imgui.

jvannugteren commented 2 years ago

Ok. I see how template class and dllexport are incompatible with one another. Working on an imgui issue.

jvannugteren commented 2 years ago

After discussing with ocornut on imgui he removed the IMGUI_API from all template classes. This fixes the problem.