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

Allowing pyimgui to provide bindings to an externally built imgui library #229

Closed
wants to merge 9 commits into from

Conversation

pvallet
Copy link

@pvallet pvallet commented Apr 26, 2021

For a personal project, I need to be able to access imgui functions both from C++ and python.
In order to do so, I built imgui as a separate library, which I linked to my own python module and pyimgui.

This PR doesn't affect the current build workflow, it only adds another option to it.

I also allowed using pyimgui as a subpackage.

@@ -1,6 +1,3 @@
# distutils: language = c++
Copy link
Author

Choose a reason for hiding this comment

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

The source files were already set up from setup.py when not using cython, I made the behavior consistent by not defining any sources here. This allows providing only the source files you want.
The include directories were already provided through the Extension

@coveralls
Copy link

Coverage Status

Coverage remained the same at 55.95% when pulling 67ac962 on pvallet:master into 480b4de on swistakm:master.

@KinoxKlark
Copy link
Member

This looks great thanks! In fact, this relates to a discussion with @hinxx in KinoxKlark#2 where we are trying to define the best way of centralizing imgui implementation to avoid duplication in submodules and allowing access to external modules that would act as extensions (like implot that extends imgui). Providing an external dll for the implementation is considered. We also explore the possibility of C export the functions through the core module (allowing cimport from external modules). Your expertise on the subject may be of great help.

I am aware that in the current state it is not convenient to share context with external code since the context management class of pyimgui is not accessible from outside. But in a case where it would be easy to share the context pointer between pyimgui and your project, what would be pros and cons of external shared library vs context sharing in your case? (No good or bad answer, I am just looking for some insights)

@pvallet
Copy link
Author

pvallet commented Apr 26, 2021

Sharing context between two modules that have their own separate implementations of the imgui functions sounds weird to me. For instance, if you want to modify the implementation of some imgui functions in your package, calling the functions through pyimgui would just give the vanilla implementation instead of the one you modified, making the behavior inconsistent across modules. That might be the intended functionality, but in that case I guess you could share the rendering context instead?
I haven't used Imgui that much so I have a very limited knowledge about it, but does sharing context allow for sharing all imgui options, like font size and stuff?

About C import and C export, that seems to be a good solution for sharing code across Cython modules, but would not work with other kind of bindings I believe (I really do not know Cython that much). I'm using pybind11 myself for building my module and I have no idea if it is even possible to interface functions with Cython this way.

Take all I said with a grain of salt, I'm pretty new to the world of python bindings.

@pvallet pvallet marked this pull request as draft April 26, 2021 10:41
@pvallet
Copy link
Author

pvallet commented Apr 26, 2021

Putting this as a draft because I hadn't tested it on other platforms than Windows. Will update it.

@learn-more
Copy link
Collaborator

Please note that the bundled imgui code has a bonus feature that is not present upstream, in the form of the ansi color code support!
https://github.com/swistakm/pyimgui/tree/master/ansifeed-cpp

@pvallet
Copy link
Author

pvallet commented May 2, 2021

Hey!
So I took a look at what was broken for my build, and everything is fixed by the last commit I did, now my builds work on all platforms, provided I have cython installed (I'm not sure what the workflow is in setup.py when cython is not installed, but the returned errors are not explicit).

Please note that the bundled imgui code has a bonus feature that is not present upstream, in the form of the ansi color code support!
https://github.com/swistakm/pyimgui/tree/master/ansifeed-cpp

That's really nice info and I was actually not aware of this, but what is it that you wanted to point out with this? Building this should not be broken by my submit, which doesn't affect those files, and I made sure that they actually were compiled. If it's about building imgui in a DLL, it doesn't change its availability from pyimgui, as the ansi color code files are still compiled as part of pyimgui. In the current workflow, they would not be included in the DLL though, making them unavailable from C++, but I think this is ok since it makes the DLL "vanilla imgui", i.e. independent from pyimgui, which makes sense.

Also, I was thinking, we might want to make that workflow covered by some kind of CI. I was thinking about using something built in the same way as pysdl2-dll to use it to build pyimgui and then run tests. However, I'm not sure how much time I can dedicate to this in the short-term.
What do you all think about this? Is it the right approach, would it be necessary to have it up and running to merge that PR?

@pvallet pvallet marked this pull request as ready for review May 2, 2021 12:45
@learn-more
Copy link
Collaborator

That's really nice info and I was actually not aware of this, but what is it that you wanted to point out with this? Building this should not be broken by my submit, which doesn't affect those files, and I made sure that they actually were compiled. If it's about building imgui in a DLL, it doesn't change its availability from pyimgui, as the ansi color code files are still compiled as part of pyimgui. In the current workflow, they would not be included in the DLL though, making them unavailable from C++, but I think this is ok since it makes the DLL "vanilla imgui", i.e. independent from pyimgui, which makes sense.

Mainly pointing out that it exists, so you can validate that it does still work.

@pvallet
Copy link
Author

pvallet commented May 2, 2021

Yeah so I tested by running one of the integrations found in doc/examples, and if it's about printing the text in green in the small window, it does work. That thing should probably be covered by CI, or at least be mentioned in the contribution guildelines though.
image

@KinoxKlark KinoxKlark mentioned this pull request Nov 1, 2021
@KinoxKlark KinoxKlark changed the base branch from master to dev/version-2.0 November 13, 2021 13:41
@KinoxKlark KinoxKlark mentioned this pull request Nov 15, 2021
@KinoxKlark
Copy link
Member

Hey, sorry for the (very) late management of your PR. As you may have seen, I have created the dev/version-2.0 branch containing the update to DearImGui 1.82.

I just created from it a feature branch feature/dll in order to study correctly this option and I have merged your PR in it (up until the second to last commit: 439f6e3). A central PR for this project has been created as #247.

I close this PR since it is considered merged. Feel free to reopen it if you think I missed something. Thanks for the help!

@KinoxKlark KinoxKlark closed this Nov 15, 2021
@KinoxKlark KinoxKlark added the release pending Merged but still needs official release label Nov 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release pending Merged but still needs official release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants