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 Freetype feature #11919

Merged
merged 1 commit into from
Jun 23, 2020
Merged

[imgui] Add Freetype feature #11919

merged 1 commit into from
Jun 23, 2020

Conversation

RT2Code
Copy link
Contributor

@RT2Code RT2Code commented Jun 13, 2020

-Add Freetype feature, which use Freetype instead of stb_truetype to build the font atlases (https:/ocornut/imgui/tree/master/misc/freetype).

-Add imgui_internal.h to public includes, because it is required to use the experimental API and improve debug capabilities. It's also required to build Implot (#11920).

-Add imgui_stdlib files to allow std::string usage with Imgui text functions (ocornut/imgui#2035).

-Fix bindings feature requiring SDL1 dependency instead of SDL2 (Imgui SDL binding is using the latter).

@RT2Code
Copy link
Contributor Author

RT2Code commented Jun 13, 2020

I'd also like to suggest to remove or at least split the bindings feature. Imgui bindings are not really part of the library, and including them all in the same feature require many dependencies to be fetched, which is probably overkill in most cases. Also copying cpp files to the include folder should be avoided.

@JackBoosY JackBoosY added category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:reviewed Pull Request changes follow basic guidelines requires:discussion labels Jun 15, 2020
@vicroms
Copy link
Member

vicroms commented Jun 23, 2020

Thanks for the PR!

@vicroms
Copy link
Member

vicroms commented Jun 23, 2020

I'm not familiar with imgui, are the bindings just example code? Or what is their purpose?

I think having them as a non-default feature is good enough, some ports already have "examples" features that do the same.

@vicroms vicroms merged commit 8e9ee5d into microsoft:master Jun 23, 2020
@RT2Code
Copy link
Contributor Author

RT2Code commented Jul 1, 2020

These bindings are very well maintained, feature complete and the author recommend to use them by default except if a custom implementation is really needed : https:/ocornut/imgui#integration

I think it's a good thing they are available as features for this port. What I dislike is the inability to pick only the required ones, it's all or nothing. Because of this, you are forced to acquire a lot of dependencies you will probably never need. It's also ugly because it isn't compiled to libs but rely instead on the manual inclusion of cpp files in your project, because some of them depends on platform specific APIs that can't be compiled everywhere.

Splitting these bindings to different features would solve these problems. The only drawback is that we would have a very long list of features :

  • Allegro5
  • Dx9
  • Dx10
  • Dx11
  • Dx12
  • Glfw
  • Glut
  • Marmalade
  • Metal
  • Opengl2
  • Opengl3
  • Osx
  • Sdl
  • Vulkan
  • Win32

I don't know if it's acceptable for vcpkg ports to have such a long feature list, but if it is, it would be a better solution than the current one, and I'd be happy to add it.

ras0219-msft pushed a commit that referenced this pull request Jul 7, 2020
* [implot] Initial port

Requires #11919

* [implot] Use find_package instead of find_path function

* [implot] Update to 0.3 version

* Update CONTROL

* [implot] Add linkage check

* Update CMakeLists.txt

* Revert 962e30a

* [implot] Fix INSTALL_INTERFACE path case and use find_package Config mode

Co-authored-by: Lily <[email protected]>
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 info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants