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.

Sign up for GitHub

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

Jump to bottom

[imgui-sfml] Add new package ImGui-SFML #7429

Merged
merged 5 commits into from
Sep 3, 2019
Merged

[imgui-sfml] Add new package ImGui-SFML #7429

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fd1365b
[imgui-sfml] Add new package ImGui-SFML
tntxtnt Jul 25, 2019
03daa62
[imgui-sfml] Fix case-sensitive imgui package name
tntxtnt Jul 25, 2019
f3ddb65
[imgui-sfml] Require C++11
tntxtnt Jul 25, 2019
0c257bb
[imgui-sfml] Include IMGUI_API define
tntxtnt Jul 26, 2019
8e86519
[imgui-sfml] Remove delegating ctor - C++11 is not required
tntxtnt Aug 8, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ports/imgui-sfml/CONTROL
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Source: imgui-sfml
Version: 2.0.2
Homepage: https://github.com/eliasdaler/imgui-sfml
Description: Library which allows you to use ImGui with SFML
Build-Depends: sfml, imgui
30 changes: 30 additions & 0 deletions ports/imgui-sfml/portfile.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
include(vcpkg_common_functions)

# Compile as static lib since vcpkg's imgui is compiled as static lib
vcpkg_check_linkage(ONLY_STATIC_LIBRARY)

vcpkg_from_github(
OUT_SOURCE_PATH SOURCE_PATH
REPO eliasdaler/imgui-sfml
REF v2.0.2
SHA512 44099e162c0e712ec9147452189649801a6463396830e117c7a0a4483d0526e94554498bfa41e9cd418d26286b5d1a28dd1c2d305c30d1eb266922767e53ab48
HEAD_REF master
PATCHES
static-build-with-vcpkg-imgui.patch
remove-delegating-ctor.patch
)

vcpkg_configure_cmake(
SOURCE_PATH ${SOURCE_PATH}
PREFER_NINJA
)

vcpkg_install_cmake()
vcpkg_fixup_cmake_targets(CONFIG_PATH lib/cmake/ImGui-SFML)
vcpkg_copy_pdbs()

# Debug include directory not needed
file(REMOVE_RECURSE ${CURRENT_PACKAGES_DIR}/debug/include)

# License
file(INSTALL ${SOURCE_PATH}/LICENSE DESTINATION ${CURRENT_PACKAGES_DIR}/share/imgui-sfml RENAME copyright)
13 changes: 13 additions & 0 deletions ports/imgui-sfml/remove-delegating-ctor.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
diff --git a/imconfig-SFML.h b/imconfig-SFML.h
index 0f43ce6..1fce2c1 100644
--- a/imconfig-SFML.h
+++ b/imconfig-SFML.h
@@ -19,7 +19,7 @@

#define IM_VEC4_CLASS_EXTRA \
ImVec4(const sf::Color & c) \
- : ImVec4(c.r / 255.f, c.g / 255.f, c.b / 255.f, c.a / 255.f) { \
+ : x(c.r / 255.f), y(c.g / 255.f), z(c.b / 255.f), w(c.a / 255.f) { \
} \
operator sf::Color() const { \
return sf::Color( \
71 changes: 71 additions & 0 deletions ports/imgui-sfml/static-build-with-vcpkg-imgui.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 015a030..33d6894 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -29,7 +29,7 @@ set(IMGUI_SFML_CONFIG_NAME "imconfig-SFML.h" CACHE STRING "Name of a custom user
set(IMGUI_SFML_CONFIG_INSTALL_DIR "" CACHE PATH "Path where user's config header will be installed")

# For FindImGui.cmake
-list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_LIST_DIR}/cmake")
+# list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_LIST_DIR}/cmake")
Copy link

Choose a reason for hiding this comment

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

Not sure if this is necessary.

Copy link
Contributor Author

@tntxtnt tntxtnt Jul 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I just want to use 1 line of vcpkg's find_package(imgui REQUIRED) and target_link_libraries(... imgui::imgui) to include imgui library, so I disable all of your FindImgui cmake as they are not needed (I guess)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I should put a check for osx triplet and enable C++11 for osx build only.

Copy link

Choose a reason for hiding this comment

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

Yes, FindImGui.cmake is not needed, if you use vcpkg's find module, but it won't really change anything, so I think to make patch smaller, this line should not be changed with it.

Also I might put some other useful CMake things in imgui-sfml's "cmake" directory, so this might break some other things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

imgui-sfml needs these sources

set(IMGUI_SOURCES
  ${IMGUI_INCLUDE_DIR}/imgui.cpp
  ${IMGUI_INCLUDE_DIR}/imgui_draw.cpp
  ${IMGUI_INCLUDE_DIR}/imgui_widgets.cpp
  ${IMGUI_INCLUDE_DIR}/misc/cpp/imgui_stdlib.cpp
)

set(IMGUI_DEMO_SOURCES
  ${IMGUI_INCLUDE_DIR}/imgui_demo.cpp
)

while vcpkg's imgui CMakeLists.txt has these compiled

set(IMGUI_SOURCES
    imgui.cpp
    imgui_demo.cpp
    imgui_draw.cpp
    imgui_widgets.cpp
)

The only file that is missing is misc/cpp/imgui_stdlib.cpp, which it states that it needs C++11: https://github.com/ocornut/imgui/blob/d057550209d794461744a91c812bf8e9e264f32f/misc/cpp/imgui_stdlib.h#L5

Copy link
Contributor Author

@tntxtnt tntxtnt Jul 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

And since vcpkg has those sources already compiled, you won't find these .cpp files, which FindImGui.cmake will create a cmake config error so I don't use it.

Copy link

Choose a reason for hiding this comment

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

imgui_stdlib functions don't need C++11 to compile, but they're not guaranteed to work. That's an imporant difference.

As for the sources vs linking to target - yeah. Again, ImGui-SFML needs to add some flags which will allow you to either build ImGui along with ImGui-SFML or link to it in some other way.


if (IMGUI_SFML_FIND_SFML)
find_package(SFML 2.5 COMPONENTS graphics system window)
@@ -42,24 +42,24 @@ endif()
# ImGui does not provide native support for CMakeLists, workaround for now to have
# users specify IMGUI_DIR. Waiting for this PR to get merged...
# https://github.com/ocornut/imgui/pull/1713
-if(NOT IMGUI_DIR)
- set(IMGUI_DIR "" CACHE PATH "imgui top-level directory")
- message(FATAL_ERROR "ImGui directory not found. Set IMGUI_ROOT to imgui's top-level path (containing 'imgui.h' and other files).\n")
-endif()
+# if(NOT IMGUI_DIR)
Copy link

Choose a reason for hiding this comment

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

Does vcpkg set imgui_DIR? Maybe I can just allow users to specify how ImGui should be searched for (e.g. "ImGui" or "imgui"), so we'll have something like this:

if(NOT ${IMGUI_NAME}_DIR)
   ...
endif()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

vcpkg imgui doesn't provide version. Someone provided CMakeLists.txt without version: project(imgui CXX)

Copy link

Choose a reason for hiding this comment

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

I guess checking for version could be disabled in some cases... I'll thunk about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could make vcpkg's imgui to export imguiConfigVersion.cmake with write_basic_package_version_file but CMake requires COMPATIBILITY mode which if you choose SameMinorVersion|ExactVersion 1.68 will not be compatible with 1.70, if you choose AnyNewerVersion|SameMajorVersion then 1.00 may be compat with 1.70 which is wrong, as new imgui doesn't guarantee backward compat between minor versions, so SameMinorVersion|ExactVersion should be compat mode here. However that will require imgui-sfml port to change imgui version everytime imgui updates new version, and it doesn't guarantee compatibility with newer verions...

Maybe you should include an exact verison of imgui, i.e. 1.68, in imgui-sfml, and don't depend on system's imgui anymore. Not sure if it's the right thing to do.

Copy link

Choose a reason for hiding this comment

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

ImGui is mostly stable in the part that ImGui-SFML uses. ImGui's widgets functions/interfaces can change, but that will break client's ImGui code, not ImGui-SFML's code.
So in my opinion requiring specific version of ImGui doesn't feel right.

+ # set(IMGUI_DIR "" CACHE PATH "imgui top-level directory")
+ # message(FATAL_ERROR "ImGui directory not found. Set IMGUI_ROOT to imgui's top-level path (containing 'imgui.h' and other files).\n")
+# endif()

# This uses FindImGui.cmake provided in ImGui-SFML repo for now
-find_package(ImGui 1.68 REQUIRED)
+find_package(imgui REQUIRED)
Copy link

Choose a reason for hiding this comment

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

This looks like ${IMGUI_NAME} variable would be useful again. Not sure what to do about the version, maybe vcpkg script for ImGui should set imgui_VERSION and I'll be able to compare it afterwards, not during find_package?


# these headers will be installed alongside ImGui-SFML
-set(IMGUI_PUBLIC_HEADERS
- ${IMGUI_INCLUDE_DIR}/imconfig.h
- ${IMGUI_INCLUDE_DIR}/imgui.h
- ${IMGUI_INCLUDE_DIR}/imgui_internal.h # not actually public, but users might need it
- ${IMGUI_INCLUDE_DIR}/imstb_rectpack.h
- ${IMGUI_INCLUDE_DIR}/imstb_textedit.h
- ${IMGUI_INCLUDE_DIR}/imstb_truetype.h
- ${IMGUI_INCLUDE_DIR}/misc/cpp/imgui_stdlib.h
-)
+# set(IMGUI_PUBLIC_HEADERS
Copy link

Choose a reason for hiding this comment

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

Not sure if this is needed for patch - yes, it won't be used, but patch should be as minimal as possible, in my opinion.

+ # ${IMGUI_INCLUDE_DIR}/imconfig.h
+ # ${IMGUI_INCLUDE_DIR}/imgui.h
+ # ${IMGUI_INCLUDE_DIR}/imgui_internal.h # not actually public, but users might need it
+ # ${IMGUI_INCLUDE_DIR}/imstb_rectpack.h
+ # ${IMGUI_INCLUDE_DIR}/imstb_textedit.h
+ # ${IMGUI_INCLUDE_DIR}/imstb_truetype.h
+ # ${IMGUI_INCLUDE_DIR}/misc/cpp/imgui_stdlib.h
+# )

# CMake 3.11 and later prefer to choose GLVND, but we choose legacy OpenGL just because it's safer
# (unless the OpenGL_GL_PREFERENCE was explicitly set)
@@ -80,6 +80,7 @@ add_library(ImGui-SFML::ImGui-SFML ALIAS ImGui-SFML)

target_link_libraries(ImGui-SFML
PUBLIC
+ imgui::imgui
sfml-graphics
sfml-system
sfml-window
diff --git a/imconfig-SFML.h b/imconfig-SFML.h
index f66ba20..0f43ce6 100644
--- a/imconfig-SFML.h
+++ b/imconfig-SFML.h
@@ -28,5 +28,3 @@
static_cast<sf::Uint8>(z * 255.f), \
static_cast<sf::Uint8>(w * 255.f)); \
}
-
-#define ImTextureID unsigned int
Copy link

Choose a reason for hiding this comment

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

Why is this line needed? ImTextureID should really be "unsigned int" for ImGui-SFML, not "void*"

Copy link
Contributor Author

@tntxtnt tntxtnt Jul 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

It creates a linkage error with static build of vcpkg's imgui. C++ name mangling magic treats imgui static compiled Imgui::Image(... and imgui-sfml Imgui::Image(unsigned int, ... differently so you either have to compile imgui with imgui-sfml (which you can't in this case, vcpkg's static compiled imgui), or don't define it and use imgui's typedef void* ImTextureID;

Maybe include your own version of imgui in imgui-sfml, just need a few files...

Copy link

Choose a reason for hiding this comment

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

I see. Well, I hope that doesn't break something.

Also yeah, it looks like using external 'imgui' package with imgui-sfml is kinda painful... I've thought about adding ImGui as git submodule for ImGui-SFML here: SFML/imgui-sfml#81 ... and maybe it'll make the whole process easier if you don't depend on ImGui's vckpg package.

\ No newline at end of file