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

ImGui Static Lib Stuff #55

Closed
xiconxi opened this issue Oct 15, 2019 · 13 comments
Closed

ImGui Static Lib Stuff #55

xiconxi opened this issue Oct 15, 2019 · 13 comments
Milestone

Comments

@xiconxi
Copy link

xiconxi commented Oct 15, 2019

vcpkg's imgui is a static lib, while link to this version of lib may due to imgui.lib multiple link to the executable, both in imgui-example.exe and magnum-integration.dll.
So Is it possible to add imgui repos as a external libs in this repos? Or make an PR to vcpkg repos ?
for details, vcpkg/ports/imgui/CMakeLists.txt

add_library(${PROJECT_NAME}
    ${IMGUI_INCLUDES_PUBLIC}
    ${IMGUI_INCLUDES_PRIVATE}
    ${IMGUI_SOURCES}
)
@mosra mosra added this to the 2019.0b milestone Oct 15, 2019
@mosra
Copy link
Owner

mosra commented Oct 15, 2019

Hi!

The static lib and symbol duplication is a valid concern -- however I thought vcpkg's imgui is static only on static builds and dynamic on dynamic builds? Or is that not so?

@Squareys how do you solve this on your side? Build all stuff as static?

If imgui can't be built as dynamic (missing symbol exports) then I'm not quite sure how to solve this. Bundling it here would only move the problem elsewhere -- it'll always be one copy in the DLL and one in the exe.

@xiconxi
Copy link
Author

xiconxi commented Oct 16, 2019

How about refining this implement as a headonly integration ?

@mosra
Copy link
Owner

mosra commented Oct 16, 2019

That could work, yes ... but complicate the building quite a lot. Alternatively, the vcpkg's magnum-integration package could always build ImGuiIntegration as a static library -- that could work too, right?

Oh, and I'm working on a solution for a similar problem with Magnum itself -- when it gets built as static and linked into two DLLs (or an exe and a DLL), the two or three globals that Magnum needs get duplicated. Once I have this solved (it's a bit tricky on Windows), I could look into adding the fix into ImGui as well.

@xiconxi
Copy link
Author

xiconxi commented Oct 16, 2019

Could EigenIntegration be a headeronly lib as EigenIntegration? Then imgui will only link to the real UI implement part by assuming the app have only one ui implement lib(binary compatible, like Qt?) or directly implement in the executable part.

@mosra
Copy link
Owner

mosra commented Oct 16, 2019

Technically probably yes (imgui itself is not a header-only lib, ), but I'm not happy about the negative effect header-only libs have on compile times. And, especially on Windows, imgui.cpp, pulls in <windows.h> with all the ugly cruft -- you don't want that in your code :/

@mosra
Copy link
Owner

mosra commented Oct 16, 2019

Found this: https://github.com/ocornut/imgui/blob/9994f5bcbed2649907ecc52be0010f8f3c9a697e/imgui.cpp#L891-L907 and, after I implement the "globals shared across DLLs on Windows", I think the ImGuiIntegration could handle this internally, with no extra effort needed from your side.

Will keep you updated.

@mosra mosra mentioned this issue Oct 18, 2019
60 tasks
@mosra
Copy link
Owner

mosra commented Oct 19, 2019

After thinking about this a bit more, I'm quite sure this can be fixed from within the final application. (And on the other hand, there's little chance we could modify the Vcpkg package to have the internal global GImGui shared across DLLs.)

What you need is ensuring that both copies of the internal GImGui variable (in the integration DLL and in your exe) are set to the same pointer. The ImGuiIntegration::Context does this implicitly for the integration DLL itself (every time you call any of its APIs, the GImGui gets updated), then what's left is calling this in your code, right after you create the Context instance:

// in the Application class definition
ImGuiIntegration::Context _imgui;

// in the constructor, once Context is created
ImGui::SetCurrentContext(_imgui.context());

That should be enough to have ImGui working in this scenario. Let me know if it fixes the problem for you.

@mosra mosra modified the milestones: 2019.10, 2019.1c Oct 23, 2019
@xiconxi
Copy link
Author

xiconxi commented Oct 24, 2019

I think that will actually fix the crashing problem. But the flicking still exist maybe is caused by some inappropriate GL state

@xiconxi xiconxi closed this as completed Oct 24, 2019
@mosra
Copy link
Owner

mosra commented Oct 24, 2019

Is this with latest master? Does that happen with the unmodified example as well? I remember some quite serious issues in the 2019.01 release, caused by bugs in Intel drivers. Those were fixed on Magnum side, but who knows what exciting new bugs the driver team messed up since.

@mosra mosra reopened this Oct 24, 2019
@alanjfs
Copy link

alanjfs commented Oct 24, 2019

But the flicking still exist maybe is caused by some inappropriate GL state

What does your flickering look like, exactly? I'm getting what one might call flicker, with a Intel HD Graphics 620, whereas it works fine with a NVIDIA GeForce GTX 1050.

cargame25_imgui1

That's on-top of getting some linker warnings that I suspect might be related, if not to the flickering than at least to this issue.

LINK : warning LNK4217: symbol '?SetCurrentContext@ImGui@@YAXPEAUImGuiContext@@@Z (void __cdecl ImGui::S 
etCurrentContext(struct ImGuiContext *))' defined in 'imguid.lib(imgui.cpp.obj)' is imported by 'Engine.ob 
j' in function '"private: bool __cdecl Magnum::ImGuiIntegration::Context::handleKeyEvent<class Magnum::Pla 
tform::Sdl2Application::KeyEvent>(class Magnum::Platform::Sdl2Application::KeyEvent &,bool)" (??$handleKey 
Event@VKeyEvent@Sdl2Application@Platform@Magnum@@@Context@ImGuiIntegration@Magnum@@AEAA_NAEAVKeyEvent@Sdl2 
Application@Platform@2@_N@Z)' [C:\Users\alan\Dropbox\dev\toy\vs\cargame\Engine\Engine.vcxproj]           
  LINK : warning LNK4217: symbol '?GetIO@ImGui@@YAAEAUImGuiIO@@XZ (struct ImGuiIO & __cdecl ImGui::GetIO(v 
oid))' defined in 'imguid.lib(imgui.cpp.obj)' is imported by 'Engine.obj' in function '"private: bool __cd 
ecl Magnum::ImGuiIntegration::Context::handleKeyEvent<class Magnum::Platform::Sdl2Application::KeyEvent>(c 
lass Magnum::Platform::Sdl2Application::KeyEvent &,bool)" (??$handleKeyEvent@VKeyEvent@Sdl2Application@Pla 
tform@Magnum@@@Context@ImGuiIntegration@Magnum@@AEAA_NAEAVKeyEvent@Sdl2Application@Platform@2@_N@Z)' [C:\U 
sers\alan\Dropbox\dev\toy\vs\cargame\Engine\Engine.vcxproj]        

@mosra
Copy link
Owner

mosra commented Oct 25, 2019

Yeah this is exactly the one I attempted to fix post-2019.01. Is this 2019.01 or master?

This was a very nasty synchronization-related bug in Intel drivers and should go away completely when you run the app with --magnum-disable-extensions GL_ARB_direct_state_access. I couldn't manage to minimize it down to a reproducible example and so the fix might not be covering all cases. It's the intel-windows-buggy-dsa-bufferdata-for-index-buffers workaround listed at https://doc.magnum.graphics/magnum/opengl-workarounds.html

@Squareys
Copy link
Contributor

Sorry for begin late to the discussion.

If imgui can't be built as dynamic (missing symbol exports)

I can thanks to the IMGUI_USER_CONFIG which allows providing a header that defines IMGUI_API to import or export. In MagnumIntegrationImGui case, this matches the integration's export symbols.

When finding source and embedding all the symbol definitions inside MagnumIntegrationImGui, that works great, since that is basically then a dynamic build of imgui.
With the static library, no idea how that would work. I have been defining IMGUI_DIR to ensure that it uses the sources and embeds imgui inside ImGuiIntegration.

@mosra
Copy link
Owner

mosra commented Jun 26, 2020

Closing this in favor of #67, which is about basically the same problem, but with additional ideas & investigation. The flickering stuff is definitely fixed by now.

@mosra mosra closed this as completed Jun 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

4 participants