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] Split bindings to multiple features #12329

Merged
merged 3 commits into from
Jul 15, 2020
Merged

[imgui] Split bindings to multiple features #12329

merged 3 commits into from
Jul 15, 2020

Conversation

RT2Code
Copy link
Contributor

@RT2Code RT2Code commented Jul 8, 2020

See #12250.

I tested all the features on x86-windows, x64-windows, x64-windows-static and x64-linux triplets. Everything build fine except:

Contrary to the previous port which simply copied the sources to a specific folder, bindings are now compiled with their dependencies. The only exception is Marmelade, because there's no port available for it yet.

Unfortunately, I can't test the OSX and metal bindings. If someone using this platform could test it, it would be very much appreciated. :)

See #12250.

I tested all the features on x86-windows, x64-windows, x64-windows-static and x64-linux triplets. Everything build fine except:
- dx12-binding on x86-windows. There's an issue opened about it here : ocornut/imgui#2406
- allegro5-binding on x64-linux, because the current allegro5 port fails to build on this platform.

Contrary to the previous port which simply copied the sources to a specific folder, bindings are now compiled with their dependencies. The only exception is Marmelade, because there's no port available for it yet.

Unfortunately, I can't test the OSX and metal bindings. If someone using this platform could test it, it would be very much appreciated. :)
@JackBoosY
Copy link
Contributor

Imgui officials tend to set this port as header only and export it as an interface port.
Therefore:

  1. many ports that depend on it use its .h and .cpp directly to use its functions.
  2. imgui did not declare any symbols to be exported in the header file.

So we have to be more careful about imgui changes.

ports/portfile.cmake Outdated Show resolved Hide resolved
@JackBoosY JackBoosY added requires:author-response category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist labels Jul 10, 2020
ports/imgui/CONTROL Outdated Show resolved Hide resolved
if(IMGUI_BUILD_ALLEGRO5_BINDING)
find_path(ALLEGRO5_INCLUDE_DIRS allegro5/allegro.h)
target_include_directories(${PROJECT_NAME} PRIVATE ${ALLEGRO5_INCLUDE_DIRS})
target_sources(${PROJECT_NAME} PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/examples/imgui_impl_allegro5.cpp)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if these sample source files should be added to the imgui target.

@ras0219-msft
Copy link
Contributor

I'm not sure if these sample source files should be added to the imgui target.

Due to the somewhat unique structure of imgui, I think this is the best approach we have. The other reasonable alternative would be to turn every backend into a separate port + separate static lib, which would nicely enable adding and removing backends without recompiling everyone using imgui, but that is a lot of boilerplate for something mostly equivalent.

@ras0219-msft ras0219-msft merged commit 0b78ddc into microsoft:master Jul 15, 2020
@ras0219-msft
Copy link
Contributor

LGTM, thanks for the awesome improvement!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants