-
Notifications
You must be signed in to change notification settings - Fork 57
This issue was moved to a discussion.
You can continue the conversation there. Go to discussion →
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
porting my app to imgui_bundle.imgui #14
Comments
To add, I guess you do not have to add classes like GlfwRenderer in pyimgui, but it would be nice if you could wrap the supported backend, so that you can write code very similar to C++, where i in python could call the equivalent of:
That also raises the question if there should perhaps be a bit more hierarchy in the lib, e.g., should I can give you access to the now private repo of my program by the way, should you find that helpful. |
Thanks for the detailed comments! This is the kind of feedback I appreciate. For the moment, I am busy solving a weird windows issue (see last comments in #7).
Hum, I need to ponder whether it is feasible and/or desirable. In the meantime, I added a warning in the readme, that says
Thanks, that was definitely one of the main goals, as well as to keep the header file documentation intact (can you see it?)
Hum, I think not, since users might want to do flags manipulations like: flags: ImGuiTableFlags = ImGuiTableFlags_.flag_a | ImGuiTableFlags_.flag_b
Hum, this one is not easy. For the moment, glfw is statically linked into the library.
Definitely, and I think it will be awesome. I will keep you posted once it is ready.
Hum, I had not thought of that. Would you be willing to do some tests on this on your side? void py_init_module_imgui_main(py::module& m)
{
// Add your manual bindings there, before the autogenerated code |
v0.6.6 is out. Support for callbacks was added (see links in release notes) Please tell me if it works. Have a nice day! |
Hi Pascal, I just looked through the diffs a bit, super cool! I really look forward to trying this out, hopefully still today. A debate: should Feedback soon! |
I see the enums still have an ImGui prefix. Did you run into some issue changing that? edit removed the |
However, both |
Did you git submodule update —init? |
Did it to be sure. It doesn't fix the pip build, still get missing include
even though `\imgui_bundle\external` is populated. But i'll wait for the
wheel.
…On Mon, Nov 21, 2022 at 1:51 PM Pascal Thomet ***@***.***> wrote:
Did you git submodule update —init?
—
Reply to this email directly, view it on GitHub
<#14 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AANUOGPM2Q63RH77NGQLQ7TWJNV5NANCNFSM6AAAAAASBDBRUY>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Humm.. Once again, I cannot reproduce the issue here. I messaged you by phone, tell me if you have a few minutes |
I don't right now, on the way out. Will try a fresh clone first, to be sure.
…On Mon, Nov 21, 2022 at 3:58 PM Pascal Thomet ***@***.***> wrote:
Humm.. Once again, I cannot reproduce the issue here.
It does work on my windows virtual machine with msvc.
I messaged you by phone, tell me if you have a few minutes
—
Reply to this email directly, view it on GitHub
<#14 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AANUOGJF7KJVZ2CFAYE3JGLWJOEZFANCNFSM6AAAAAASBDBRUY>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
k |
hmm, with clean clone and new venv it all builds just fine. Maybe some stale cache was left after i pulled the latest version? |
There is a folder named _skbuild where some compilation artifacts are stored. May be it was the culprit
|
Should the all in
? Further things i ran into (some repeats):
6 in particular is a blocking error for me, so i had to stop further porting. I have just committed step 2 (and hey, got a bunch further!) in my private repo that you have access to, so have a look what i did. |
In
? thats what imgui example code does |
Yes, I just corrected it; but this commit is on the dev branch |
Done in dev branch. all is here to make mypy happy.
Automatic renaming requires more effort than I originally thought, one has to take care and differentiate variable name, type names, function names, class and enum names when renaming, since they have all different naming conventions...
Well, an automatic generation for `const char * ImGuiIO::IniFilename" is hopeless, since it provides absolutely no storage.
I did not study this yet, although I am also interested in it.
IMGUI_API void GetTexDataAsAlpha8(unsigned char** out_pixels, int* out_width, int* out_height, int* out_bytes_per_pixel = NULL); // 1 byte per-pixel
IMGUI_API void GetTexDataAsRGBA32(unsigned char** out_pixels, int* out_width, int* out_height, int* out_bytes_per_pixel = NULL); Well, well, well. Those signatures with naked pointers and double pointers are hopeless for an autogenerated binding! I wrote a draft commit on the dev branch that adds Please try it. I have no client code to test it against on my side. If it works, can you please also add a signature for the Alpha and post it as a PR?
bool IsBuilt() const { return Fonts.Size > 0 && TexReady; } // Bit ambiguous: used to detect when
void SetTexID(ImTextureID id) { TexID = id; } Well, those functions are not marked as part of the ImGui API (they are missing the IMGUI_API marker). So, a manual binding will be required.
I suppose you do not really need ImWchar, but you need to be able to specify glyph ranges when calling I made a draft commit for this in the dev branch. Can you please test it on your side (and may post a PR where you remove the experimental suffix if all goes well). |
On small question: what did you mean when you wrote
|
Final note for today. If you add this // </litgen_pydef> // Autogenerated code end
// !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! AUTOGENERATED CODE END !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
pyClassImFontAtlas.def("add_font_from_file_ttf_experimental", fontAtlas_AddFontFromFileTTF);
at the end of pybind_imgui.cpp, then you get an additional method for ImFontAtlas in python. And if all goes well it might someday be a manual replacement of the autogenerated one. |
PyPI doesn't accept my current dependencies: |
My port is now available publicly at
https://github.com/dcnieho/glassesValidator/tree/imgui_bundle
Thanks for all the work you did with my comments! I'll have a good look at
them in the next few days.
… Message ID: ***@***.***>
|
Nice 🚀
I was happy to help. I look forward to reading your feedback Some other elements and questions:
(If I do rename the project, I will not remove the existing packages on pypi if you need them, of course, I will simply add a new one and mention the renaming in the readme)
|
I think the current name is best. It is clear and it contains imgui, which is the key part. If i see the name, i think "ah, imgui, and more". Isn't that just what you want? My vote is don't change. |
This is so very cool!
Thank you, that will be very nice to have. |
I will add named constructors when there is no user defined constructor. However, when there is one such as for ImFontConfig, it is much too dangerous to provide a named constructor: For example, let's look and ImFontConfig default constructor: ImFontConfig::ImFontConfig()
{
memset(this, 0, sizeof(*this));
FontDataOwnedByAtlas = true;
OversampleH = 3; // FIXME: 2 may be a better default?
OversampleV = 1;
GlyphMaxAdvanceX = FLT_MAX;
RasterizerMultiply = 1.0f;
EllipsisChar = (ImWchar)-1;
} ==> If we create a named constructor that use default value for all params types, we end-up overwriting the carefully selected default values. I was bitten hard by this, here and also in the node editor. I know I could may be investigate params being Optional[...], but I decided not to go into it. The generation is already complex and does enough on this side IMHO. |
Hum, this is due to a trick in imgui code: ImGuiSelectableFlags_SelectOnClick is not a member of imgui.h says: // Flags for ImGui::Selectable()
enum ImGuiSelectableFlags_
{
ImGuiSelectableFlags_None = 0,
ImGuiSelectableFlags_DontClosePopups = 1 << 0, // Clicking this don't close parent popup window
ImGuiSelectableFlags_SpanAllColumns = 1 << 1, // Selectable frame can span all columns (text will still fit in current column)
ImGuiSelectableFlags_AllowDoubleClick = 1 << 2, // Generate press events on double clicks too
ImGuiSelectableFlags_Disabled = 1 << 3, // Cannot be selected, display grayed out text
ImGuiSelectableFlags_AllowItemOverlap = 1 << 4 // (WIP) Hit testing to allow subsequent widgets to overlap this one
}; and imgui_internal says: // Extend ImGuiSelectableFlags_
enum ImGuiSelectableFlagsPrivate_
{
// NB: need to be in sync with last value of ImGuiSelectableFlags_
ImGuiSelectableFlags_NoHoldingActiveID = 1 << 20,
ImGuiSelectableFlags_SelectOnNav = 1 << 21, // (WIP) Auto-select when moved into. This is not exposed in public API as to handle multi-select and modifiers we will need user to explicitly control focus scope. May be replaced with a BeginSelection() API.
ImGuiSelectableFlags_SelectOnClick = 1 << 22, // Override button behavior to react on Click (default is Click+Release)
ImGuiSelectableFlags_SelectOnRelease = 1 << 23, // Override button behavior to react on Release (default is Click+Release)
ImGuiSelectableFlags_SpanAvailWidth = 1 << 24, // Span all avail width even if we declared less for layout purpose. FIXME: We may be able to remove this (added in 6251d379, 2bcafc86 for menus)
ImGuiSelectableFlags_DrawHoveredWhenHeld = 1 << 25, // Always show active when held, even is not hovered. This concept could probably be renamed/formalized somehow.
ImGuiSelectableFlags_SetNavIdOnHover = 1 << 26, // Set Nav/Focus ID on mouse hover (used by MenuItem)
ImGuiSelectableFlags_NoPadWithHalfSpacing = 1 << 27 // Disable padding each side with ItemSpacing * 0.5f
};
|
This is solved with:
Solved inn 6ef5df6 : added imgui.set_io_ini_filename() & imgui.set_io_log_filename()
In f6ae207, I added named constructors when there is no user defined constructor. This adds lots of named constructors. However, when there is a user defined constructor such as for ImFontConfig, it is much too dangerous to provide a named constructor. For example, let's look and ImFontConfig default constructor: ImFontConfig::ImFontConfig()
{
memset(this, 0, sizeof(*this));
FontDataOwnedByAtlas = true;
OversampleH = 3; // FIXME: 2 may be a better default?
OversampleV = 1;
GlyphMaxAdvanceX = FLT_MAX;
RasterizerMultiply = 1.0f;
EllipsisChar = (ImWchar)-1;
} ==> If we create a named constructor that use default value for all params types, we end-up overwriting the carefully
From these remarks I suppose you want to be able to load custom font, but you actually don't care about IsBuilt, Support for loading custom fonts was added: see fa91994 and 23871b1 |
thanks very much for getting all these things done, i can't wait to test
them out! I skimmed over the diffs and left one comment, hope you got
notified of that. I hope to find a bit of time this weekend to take it all
for a spin. Would you be willing and able to make a new release, say, on
friday as that would make it greatly easier for me to test it all out if i
can just pip install form pypi?
Thanks again!
… Message ID: ***@***.***>
|
Hi, I am fighting with the CI at the time (wheels build fail in the CI). So, to test the version you will have to checkout the dev verison and pip install from sources. If you want support for immvision, you need to install and setup conan |
|
Could you try adding this to cmake/add_hello_imgui.cmake: # 2. Build glfw
if (IMGUI_BUNDLE_BUILD_PYTHON)
# Build glfw as a *shared* library:
# this is required if we want to be able to use python bindings
# for glfw, using https://github.com/FlorianRhiem/pyGLFW
# Note: the rpath is set by a call to
# lg_target_set_rpath(${python_native_module_name} ".")
# (inside add_imgui_bundle_bindings)
set(BUILD_SHARED_LIBS ON)
set(GLFW_BUILD_EXAMPLES OFF) # Add these lines
set(GLFW_BUILD_TESTS OFF)
set(GLFW_BUILD_DOCS OFF)
set(GLFW_INSTALL OFF)
add_subdirectory(external/glfw)
And tell me if it helps? |
CMaking and building with visual studio does work fine, only observations:
|
Thanks!
Argh... I hope this is not in the library code. If so, I might disable the sequencer for a while.
This one should be easy. I just need to find a way to change the implot's plot size
I had a very similar same shader error when using imgui_tex_inspect on my Mac. I thought this was a mac only issue. Did you try demo_guizmo (the one with the 3D gizmo manipulator). Does it work? Please keep me informed about the pip building process :-) |
Well it does work and is nice to have (i see a popup indicating pixel color under my mouse), so despite shader error keep it? By the way, this is not in the pip-installed imgui_bundle_demo, right?
Yes, works fine. Takes a little (tiny) bit of getting used to, but that's imguizmo's UI itself. Hopefully today i can try continuing my port, we should be very close if not all the way there now :) |
Ah, i just see that something while running the imgui_bundle_demo also put |
I ran into the next problem when porting my app. I manually modify the default style by modifying the values in the ImGuiStyle::Colors ( |
And what would make the library a bit nicer to use would be some implicit conversions. For instance, i had |
I wanted to test |
Same for KeysDown (should be |
Another thing i ran into, might be user error. I have an image i want to draw. It is uploaded through OpenGL and i have a texture id. I then call
Removing
|
I think |
Ok, last comment for now. Nice work man on the wrapper, i got my first windows to show even though not everything works yet. Anyway, what i run in now is that there are two versions of ´imgui.selectable´ (and i guess there are more like this one) where one takes a bool and returns a bool, and the other takes a pointer to bool and returns a tuple of two bools. Here i made it error so you can see the signatures:
I want to use the latter, but get the former. Even though both are defined in the generated pybind code. I guess that is because their python function syntax is identical? Both just take a bool. Perhaps this is user error. How do i call the latter one with signature returning By the way, it would be nice to provide |
Well, some more:
calling it with e.g. 1000 as argument works fine. And in my port i don't get font loading to work. That is, my icons show up as question marks. I have tried adding
after the calls to |
We had a closely related discussion about this before, IIRC. The two functions are redundant for the python binding, so we have to choose one style. for pyimgui compatibility (and because i know of no other good reason) and additional flexibility (this form is more informative) i vote to only expose version 2. I'm not sure if this case can be automatically detected (that two different c++ function lead to a python function with the same arguments) and the choice automated. By the way, I'll go through all this and make separate issues of each thing, so its possible to track them. Right now its become a mess thanks to me. |
I think i have now poored all this into separate issues. Hope i didn't miss anything. Thanks again for the hard work, i'm excited where this is heading! |
Thanks for taking the time to separate this into issues, this is helpful! I will answer in those |
Thanks for looking at all the requests and your offers of help. I hope to
look at them later today! Else probably friday or latest the weekend.
… Message ID: ***@***.***>
|
I get a whole bunch of compile errors right now, despite a completely clean
check out of dev. some excerpts of what `pip install .` gives me (rather
baffling):
```
FAILED:
CMakeFiles/imgui_color_text_edit.dir/external/ImGuiColorTextEdit/TextEditor.cpp.obj
C:\PROGRA~1\MIB055~1\2022\COMMUN~1\VC\Tools\MSVC\1434~1.319\bin\Hostx86\x64\cl.exe
/nologo /TP -DIMGUI_BUNDLE_BUILD_PYTHON -DIMGUI_BUNDLE_PYTHON_API
-DIMGUI_USER_CONFIG=\"C:/Users/huml-dkn/Desktop/imgui_bundle/cmake/imgui_bundle_config.h\"
-DImTextureID=int
-IC:\Users\huml-dkn\Desktop\imgui_bundle\external\ImGuiColorTextEdit\..
-IC:\Users\huml-dkn\Desktop\imgui_bundle\cmake\..\external
-IC:\Users\huml-dkn\Desktop\imgui_bundle\external\imgui /DWIN32 /D_WINDOWS
/GR /EHsc /O2 /Ob2 /DNDEBUG -MD -std:c++17 /showIncludes
/FoCMakeFiles\imgui_color_text_edit.dir\external\ImGuiColorTextEdit\TextEditor.cpp.obj
/FdCMakeFiles\imgui_color_text_edit.dir\imgui_color_text_edit.pdb /FS -c
C:\Users\huml-dkn\Desktop\imgui_bundle\external\ImGuiColorTextEdit\TextEditor.cpp
C:\Users\huml-dkn\Desktop\imgui_bundle\external\ImGuiColorTextEdit\TextEditor.h(75):
error C3861: 'assert': identifier not found
C:\Users\huml-dkn\Desktop\imgui_bundle\external\ImGuiColorTextEdit\TextEditor.cpp(228):
error C3861: 'assert': identifier not found
```
```
FAILED: CMakeFiles/imgui_bundle.dir/src/imgui_bundle/imgui_bundle.cpp.obj
[...]
C:\Users\huml-dkn\Desktop\imgui_bundle\external\imgui\imgui.h(3067):
warning C4297: 'ImGuiViewport::~ImGuiViewport': function assumed not to
throw an exception but does
C:\Users\huml-dkn\Desktop\imgui_bundle\external\imgui\imgui.h(3067): note:
destructor or deallocator has a (possibly implicit) non-throwing exception
specification
C:\Users\huml-dkn\Desktop\imgui_bundle\src\imgui_bundle\imgui_bundle.cpp(28):
error C2440: '=': cannot convert from 'int' to 'void *'
C:\Users\huml-dkn\Desktop\imgui_bundle\src\imgui_bundle\imgui_bundle.cpp(28):
note: Conversion from integral type to pointer type requires
reinterpret_cast, C-style cast or parenthesized function-style cast
```
```
FAILED: CMakeFiles/imgui_md.dir/external/imgui_md/imgui_md/imgui_md.cpp.obj
C:\Users\huml-dkn\Desktop\imgui_bundle\external\imgui_md\imgui_md\imgui_md.cpp(699):
error C3861: 'assert': identifier not found
```
Will try again later.
…On Wed, Dec 7, 2022 at 9:01 AM Diederick C. Niehorster ***@***.***> wrote:
Thanks for looking at all the requests and your offers of help. I hope to
look at them later today! Else probably friday or latest the weekend.
> Message ID: ***@***.***>
>
|
It should be solved now. I don't know why suddenly this include became necessary, but it is cleaner with it. |
Thanks, installed fine now :)
… Message ID: ***@***.***>
|
If you want to test immvision, install like this:
|
I will do so later, don't need it for glassesValidator but for another
(C++) project. That one will be more involved to port to hello_imgui
with all bells and whistles, but also very worth it!
Man, I am tantalizingly close now to making it all work! Mostly fixing
small things now where pyimgui deviated from the imgui api, and i have to
now change names of arguments or so. Those are great kinds of problems to
have :)
… Message ID: ***@***.***>
|
I will transfer this issue to discussions. This will be a good way for to exchange on the subject. |
This issue was moved to a discussion.
You can continue the conversation there. Go to discussion →
This is using pip-installed v0.6.5
I decided to port a project from pyimgui to imgui_bundle as I would like to post it to pypi and I can’t do that with direct dependencies (like pyimgui version 2.0).
So, some observations from porting this program (not yet available online).
First it tried to simple replace
import imgui
withfrom pyimgui to imgui_bundle.imgui
. Obviously a bunch of code adjustments are needed, mostly for the different flags (e.g.imgui.TABLE_SCROLL_X
->imgui.ImGuiTableFlags_.scroll_x
). These all get a bit long, even if they are very tight to the original imgui header. That said, this naming is not ideal I think and we have some freedom here since in C++ you’d simply directly useImGuiTableFlags_ScrollX
.imgui.TableFlags.scroll_x
would be nice, orimgui.table_flags.scroll_x
to be more pythonic and have consistent change from camel to snake case. So that’s no double imgui, and no trailing underscore.Relatedly, autocomplete works very nicely, but is a little confusing as you have each enum twice (e.g.
imgui.ImGuiTableFlags
andimgui.ImGuiTableFlags_
. The former is just an int if I understand correctly. Shall we just have the latter (ideally renamed like above)?My biggest problem: how do I use imgui by itself? Or is that not the idea? I have no equivalent of
from imgui.integrations.glfw import GlfwRenderer
in imgui_bundle. Is the idea that you must use the hello_imgui wrapper? That makes it much less of a drop-in replacement. And I think I cannot use that, because I use glfw callbacks for my app, but can’t access those, I think? I have set up:And other interaction with glfw (such as window scaling when moving between dpi monitors).
Can I set these up? How?
Does imgui_bundle also expose its opengl binding? Can I port
import OpenGL.GL as gl
to something in the imgui_bundle, or is that out of scope? I’m asking as my code for instance does:Some small observations:
imgui_internal.ImGuiSelectableFlagsPrivate_.im_gui_selectable_flags_select_on_click
is a bit over the top, some conversion error?FLT_MIN
(pyimgui:imgui.FLOAT_MIN
) is unavailable. Is ok, can be replaced withnp.finfo('single').tiny
(untested), but seems a bit heavy to require numpy for just that. That pyimgui provides imgui.FLOAT_MIN is a good solution I think. Users may well get this wrong otherwise, since a python float is a double.The text was updated successfully, but these errors were encountered: