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

[geogram] Fix imgui not downloading #32487

Closed
wants to merge 2 commits into from
Closed

[geogram] Fix imgui not downloading #32487

wants to merge 2 commits into from

Conversation

jimwang118
Copy link
Contributor

Fixes #32421
Fixed imgui not downloading when compiling geogram.
Fix the issue that the header file cannot be found when compiling geogram_gfx.
Usage test pass with following triplets:

x86-windows
x64-windows
x64-windows-static
  • Changes comply with the maintainer guide
  • SHA512s are updated for each updated download
  • The "supports" clause reflects platforms that may be fixed by this new version
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

@jimwang118 jimwang118 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 Jul 11, 2023
@jimwang118 jimwang118 marked this pull request as ready for review July 11, 2023 07:08
@Adela0814 Adela0814 added the info:reviewed Pull Request changes follow basic guidelines label Jul 11, 2023
@dg0yt
Copy link
Contributor

dg0yt commented Jul 11, 2023

Is it unable to use the vcpkg port?
Or is it only used by tools, not libs, in this port?

#third_party: imgui
vcpkg_from_github(
OUT_SOURCE_PATH IMGUI_SOURCE_PATH
REPO ocornut/imgui
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imgui is a port so vendoring it like this isn't OK. Please use the vcpkg port instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also it looks like this is only necessary with the graphics feature turned on?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also it looks like this is only necessary with the graphics feature turned on?

Yes, imgui is only called when the graphics feature is compiled. The imgui version used for gram is version 1.89.4. The imgui version in vcpkg is 1.89.6. The imgui in vcpkg does not define ImGuiWindowFlags_NoDocking ImGuiCol_DockingPreview ImGuiCol_DockingEmptyBg, but it is compiled in geogram used in . So I am not sure whether to wait for the upstream to update to the latest imgui to modify, or to modify according to the currently used version 1.89.4.
I raised an issue 89 upstream to ask the author if he wants to update imgui.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for following up with upstream!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for following up with upstream!

The upstream will modify the differences between these different versions in the next version, so we need to wait for the upstream to adapt and update the version or use the current patch to fix the problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the cmakelists.txt under the src\lib\geogram_gfx\third_party directory, part of the code of imgui will be compiled, and imgui is a network link when the geogram code is downloaded, if it is not downloaded separately, it will report that there is no error in the compiled cpp file. So the download process of imgui code is added in the file.

Copy link

@BrunoLevy BrunoLevy Jul 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The version of imgui used in geogram is a specific branch docking branch, that has additional functionalities (and the constants ImGuiWindowFlags_NoDocking, ImGuiCol_DockingPreview, ImGuiCol_DockingEmptyBg). If you update the vcpkg wrapper to download imgui, make sure you get the docking branch (git clone --branch docking https://github.com/ocornut/imgui.git)

If you tell me that having a dependency to a specific branch is a problem in vcpkg, I can detect which version of imgui is used and deactivate some features at compile time (but it is not ideal: user experience is much better with the additional functionalities given by the docking branch)

-- Bruno (main author/maintainer of geogram)

P.S. If there is anything I can do to help packaging in the next versions of geogram (including reorganizing a bit) do not hesitate to file issues in geogram github. (I have seen what you have done for BLAS and LAPACK, awesome !

@BillyONeal BillyONeal marked this pull request as draft July 11, 2023 18:52
@BillyONeal BillyONeal removed the info:reviewed Pull Request changes follow basic guidelines label Jul 11, 2023
@BillyONeal BillyONeal added the depends:upstream-changes Waiting on a change to the upstream project label Jul 20, 2023
@jimwang118 jimwang118 closed this by deleting the head repository Sep 25, 2023
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 depends:upstream-changes Waiting on a change to the upstream project 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.

[geogram] Build failure
5 participants