Implot v0.9 binding #2
Conversation
|
It looks awesome! I will try it when I'll have a little time ;) Concerning the big question of releasing it as a separate project or merging it in pyimgui, I am not sure of the answer. In my humble opinion, it makes more sense to have them as separate projects so that the interested user can choose to take it or not. But as you pointed out this leads to lots of questions concerning the sharing of code at cython level. It is an important question and it feels really similar to the one about the 'docking' branch of DearImGui. For now, in this upgrade branch, we have two submodules: In order to be sure that each submodule constantly own a pointer to the correct context we have to propagate it from Having two copies of DearImGui is clearly a waste of memory and does not scale if we want to add more submodules, but I couldn't see a better way to deal with it at the time. This may change in the future. I wonder if we could keep a centralized statically linked version of DearImGui and provide a way to point to it. In that case, submodules could ask it and, if we make it public, external modules will be able too. This raise question about how to implement this using cython and how to deal with DearImGui context management without starting a callback war. I truly don't know what is best, these are my thoughts for now and any comment is welcome. I will try to invest time in that reflection. If the need for external addons like pyplot is here, we shall think about it! |
|
@KinoxKlark thank you for your input!
Exactly this: sharing code on cython level, between projects, seems the main point of congestion, IMO. I'm not aware docking branch issue and how it relates, though.
Curious: why are the internals exposed over cython?
ImPlot has this on C++ level and I had to use it in order to make things work. In the Without this ImPlot reference to ImGui context remains NULL and segfaults are imminent.
If I imagine that the ImPlot would be a separate project, IMHO it means that access to cython parts of the pyimgui are off the table. Then I would expect that ImGui and ImPlot C++ sources need to be compiled, and cherry picked core, enums and possibly internals from pyimgui would need to be replicated (not sure how big of an amount). Since the ImGui is not usually built into a shared library (i.e. for distribution) neither pyimgui nor pyimplot can avoid compiling ImGui separately into their binding. Do we want to postulate that the ImGui would need to be provided as a (system wide) shared library for these two bindings to work? I would guess not. The use cases for ImGui are quite different from other GUI toolkits. OTOH, I'm also trying to understand how would pyimplot be able to access ImGui shared library if it would be provided by the pyimgui, instead of having it system wide. If this would somehow work, we would cut out lots of code duplication since ImGui core and internals would be in a form of a shared library. This still does not solve the access to pyimgui cython parts that pyimplot might need.
I can think of file dialogs, or some other addons that are kept outside of ImGui, but you're probably aware of those yourself. Not that these are something I actually use today in python, so no hurry there. As an exercise, I'll try to split the ImPlot from pyimgui as seen in this PR, just to see how that would look like. I'm also intrigued to see if bundling shared ImGui with pyimgui would be of benefit for pyimplot. Last but not least, I need educate myself if cython code sharing between modules/projects is a thing to pursue. Thank you for your time! |
|
Splitting the ImPlot out of the pyimgui here. Looks promising. I had to define This is what happens: This is similar to what you have done with Any ideas on how to overcome this? |
Some specific internal manipulation where requested. With official dearimgui it is possible to disable fields using internal flags. The internal exposition has been done to offer this possibility to python users, a detailed example here: pyimgui#192 (comment). Clearly, not all internals should be mapped, I think the idea was to start the foundations of submodules separation so that it would become possible to map other things as needed. Before that, there was no submodules separation.
I think it would require some adjustment on the pyimgui side but it seems feasible. The idea would be to extract the declaration of I designed a quick test to check the concept: I created two folders My file tree looks like this: I had to copy the For reference here are my test files: # A/setup.py
from setuptools import setup
from Cython.Build import cythonize
setup(ext_modules=cythonize(["module_a.pyx"]))# A/module_a.pxd
cdef class CommonContext:
cdef int ctx# A/module_a.pyx
cdef class CommonContext:
def __cinit__(self, int ctx):
self.ctx = ctx
def create_context():
cdef CommonContext cc = CommonContext(42)
print("Context from A:", cc.ctx)
return cc# B/setup.py
from setuptools import setup
from Cython.Build import cythonize
setup(ext_modules=cythonize(["module_b.pyx"]))# B/module_b.pyx
cimport module_a
import module_a # necessary to find the .pyd
def set_context(module_a.CommonContext cc):
print("Context from B", cc.ctx)I then compiled both projects. Note that for convenience I manually copied the compiled A quick python test gives: Thus, the This may not solve the static library duplication problem but it seems that it could help with cython specific sharing declaration. I will spend more time in the coming days investigating this. What are the pyimgui cython definition that you may need to access other than |
|
Thanks for the help. I can run the example and see it work as advertised. I added this to Commented out the Files/folders: Results: The names of the argument now match, but python seems confused and still thinks they are not the same :) ?! Need to dig deeper .. |
|
I think I will need to look at it and play with it. Can you push this here or in a branch of your fork please? (Sorry if you already did, I didn't found it) |
|
I've just pushed the code to https://github.com/hinxx/pyimplot. Also, https://github.com/hinxx/pyimgui/tree/changes-for-pyimplot holds the changes that are required for pyimplot to see _ImGuiContext.
You can see them in the pyimplot's |
|
Thanks! I looked at it and it seems that you need to I created a PR on your repo with my changes. Let me know if there are any issues! (Here for ref: hinxx/pyimplot#1) If this is working, we now have to deal with context updates. ImGui context can be destroyed, created, or changed at any time. One option is to let the user manually signal the change in context. As I understand it is already the case for cpp pyplot and cpp imgui. Unfortunately this leads to the question of dealing with context destruction and pointer access violation. Another option would be to hide this from the user and register callbacks from imgui to implot. Another question that should not be discarded is: How to manage pyimgui version requirements correctly? For now, any version of pyimgui can be used but (correct me if I am wrong) only one of them is used for compilation. This would imply that pyimplot should require exactly this version (e.g. in Note also that this seems to solve the linking problem between pyimgui and pyplot but you still have a copy of DearImGui compiled in your module. |
As noted in hinxx/pyimplot#1 (comment) your fix solved lots of issues in one sweep. NICE! Thank you again! 👍
You're way ahead of me on this one, and I sounds it needs addressing sooner or later. I'm assuming same context for the time being. Is there a simple use case you can describe when context would change and for what reason?
That was one of my early open questions. It might be that it does not matter (to an extent) if the versions differ, because both bindings carry their own compiled ImGui C++ source. I guess where it might break is if pyimgui passes over to pyimgui a data type that is different, and similar situations. Definitely needs more insight and addressing. Would use of ImGui shared library solve the issue? Some thoughts below.
Right. Working on this TypeError issue on my side I went and created Let me play around with this a bit more and report back! |
|
Here is the new branch for changes in pyimgui https://github.com/hinxx/pyimgui/tree/use-libimgui-so that uses shared library Both bindings still checkout the sources and compile them into a shared library that is placed into same folder as cython output. I had to export LD_LIBRARY_PATH before starting the python demo script (we can probably make this go away). Another pending optimization is the pyimplot In the process I managed to get rid of all the pyimgui pxd file copies initially introduced to pyimplot. Tested with the |
|
Introduced changes to pyimgui https://github.com/hinxx/pyimgui/tree/use-libimgui-so to void the need for setting the LD_LIBRARY_PATH with location of Additional changes in pyimplot commit hinxx/pyimplot@2b0db86 allow locating path to pyimgui provided Tested with the doc/examples/integrations_glfw3.py from the pyimplot that shows ImGui and ImPlot demo windows. |
Looking at this now. I did not find many uses of the Here, I'm assuming the script has control of both contexts and knows when they are being created/destroyed. It feels natural that the developer would take of the context updating explicitly. I'm trying to think of a use case where something more sophisticated / automated would be required, as opposed to manual context setting, but I'm coming up with nothing.
Can there exist multiple ImGui contexts in an app (script)? If so, could such "under the hood" pyimplot context changes lead to undesired effects? |
I have no direct example but I would have thought when you want to manage different windows you would need to manage different contexts for different windows. (Just a thought, not a piece of actual knowledge, I may be wrong about that.) I will need to go deeper into this to have a better idea. Hereafter a rapid search I found this: ocornut/imgui#586 (comment) (not sure if it can help the conversation but I drop them anyways).
That is my concern, if we don't know if the compiled Dear ImGui version is the same we can't be sure that the context we pass has the exact same definition. It may be updated in the future and using the newer version from the older code could lead to unexpected behavior.
I would say so. If ImGui was proposed as a shared library then we would be sure that everybody "speaks the same language". I did not have time yet to test your separation in a shared library. I hope to be able to check it at the beginning of next week. I was wondering if, instead of having a purely extern shared library, we couldn't compile ImGui only in By compiling your module with a I don't know if it can help but in // DLL users:
// - Heaps and globals are not shared across DLL boundaries!
// - You will need to call SetCurrentContext() + SetAllocatorFunctions() for each static/DLL boundary you are calling from.
// - Same apply for hot-reloading mechanisms that are reliant on reloading DLL (note that many hot-reloading mechanism works without DLL).
// - Using Dear ImGui via a shared library is not recommended, because of function call overhead and because we don't guarantee backward nor forward ABI compatibility.
// - Confused? In a debugger: add GImGui to your watch window and notice how its value changes depending on your current location (which DLL boundary you are in).
// Current context pointer. Implicitly used by all Dear ImGui functions. Always assumed to be != NULL.
// - ImGui::CreateContext() will automatically set this pointer if it is NULL.
// Change to a different context by calling ImGui::SetCurrentContext().
// - Important: Dear ImGui functions are not thread-safe because of this pointer.
// If you want thread-safety to allow N threads to access N different contexts:
// - Change this variable to use thread local storage so each thread can refer to a different context, in your imconfig.h:
// struct ImGuiContext;
// extern thread_local ImGuiContext* MyImGuiTLS;
// #define GImGui MyImGuiTLS
// And then define MyImGuiTLS in one of your cpp file. Note that thread_local is a C++11 keyword, earlier C++ uses compiler-specific keyword.
// - Future development aim to make this context pointer explicit to all calls. Also read https://github.com/ocornut/imgui/issues/586
// - If you need a finite number of contexts, you may compile and use multiple instances of the ImGui code from different namespace.
// - DLL users: read comments above.
I totally agree, context management responsibility should be given to the user. The problem is that (for now, in a non-shared version) the context has to be pass to each submodules. Changing the context in I really have no good insight for now and all this seems a bit blurry to me. I will need to dive deeper into it and do some tests. I also will try your shared solution. Thanks for your help! |
Interesting. This might work. in
Sounds sane.
No worries. Take you time! Note to self. We are tackling couple of items:
|
|
Hey! I didn't forget you but I have been a little bit overbooked with some projects at university. Sorry for the lack of news. I couldn't check it yet. Any news on your side? |
|
I was literally thinking of dropping you a line on this subject today! 😆 Hope your university projects went OK! I have not been looking into this for the last three weeks either. I'll need to remind myself of the state of the matters this week again. I recall last thing that I was doing is addressing the last bullet point:
I've copied the ImGui C++ headers to installation folder of pyuimgui and was able to use them from pyimplot thus avoiding the whole ImGui rebuild. With this and the use of Essentially all four bullet points were addressed and I think all the changes for that are in my fork/branch and in pyimplot repo. Can you take a look at that? I'll have some time this week to revive this effort, too. |
|
I checked your implementation and it sounds promising. Unfortunately, I work on windows and I didn't try to adapt the compilation to test it properly. We can try to set up different compilation pipelines to compile the dll on various platforms but I must admit that I am not very fond of compiling it with a separate pipeline. It seems to be a lot of work to ensure that it will always work on all targeted platforms simultaneously as the pyimgui module compilation. Having it in an external dll also raises the question of distribution. I believe we could include the dll directly into the package using the 'package_data' field of 'setup()' function. I think that it would be nicer if imgui was directly compiled into a 'core' pyimgui and accessed as an external library from other modules. Thus I took time to dig a bit into that. Here is what I have learned: Cython offers the possibility to 'cimport' C declaration from other modules. As I understand it, it is equivalent to 'cimport' the module '.pxd' if it exists. Additional '.pxd' files can be made accessible by including them in the module using 'package_data' from 'setup()'. As you probably know, '.pxd' acts as a header file for cython. If I have a module 'A' with some C declaration in 'A.pxd' and the appropriate implementation in 'A.pyx', then, once compiled in a dll and packed as an importable module I can 'cimport' any C declaration of 'A.pxd' into another module 'B', let's say 'B.pyx'. Here 'B' can be compiled and packed and it will refer to the implementation of 'A' without duplicating the implementation. (This is what we already explore before for the context management from Cython). The problem here is that only the C functions implemented in the 'A.pyx' are correctly linked and available from external modules. If you 'cdef extern "xxx.h"' a function in 'A.pxd' and compile the module 'A' statically linking the correct '.c' files you will be able to use this function in that compilation context (e.g. in 'A.pyx'), but not in external module 'B'. To be precise, in 'B' you will be able to 'cimport' the 'A.pxd' containing the prototype of the function and you will be able to use it (which is nice if this is the prototype of a class that you only want to make reference to), but as soon as you try to call it will crash as it can't find the function implementation, even if the implementation is compiled in 'A'. Fortunately, there is a way of making it works. What should be understood is that the 'cdef extern "xxx.h"' does not create à conventional C declaration but defer an '#include' which will include the desired declaration from 'xxx.h'. It can in fact also be used on 'xxx.c' to force the inclusion of the implementation. The idea is thus to do a 'cdef extern "xxx.c"' into the '.pyx' (and not the '.pxd') forcing the external C function to be compiled inside the module 'A' as any 'cdef' cython function. To be able to import it in another module we have to declare it in 'A.pxd' as any 'cdef' cython function (Not as an 'extern' anymore!). Doing so, module 'A' can be 'cimport' from module 'B' and the external C function can be referred to and called in 'B' through 'A'. (For reference, here is where I found this trick: https://docs.cython.org/en/latest/src/userguide/external_C_code.html#implementing-functions-in-c) For this to work the C function in 'xxx.c' has to be declared as 'static'. Thus it should be possible to compile a unique version of imgui into the 'pyimgui.core' (or another submodule, to be discussed) and use directly pyimgui as the dll for accessing it in external modules. This can also be used to clean the code duplication in different submodules of pyimgui such as 'pyimgui.internals'. This is a proof of concept and it definitely needs a more careful investigation on how to do it for pyimgui. Unfortunately, that is all the time that I have this week and this will have to wait a little bit from my side. I hope this is helpful! |
|
I think I understand your idea behind going with
I guess this would require some changes to original ImGui sources? Maybe just defining
Yes, this was my intent, too.
What is meant here with the 'separate pipeline'? Are you referring to adding another binary wheel in addition to those already supported by ie. https://pypi.org/project/imgui/? Or are you referring to some kind of CI pipeline? This is kind of related to the distribution, in my opinion, let's see if I understand this how the shared library concept would work. Is there a middle ground? If we imagine that pyimgui wheels distributed the precompiled ImGui shared library for a handful of OS / python combinations (as it does now), would that be acceptable to you? If the wheel is missing user needs to locally build the pyimgui using cython. No changes there, so far, except for adding a dll/so to the binary package. Any depending modules, like pyimplot for example, would highly likely need to specify which pyimgui release they need to use (and implicitly which ImGui C++ code / API). Those external modules would need to setup their CI scripts such that their binary wheels are build with correct pyimgui / python (this is also the case is not using shared shared libraries). External modules are free to provide a handful of binary wheels, just as in case with pyimgui. For the missing OS / python combinations or specific pyimgui release (without a binary wheel) local compilation is required. Anyway, I'm willing to go with whatever makes our lives easier and we should definitely explore the path you propose! |
|
I took some time to quickly hack the pyimgui code as you proposed. I was able to run the demo See https://github.com/hinxx/pyimgui/tree/embedd if you're interested. EDIT: As soon as I started adding declarations to |
I'm just throwing this out here. This is supposed to be cython baed ImPlot (https://github.com/epezent/implot) binding for python. It is based off pyimgui in upgrade/v1.79 branch (v1.82 at this point).
I don't really know where to ask the questions about how to proceed with this; going for
pyimplotas a separate project or offering it as an inclusion intopyimgui. Referencing the first mention of this in pyimgui#192 (comment).Read on if you are interested..
Have a look at the
doc/examples/integrations_glfw3_implot.pythat includes ImPlotdemo. Note that the demo was NOT ported to python yet.
It is not clear to me if this is the best place for ImPlot python binding. The
question remains if it would be better to place this code into a separate project,
or provide it as part of pyimgui. As of now the latter has been chosen due to my
laziness, poor undestanding of cython and also due to some obstacles
encountered during the process of creating this binding.
Read on if you are interested in couple of details.
As per ImGui and ImPlot documentation using them both as shared library is not recomended.
In case of python binding this is exactly what is being done. The ImGui part ends up being
a shared library and plot becomes a separate shared library. Creating a single shared library
(with ImGui and ImPlot) does not sound like a good idea at all so lets not go there. Not going
with shared library is also something that we can not do; AFAICT due to the way cypthon does
its business (I might be wrong).
At the level of C++ that cython wraps into python module and functions, ImPlot want access
to
imgui.handimgui_internal.h. For example ImPlot usesImTextureID,ImGuiCond,ImGuiMouseButton,ImGuiKeyModFlags,ImGuiDragDropFlags,ImU32,ImDrawList,ImGuiContext,ImVec2,ImVec4and alike. These need to be exposed a cython level, too. Currently,these come from
cimguiandinternalmodules provided by pyimgui binding. Using them isas simple as adding a
cimportline to acimplot.pxd. pyimgui (c)imports were requiered forplot.pyxas well:If the ImPlot is placed in a separate project/repo these would need to be redefined.
When I tried to compile the ImPlot C++ code for cython binding (without adding ImGui sources)
for that shared library, doing
import imguiin a user script fails withGImGuibeing undefined.Have I missed something there during the library build?! I don't know.
If the ImPlot binding is separate from ImGui binding (cleanest approach), I'm not sure how
the user script would behave if pyimgui and pyimplot would be based of different ImGui C++ code.
Might be a non-issue, but I just do not know.
Have any ideas or suggestions on the above topics? Bring them up, please!