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

tntxtnt · 2019-07-25T21:20:05Z

Static build only to match with imgui.

* Static build only to match with `imgui`.

eliasdaler

Thanks for submitting the PR. I feel like having ImGui-SFML on vcpkg would be helpful.
However, I feel like some things can be improved and maybe there'll be less patches required if ImGui-SFML fixes some of the problems you've had to work around.

eliasdaler · 2019-07-25T22:40:29Z

ports/imgui-sfml/require-c++11.patch

+ )
+
+set(CMAKE_CXX_STANDARD 11)
+set(CMAKE_CXX_STANDARD_REQUIRED ON)


Why is C++11 required? ImGui-SFML, ImGui and SFML can build with C++03 just fine.

Somehow in the x64-osx build I got this error:

In file included from /Users/vagrant/azure_agent/_work/3/s/buildtrees/imgui-sfml/src/v2.0.2-d5398dcc0e/imgui-SFML.cpp:2: /Users/vagrant/azure_agent/_work/3/s/installed/x64-osx/include/imgui.h:192:5: error: delegating constructors are permitted only in C++11 IM_VEC4_CLASS_EXTRA // Define additional constructors and implicit cast operators in imconfig.h to convert back and forth between your math types and ImVec4. ^~~~~~~~~~~~~~~~~~~ /Users/vagrant/azure_agent/_work/3/s/buildtrees/imgui-sfml/src/v2.0.2-d5398dcc0e/imconfig-SFML.h:22:11: note: expanded from macro 'IM_VEC4_CLASS_EXTRA' : ImVec4(c.r / 255.f, c.g / 255.f, c.b / 255.f, c.a / 255.f) { \ ^~~~~~

error: delegating constructors are permitted only in C++11

So I add C++11 to your CMakeLists. It's from imgui.h (?), I don't know why only x64-osx build failed with this. This is a quick and dirty patch to solve this problem.

Ah, so in imconfig-SFML.h you use delegating constructors, which is a C++11 feature:

#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) { \ } \

Could be (badly 😄) rewritten to:

#define IM_VEC4_CLASS_EXTRA \ ImVec4(const sf::Color & c) \ : x(c.r / 255.f), y(c.g / 255.f), z(c.b / 255.f), w(c.a / 255.f) { \ } \

Yes, this should be fixed in ImGui-SFML - will do!

eliasdaler · 2019-07-25T22:41:41Z

ports/imgui-sfml/static-build-with-vcpkg-imgui.patch

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


Not sure if this is necessary.

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)

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

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.

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

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.

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.

eliasdaler · 2019-07-25T22:43:22Z

ports/imgui-sfml/static-build-with-vcpkg-imgui.patch

+-  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)


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()

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

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

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.

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.

eliasdaler · 2019-07-25T22:45:07Z

ports/imgui-sfml/static-build-with-vcpkg-imgui.patch

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


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?

eliasdaler · 2019-07-25T22:45:53Z

ports/imgui-sfml/static-build-with-vcpkg-imgui.patch

+-  ${IMGUI_INCLUDE_DIR}/imstb_truetype.h
+-  ${IMGUI_INCLUDE_DIR}/misc/cpp/imgui_stdlib.h
+-)
+# set(IMGUI_PUBLIC_HEADERS


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.

eliasdaler · 2019-07-25T22:48:13Z

ports/imgui-sfml/static-build-with-vcpkg-imgui.patch

+     #if _WIN32
+         #ifdef IMGUI_SFML_EXPORTS
+             #define IMGUI_SFML_API __declspec(dllexport)
+-            #define IMGUI_API __declspec(dllexport)


Does this really affect anything? If ImGui-SFML is compiled statically, then IMGUI_SFML_EXPORTS should not be defined?

eliasdaler · 2019-07-25T22:49:18Z

ports/imgui-sfml/static-build-with-vcpkg-imgui.patch

+         #endif
+     #elif __GNUC__ >= 4
+         #define IMGUI_SFML_API __attribute__ ((visibility("default")))
+-        #define IMGUI_API __attribute__ ((visibility("default")))


Okay, maybe I'll need to add #ifndef IMGUI_API before this, but again, I wonder if it really affects anything.

You can include them but I think it's kinda weird to define another library's defines when you don't build that library (use vcpkg's imgui) so I removed them.

On second thought I should include them since imgui-sfml is imgui's replacement.

eliasdaler · 2019-07-26T10:40:20Z

ports/imgui-sfml/static-build-with-vcpkg-imgui.patch

@@ -69,31 +69,3 @@ index f66ba20..0f43ce6 100644
 -
 -#define ImTextureID unsigned int


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

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...

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.

dan-shaw · 2019-08-08T13:04:05Z

@tntxtnt @eliasdaler Is this ready to be merged?

tntxtnt · 2019-08-08T13:50:56Z

It's still missing misc/cpp/imgui_stdlib.cpp from imgui port but I think it's ok.

dan-shaw · 2019-09-03T21:46:30Z

Thanks for the PR! I've merged this, but it may be updated in #8004.

tntxtnt added 2 commits July 25, 2019 16:17

[imgui-sfml] Add new package ImGui-SFML

fd1365b

* Static build only to match with `imgui`.

[imgui-sfml] Fix case-sensitive imgui package name

03daa62

ras0219-msft assigned dan-shaw Jul 25, 2019

[imgui-sfml] Require C++11

f3ddb65

eliasdaler suggested changes Jul 25, 2019

View reviewed changes

[imgui-sfml] Include IMGUI_API define

0c257bb

eliasdaler reviewed Jul 26, 2019

View reviewed changes

eliasdaler mentioned this pull request Jul 26, 2019

Doesn't compile on C++03 because of delegated ctor SFML/imgui-sfml#97

Closed

This was referenced Jul 29, 2019

[imgui] Add sfml feature. #7421

Closed

[New Port Request] Imgui-sfml #7333

Closed

[imgui-sfml] Remove delegating ctor - C++11 is not required

8e86519

cenit mentioned this pull request Aug 31, 2019

[imgui-sfml] initial port #8004

Merged

dan-shaw merged commit 79b5a9a into microsoft:master Sep 3, 2019

eliasdaler mentioned this pull request Sep 4, 2019

vcpkg support SFML/imgui-sfml#96

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tntxtnt commented Jul 25, 2019

eliasdaler left a comment

eliasdaler Jul 25, 2019

tntxtnt Jul 25, 2019 •

edited

Loading

tntxtnt Jul 26, 2019

eliasdaler Jul 26, 2019

eliasdaler Jul 25, 2019

tntxtnt Jul 25, 2019 •

edited

Loading

tntxtnt Jul 25, 2019

eliasdaler Jul 26, 2019

tntxtnt Jul 26, 2019

tntxtnt Jul 26, 2019 •

edited

Loading

eliasdaler Jul 26, 2019

eliasdaler Jul 25, 2019

tntxtnt Jul 25, 2019

eliasdaler Jul 26, 2019

tntxtnt Jul 26, 2019

eliasdaler Jul 26, 2019

eliasdaler Jul 25, 2019

eliasdaler Jul 25, 2019

eliasdaler Jul 25, 2019

eliasdaler Jul 25, 2019

tntxtnt Jul 26, 2019 •

edited

Loading

tntxtnt Jul 26, 2019

eliasdaler Jul 26, 2019

tntxtnt Jul 26, 2019 •

edited

Loading

eliasdaler Jul 26, 2019

dan-shaw commented Aug 8, 2019

tntxtnt commented Aug 8, 2019

dan-shaw commented Sep 3, 2019

		@@ -69,31 +69,3 @@ index f66ba20..0f43ce6 100644
		-
		-#define ImTextureID unsigned int

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

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

Conversation

tntxtnt commented Jul 25, 2019

eliasdaler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tntxtnt Jul 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tntxtnt Jul 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tntxtnt Jul 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tntxtnt Jul 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tntxtnt Jul 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dan-shaw commented Aug 8, 2019

tntxtnt commented Aug 8, 2019

dan-shaw commented Sep 3, 2019

tntxtnt Jul 25, 2019 •

edited

Loading

tntxtnt Jul 25, 2019 •

edited

Loading

tntxtnt Jul 26, 2019 •

edited

Loading

tntxtnt Jul 26, 2019 •

edited

Loading

tntxtnt Jul 26, 2019 •

edited

Loading