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] Fix find dependencies #15063

Merged
merged 11 commits into from
Dec 26, 2020
Merged

Conversation

JackBoosY
Copy link
Contributor

@JackBoosY JackBoosY commented Dec 11, 2020

  • Add dependent find_dependency.

Fixes #14753.

@JackBoosY JackBoosY added category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. labels Dec 11, 2020
@JackBoosY
Copy link
Contributor Author

Special thanks to @thedmd for giving me technical help.

@JackBoosY JackBoosY changed the title [imgui] Rewrite build process realted to bindings [imgui] Rewrite build process related to bindings Dec 11, 2020
@JackBoosY JackBoosY marked this pull request as ready for review December 11, 2020 10:27
@JackBoosY
Copy link
Contributor Author

@cloudhan Can you please test this PR?

Thanks.

@JackBoosY JackBoosY requested a review from PhoebeHui December 14, 2020 02:20
@JackBoosY
Copy link
Contributor Author

The sample solution provided by @cloudhan has been successfully tested.

@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Dec 15, 2020
@BillyONeal BillyONeal removed info:reviewed Pull Request changes follow basic guidelines requires:discussion labels Dec 17, 2020
@ras0219-msft
Copy link
Contributor

ras0219-msft commented Dec 17, 2020

This PR makes very large, breaking changes to how users consume imgui through vcpkg; it moves the example backends out of the main library file and injects the sources into all consuming DLLs, static libs, and executables.

I don't think this large change is warranted by the actual issue (#14753).

The linked PR in imgui has been in development for the last two and a half years, so I don't think it's appropriate to assume it will be merged very soon and regardless should be done separately from solving issue at hand.

@JackBoosY
Copy link
Contributor Author

@ras0219-msft The changes to this PR come from part of ocornut/imgui#1713.

I think it is inappropriate for us to force imgui to be used as a library, which is also contrary to the official intention. This caused the previous problem that the variables in imgui were set twice(issue #9660).

The modification in this PR fits the official definition of imgui (as a header-only library), and many people believe that this change is effective and reasonable. You can check the user's response in the upstream PR I provided.

@xarthurx
Copy link
Contributor

@strega-nil May I ask when this will be reviewed and merged?

@BillyONeal
Copy link
Member

@xarthurx

This PR is controversial because:

  1. It does something that upstream appears to be explicitly rejecting right now
  2. It's a potentially breaking change for existing imgui customers

(See Robert's reply #15063 (comment) for more details)

so it doesn't look like there's support to merge this right now, so it isn't a timeframe thing. Jack and Robert presumably need to have further discussion to figure out what happens.

@ras0219
Copy link
Contributor

ras0219 commented Dec 23, 2020

From the imgui README:

The core of Dear ImGui is self-contained within a few platform-agnostic files which you can easily compile in your application/engine. They are all the files in the root folder of the repository (imgui.cpp, imgui.h, imgui_demo.cpp, imgui_draw.cpp etc.).

No specific build process is required. You can add the .cpp files to your existing project.

Imgui is not a header-only library; there are several files that must be compiled in order to use it. The official instructions for the core code are (almost perfectly) equivalent to linking it as a static library.

There are three differences between prebuilding it as a static library compared to injecting the code into the consumer project:

  1. Injecting the code causes it to inherit macros and compiler settings from the consumer
  2. Injecting the code makes it extremely restrictive to write additional libraries on top of imgui. It forces these other libraries to also inject their code into the consumer to maintain the same contract (inherit compiler settings).
  3. It forces the final consumer to build the entire transitive application from source instead of being able to use binary caching.

The real reason why imgui recommends adding the sources to your code and discourages a separate static library build is because:

[Building separately] is a little harmful as it prevents people from configuring ImGui for their application via imconfig.h.

Modifying the sources of imgui is expected to be a way to configure the library. However, this change (injecting the source into the consumer) does not fundamentally fix this issue without further efforts to ensure that a user-provided imconfig.h would be selected over the file in include/.

Today, the user can overlay a modified version of the imgui port that copies a private imconfig.h over the sources before building; something like:

file(COPY "${CMAKE_CURRENT_LIST_DIR}/imconfig.h" DESTINATION "${SOURCE_PATH}")

which will work perfectly with building a static library and still enables full binary caching of imgui and any libraries that depend on it (such as imgui-sfml).

The primary issue in #9660 would have been made no better by this change; it's net neutral (or worse!). The problem was that the imgui-sfml library was being built as a DLL and thus having a separate copy of all the imgui global variables as the user's final application. The only correct fix for this problem was to build imgui-sfml statically, which we've done. Furthermore, this change actually introduces an expectation violation because if the user did successfully leverage these changes to configure imgui by replacing imconfig.h in the user's project, the sources built as part of imgui-sfml would not have that configuration and possibly re-introduce a similar bug!

Finally, the linked PR (ocornut/imgui#1713) has been in limbo for several years; it is unclear if it will be adopted as an official build. Even if it was, the author explicitly stated that the only purpose they see in having this build is to maintain the example apps:

... to provide the example applications I will maintain premake/cmake ....

which means there would still be no official buildsystem for consuming it as a library (since the author intends the user to copy the source code into their project).

Even if I have committed several egregious errors above, we should separate fixing the issue in question (#14753, which really just needs some find_dependency() & friends) with fundamentally shifting the way we currently provide imgui.

@JackBoosY
Copy link
Contributor Author

@ras0219 Fine, I will separate the new compilation method of bindings to another PR.

@JackBoosY JackBoosY requested a review from BillyONeal December 24, 2020 08:20
@JackBoosY JackBoosY changed the title [imgui] Rewrite build process related to bindings [imgui] Fix find dependencies Dec 24, 2020
@JackBoosY
Copy link
Contributor Author

Test complete locally on x64-windows / x64-windows-static.

@BillyONeal BillyONeal merged commit 08b6c85 into microsoft:master Dec 26, 2020
@BillyONeal
Copy link
Member

Thanks for your edits!

@JackBoosY JackBoosY deleted the dev/jack/14753 branch December 28, 2020 02:12
ryukw7 pushed a commit to ryukw7/vcpkg that referenced this pull request Dec 29, 2020
@Agorath Agorath mentioned this pull request Jan 14, 2025
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[imgui] issue with opengl bindings
7 participants