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

Add simple CMake support for imgui #5394

Closed
wants to merge 0 commits into from

Conversation

jiapei100
Copy link

  1. Add simple CMakeLists.txt to build imgui into a shared library, and add the options to build a couple of more backend modules, as well as examples.
  2. SDL is now supporting version 2, namely: SDL2.

@rokups
Copy link
Contributor

rokups commented Jun 14, 2022

Cool that you chose to implement build in a single file, not littering entire project with CMake-isms! There already were other attempts, adding them for reference: #4614, #3027, #2820, #2033, #1778, #1713, #1573, #255, CMakeLists.txt. Wow, this was a lot. Seems like there is a persistent demand, even though building Dear ImGui does not require any special build steps and is easy to include as part of your project, whatever build system is used.

This PR is by no means complete though. I love simplicity of this build script, but unfortunately it will not be enough to build all examples on most supported operating systems. If you look at my linked CMakeLists.txt you would find that (unfortunately) there are too many ways to find dependencies. All these ways are unfortunately necessary to take into account, because a combination of each supported OS + CMake version has or does not have certain bits. For example, you use PkgConfig cmake package. This will not work on windows when targeting MSVC. Certain dependencies on certain OS can be found via pkg-config, other dependencies wont have .pc script, but can be found through CMake's find_package(). Sometimes it is useful to be able to build these dependencies from source. Also one must consider that users would use this build script in their projects, therefore we must support usecase through add_subdirectory(). In such cases users may want to use dependencies that already exist in their project (for example SDL/GLFW), but library targets may have different names. Apple's examples deserve a special pot in hell, i have not been able to fully sort out fully building anything beyond example_apple_opengl2. And so list goes on... This sounds like a simple problem, but when you start to consider all edgecases it spirals out of control 😞

SDL is now supporting version 2, namely: SDL2.

This is incorrect use of SDL. By convention users must import SDL with it's special include paths:

~ % pkg-config --cflags sdl2   
-I/usr/include/SDL2 -D_REENTRANT

You on the other hand changed includes in a way that depends on /usr/include being available in include path, disregarding windows.

set(CMAKE_CXX_STANDARD 14) # Enable c++14 standard

Dear ImGui requires C++11, there is no point to force a higher standard version. Also care must be taken to allow users to set higher version should they desire. This would be best done through target_compile_features().

include_directories(./)

This is a big no-no. With modern CMake we use target-based approach, assigning include paths to targets. You should use target_include_directories() and other calls starting with target_ so we would not have to rewrite build script later.

All in all this build script only covers a tiny fraction of usecases - building of examples for demonstration purposes on Linux. A lot more work is still required. But maybe its a good start! 👍

CMakeLists.txt Outdated
Comment on lines 6 to 8
# You may have to manually set CMAKE_C_COMPILER and CMAKE_CXX_COMPILER respectively to clang
# set(CMAKE_C_COMPILER "/usr/local/bin/clang")
# set(CMAKE_CXX_COMPILER "/usr/local/bin/clang++")
Copy link

Choose a reason for hiding this comment

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

This has nothing to do with this repo. It is recommended to put this on a toolchain file, the user can do this on it's side

CMakeLists.txt Outdated
@@ -0,0 +1,275 @@
cmake_minimum_required(VERSION 3.13) # CMake version check
project(imgui) # Create project "imgui"
set(CMAKE_CXX_STANDARD 14) # Enable c++14 standard
Copy link

Choose a reason for hiding this comment

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

Two things with this

  • This is expected to be 11
  • This must be set at the target level. You are polluting the global scope!

CMakeLists.txt Outdated
option(BUILD_SHARED_LIBS "Build using shared libraries" ON)

# Add library target with source files listed in SOURCE_FILES variable
add_library(imgui ${SOURCE_FILES})
Copy link

Choose a reason for hiding this comment

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

List the sources directly here

CMakeLists.txt Outdated


# Add all .cpp files of project root directory as source file
set(SOURCE_FILES imgui.cpp imgui_draw.cpp imgui_demo.cpp imgui_tables.cpp imgui_widgets.cpp) # Add all .cpp files of project root directory as source files
Copy link

Choose a reason for hiding this comment

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

Suggested change
set(SOURCE_FILES imgui.cpp imgui_draw.cpp imgui_demo.cpp imgui_tables.cpp imgui_widgets.cpp) # Add all .cpp files of project root directory as source files

CMakeLists.txt Outdated
# Add all .cpp files of project root directory as source file
set(SOURCE_FILES imgui.cpp imgui_draw.cpp imgui_demo.cpp imgui_tables.cpp imgui_widgets.cpp) # Add all .cpp files of project root directory as source files

option(BUILD_SHARED_LIBS "Build using shared libraries" ON)
Copy link

Choose a reason for hiding this comment

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

Put all options together and as high as possible on the file

CMakeLists.txt Outdated
Comment on lines 39 to 40
set(BACKEND_FILE ./backends/imgui_impl_allegro5.cpp)
add_library(imgui_backend_allegro5 ${BACKEND_FILE})
Copy link

Choose a reason for hiding this comment

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

Suggested change
set(BACKEND_FILE ./backends/imgui_impl_allegro5.cpp)
add_library(imgui_backend_allegro5 ${BACKEND_FILE})
add_library(imgui_backend_allegro5 backends/imgui_impl_allegro5.cpp)

CMakeLists.txt Outdated
find_package(PkgConfig REQUIRED) # For .pc packages


include_directories(./backends)
Copy link

Choose a reason for hiding this comment

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

Use target version

CMakeLists.txt Outdated
Comment on lines 154 to 155
set(EXAMPLE_MAIN_FILE ./examples/example_apple_opengl2/main.mm)
add_executable(example_apple_opengl2 ${EXAMPLE_MAIN_FILE})
Copy link

Choose a reason for hiding this comment

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

Suggested change
set(EXAMPLE_MAIN_FILE ./examples/example_apple_opengl2/main.mm)
add_executable(example_apple_opengl2 ${EXAMPLE_MAIN_FILE})
add_executable(example_apple_opengl2 examples/example_apple_opengl2/main.mm)

CMakeLists.txt Outdated


include_directories(./backends)
if(${BUILD_EXAMPLE_ALLEGRO5})
Copy link

Choose a reason for hiding this comment

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

Suggested change
if(${BUILD_EXAMPLE_ALLEGRO5})
if(BUILD_EXAMPLE_ALLEGRO5)

Comment on lines 67 to 68
#include <SDL2/SDL.h>
#include <SDL2/SDL_syswm.h>
Copy link

Choose a reason for hiding this comment

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

This is not needed with properly set targets

@Endilll
Copy link
Contributor

Endilll commented Aug 3, 2022

@rokups are there other reasons besides building examples on Apple why you haven't already created a PR out of your CMakeLists? It looks quite comprehensive and addresses at least some of the points you mentioned.

@rokups
Copy link
Contributor

rokups commented Aug 4, 2022

Build systems are a complicated can of worms. Adopting a build script implies maintenance. Current build infrastructure (vcxproj + Makefile) are not used by people so they only need to be maintained as much as we need to use them for testing or on CI. A comprehensive CMakeLists will be used by people so it will definitely be more work maintaining that as well. In the end it is pretty trivial for everyone to come up with a minimal CMakeLists for imgui, that fits their project needs. Even in my own sideprojects i just throw couple lines simple CMakeLists and use that instead of using my script as it is just simpler.

tl;dr; there is no real hard need for that so its on hold. maybe one day, maybe not, who knows \o/

CMakeLists.txt Outdated
Copy link

@SomeoneSerge SomeoneSerge Feb 14, 2024

Choose a reason for hiding this comment

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

even though building Dear ImGui does not require any special build steps and is easy to include as part of your project, whatever build system is used

Offtopic but this very much sounds like https://youtu.be/sBP17HQAQjk?t=318 🤣

I'm wondering if maybe at this point it's more sensible to just advertise vcpkg's (or equivalent) cmake rules for imgui. From bootstrapping and/or distributions' points of view, people vendoring imgui is an issue, because that is usually done in a way that prevents substitution/dependency injection. Just having standardized pkg-config/cmake target names (e.g. ones declared by vcpkg) that people could test for before falling back to their vendored stuff would already be quite helpful

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants