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] Add feature bindings and remove feature example #10253

Merged

Conversation

JackBoosY
Copy link
Contributor

@JackBoosY JackBoosY commented Feb 28, 2020

Since the binary generated by examples is rarely used, remove feature examples and add feature bindings.

Related: #10252 #8788.

Note: because the current feature tools still have generation problems, this PR only performs a simple separation feature.

All features passed in the following triplets test:

  • x86-windows
  • x64-windows
  • x64-windows-static

@JackBoosY JackBoosY added the info:internal This PR or Issue was filed by the vcpkg team. label Feb 28, 2020
@JackBoosY JackBoosY marked this pull request as ready for review February 28, 2020 12:45
@idcrook
Copy link

idcrook commented Feb 29, 2020

I found this issue while trying to resolve imgui install issue.

The header files and source files from imgui examples are used for bindings to call into imgui. And they sometimes change with imgui versions. Alongside examples and tools, there should be "bindings" included with result artifacts so that they can be found within the package.

$ ls -1 examples/imgui_impl_opengl3.* examples/imgui_impl_glfw.*
examples/imgui_impl_glfw.cpp
examples/imgui_impl_glfw.h
examples/imgui_impl_opengl3.cpp
examples/imgui_impl_opengl3.h

There is no build or binary required from these, so it should be safe to always include in result.
so something resembling

    # Install bindings
    file(GLOB IMGUI_BINDINGS ${SOURCE_PATH}/examples/*.h ${SOURCE_PATH}/examples/*.cpp)
    file(INSTALL ${IMGUI_BINDINGS} DESTINATION ${CURRENT_PACKAGES_DIR}/resources)

I used resources there as a placeholder for resources or bindings or what have you.

EDIT

example from conan version
https://github.com/conan-io/conan-center-index/blob/master/recipes/imgui/all/conanfile.py

self.copy(pattern="examples/imgui_impl_*", dst="res/bindings", src=self._source_subfolder, keep_path=False)

so a more appropriate GLOB might be

file(GLOB IMGUI_BINDINGS ${SOURCE_PATH}/examples/imgui_impl_* )

@idcrook
Copy link

idcrook commented Feb 29, 2020

Note: for thesebindings to work, it also relies on imgui_internal.h being available

which relies on other include files too, like imstb_textedit.h

@idcrook
Copy link

idcrook commented Mar 1, 2020

I have an example branch off master that works for demonstrating a "bindings" feature

Under a new feature [bindings]:

  • copies imgui_impl_* to bindings
    • installing under share or share/imgui, etc. did not seem to "feel right"
  • installs "internal" headers needed for compilation usings these bindings to include

https://github.com/idcrook/vcpkg/tree/imgui-trials

i was able to test in a build where these bindings from a vcpkg install are used.

@JackBoosY
Copy link
Contributor Author

@idcrook Already follow your suggestion, but I prefer to install it's code files to include/bindings.

@JackBoosY JackBoosY requested a review from PhoebeHui March 3, 2020 09:36
@JackBoosY JackBoosY changed the title [imgui] Separate feature tools from feature example [imgui] Add feature bindings and remove feature example Mar 3, 2020
@idcrook
Copy link

idcrook commented Mar 3, 2020

@idcrook Already follow your suggestion, but I prefer to install it's code files to include/bindings.

looks good to me

@idcrook
Copy link

idcrook commented Mar 6, 2020

After looking over again, the build dependencies should be empty for “bindings” feature in the control file, as those are wrappers into imgui, and those files are part of imgui distribution.

If there is an “examples” feature still included, that would be where those build dependencies would probably go…

Copy link
Contributor

@PhoebeHui PhoebeHui left a comment

Choose a reason for hiding this comment

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

LGTM

@PhoebeHui
Copy link
Contributor

/azp run

@PhoebeHui PhoebeHui added the info:reviewed Pull Request changes follow basic guidelines label Mar 24, 2020
@helynranta
Copy link
Contributor

helynranta commented Apr 11, 2020

Just dropping in a suggestion here (basically same thing that idcrook said):
If user wants the official bindings for sdl for example, he gets all of these other frameworks in the same package. There is really no point in having glfw3, sdl and allegro linked in same project.

My suggestion, to make this feature usable, is to provide CMake variable for source root of the bindings folder so that user could choose a bindings to build with and add it to include directories. Or something similar.

@JackBoosY
Copy link
Contributor Author

@lerppana All files in buildtrees are temporary, we can only install the bindings source files in installed and then use them.

@helynranta
Copy link
Contributor

Yeah sorry for my messed up explanation. What I mean is as a port user to use binding source files from vcpkg installed folder in my projects build (better explained what I mean? 😄). So whatever this feature is, without build-depends (see what idcrooks comment)

@strega-nil
Copy link
Contributor

This looks good to me :)

@strega-nil
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@strega-nil strega-nil merged commit 7db2ffa into microsoft:master May 1, 2020
@JackBoosY JackBoosY deleted the dev/jack/separate_imgui_feature_test branch May 6, 2020 04:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
info:internal This PR or Issue was filed by the vcpkg team. info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants