Get cimgui_internal.h to a compileable state

[ZimM-LostPolygon]

This gets the generator to the point where imgui_internal.h can be successfully parsed and the corresponding C bindings are generated and are able to be compiled.

This is highly experimental - the only benchmark so far is that example_null.c is able to compile and use a function from imgui_internal.h.

Some notes:

• I'm not sure what the original idea was, but the way I've made it work, is first generate cimgui.h, then generate cimgui_internal.h while referencing the imgui.h using --config-include.
• With that in mind, --config-include was made appendable, meaning it can be specified multiple times.

Few things that might need to be improved/fixed:

• ImGuiContext is special-treated as an opaque pointer, with all the struct innards removed. This is because it includes a field of type ImGuiInputTextState, which includes a field of type ImStb::STB_TexteditState, and I'm not sure how to really handle that at the moment. Do we also parse imstb_textedit.h? cimgui seems to be doing that, at least.
• The resulting JSON output basically combines stuff from cimgui.h and cimgui_internal.h. For me personally, that's nice, but perhaps there's a use-case for internal-only metadata?
• Should literally everything under cimgui_internal.h be marked is_internal in JSON metadata? Or perhaps the source file is enough to infer that a thing is coming from imgui_internal.h?
• Backend headers generation was not tested yet, at all.

At this point, I'll try to use this branch with the new C# wrapper I'm developing, and as I'm bound to find bugs, there will probably be some fixes.

[ocornut]

Really great to make progress on that!
I’ll let Ben see how it aligns with his intended design, but some real quick thoughts:

, which includes a field of type ImStb::STB_TexteditState, and I'm not sure how to really handle that at the moment

Would there is a way to get the sizeof() of that structure and temporarily replace it by an opaque buffer, so the outer struct can work?

the resulting JSON output basically combines stuff from cimgui.h and cimgui_internal.h. For me personally, that's nice, but perhaps there's a use-case for internal-only metadata?
Should literally everything under cimgui_internal.h be marked is_internal in JSON metadata? Or perhaps the source file is enough to infer that a thing is coming from imgui_internal.h?

My thoughts was that ideally we should generate cimgui.h, cimgui.json and cimgui.json, cimgui_internal.json as separate files. At minimum the .h file should be separate.

[ZimM-LostPolygon]

Just a progress update - with just some minor hacks to Dear ImGui, my in-development C# bindings generator currently generates bindings for the base Dear ImGui, internals, as well as SDL2 and Win32 backends, all nicely separated unlike ImGui.NET 😛 Still quite a lot of work to do, but it's coming together, all thanks to Dear Bindings.
I can't present it yet, but I believe this validates the PR well.

What is the work required for this PR? At least for the initial internals support version?

[ShironekoBen]

Sorry about the delay!

That's awesome work - thank you so much!

I've done some testing with this now - the code all looks very sensible and I'm not seeing any regressions so on that front I think everything is fine. With a bodge in place for stb_textedit I was able to call some imgui_internal.h functions, so I think that is working too.

There are just three points that I think may need tweaking:

1. What was the reasoning behind removing storage of the initialiser/"default value" (as the existing code somewhat erroneously refers to it) from fielddeclaration.py?
   As far as I can tell it's harmless (well, the name is slightly confusing), and whilst I don't think there's anything in imgui currently that uses it the code does exist to export that data to the JSON file for anyone who is interested.
   I'm not against removing support if it's unused and causing a problem, but my inclination is to either keep it in a moderately functional state or remove it completely (i.e. remove default_value_tokens from DOMFieldDeclaration and the associated code that writes that to the JSON) to avoid confusion.

2. The example code doesn't compile out-of-the-box due to CIMGUI_WITH_GENERATED_DEFAULT_ARG_FUNCTIONS not being set (the github action sets that but if you just run the converter manually you won't get it), and ImGui_DebugLogUnformatted() not existing (not sure where that comes from - am I missing something obvious?).
   It seems like it would make sense with CIMGUI_WITH_GENERATED_DEFAULT_ARG_FUNCTIONS to either add the define in the code itself so that it is automatically included, or switch it to be CIMGUI_WITHOUT_GENERATED_DEFAULT_ARG_FUNCTIONS (so that the default case is the same as what you get if you just run the converter with no special arguments). Does that sound sensible?

3. We need to figure out what to do about ImStb - I was able to work around the problem by using your minimal header and doing the following:

a) Create templates so that imtextedit_minimal_header can be processed with Dear Bindings like follows:

python dear_bindings.py --imconfig-path ..\imgui\imconfig.h -o generated\cimstb_textedit src\external\imstb_textedit_minimal.h

b) Add a remap to change the included STB header to the cimstb_textedit.h generated in step a.

c) Add a remap to rewrite ImStb::STB_TexteditState as STB_TexteditState.

That then produced a version that I could compile and run without needing changes in ImGui itself or manual edits to the file.
You can see the full set of edits I did here: b9ff819

That isn't an ideal solution but it feels like it may well be good enough for the time being?
Or do you have any other workaround ideas?

From my perspective if we can get those three sorted out then I think we're good to merge this - at a minimum, if we can resolve 1 and 2 in this PR then I think we can pull this in as-is (as 3 only affects imgui_internal.h and thus won't cause problems for anyone currently using the library). Alternatively I'm happy to pull it into a branch here and do the tweaks before merging into main. (I have to confess that I don't use Github PRs much so if there's a smarter way to "stage" a PR so edits from multiple people can be incorporated prior to merging then please let me know!)

[ocornut]

I haven’t looked into it but would accept any PR in main lib that solves your textedit problem.
(Might i suggest if you can inline that extra file to have an extra file for the end user?)

(Since presently we support a single active InputTextState only maybe I should make that structure use a heap allocation as an indirection…)

[ZimM-LostPolygon]

What was the reasoning behind removing storage of the initialiser/"default value" (as the existing code somewhat erroneously refers to it) from fielddeclaration.py?

Huh. I don't remember deliberately doing that. Looking at it, it's probably a result of a bad merge conflict resolution on my side (initializer storage was added after I did the initial version of this PR last year). I'll bring it back.

ImGui_DebugLogUnformatted() not existing (not sure where that comes from - am I missing something obvious?).

That actually comes from this PR #46 - the github action adds the --generateunformattedfunctions argument as well. I'll take care of this.

We need to figure out what to do about ImStb - I was able to work around the problem by using your minimal header and doing the following

I'm a bit concerned about how generating the internals becomes a multi-step process. It's still better since it works around having to change the main library, but perhaps we can document it better? Or maybe even automate that cimstb_textedit.h generation step if it's not found already (which would be both easier to use and save us from adding that extra step to github actions)?

I'm not aware of any clever way to allow multiple people to work on a PR. So I've just gave you access to my fork on GitHub, feel free to do any edits there!

@ocornut I've poked around at the main library, but I couldn't come up with a universal way to expose ImStb::STB_TexteditState without either a lot of code changes, or without making things ugly, or without introducing yet another imconfig.h option just for this. Heap-allocating ImGuiInputTextState is indeed a viable option, but I admit, I don't think I can do that, my cpp-fu is just good enough for reading...

[ocornut]

STB_TexteditState layout is extremely stable so I believe hard-coding something in the form of an embedded template would work. My only recommendation is that the template/include is inlined in cimgui_internal.h.
(Additionally: my expectation is that I will ditch this library next year. So any cruft/workaround built today might eventually be cleaned up.)

I'll leave it up to you and maybe Ben is in a better position to tell if a change in core lib can help you.

[ocornut]

I have pushed those changes.
ocornut/imgui@21d03ed
ocornut/imgui@ca5701d

This add an indirection, renames the type and move helper functions to the .cpp files.

The only thing left is the block that includes imstb_textedit.h which I think you can skip and replace with a forward declaration.

(I am trying to move that block to the .cpp file now, but struggling with C/C++ forward of typedefs)

[ocornut]

Moved the include to .cpp file:
ocornut/imgui@15cb7d6

I believe the situation should be easy to solve on your end.

[ZimM-LostPolygon]

Thank you! This simplified things a lot. I now simply replace ImGuiInputTextState.Stb type with a void*, and everything works without any extra headers or additional main lib modifications.

Feels pretty robust now overall, to me at least :)

[ShironekoBen]

Great - that does indeed seem to solve everything in a nice and simple manner.
Thanks!
@ZimM-LostPolygon, if you're all happy with this I'll go ahead and merge it, and then update the readme/etc as a commit on top. Just to double-check are you OK with being credited for this with your Github username?

[ZimM-LostPolygon]

Feel free to merge it, yes, it has been a long time coming :)

Just to double-check are you OK with being credited for this with your Github username?

Sure thing!

[ocornut]

It may be interesting to do a sanity check of comparing cimgui.h/cpp/metadata before/after this change?
I'll look into running it now to have a look a the cimgui_internal.h output.
I am very excited about this :) Thanks for your amazing work!

[ocornut]

It may be interesting to do a sanity check of comparing cimgui.h/cpp/metadata before/after this change?

I've done that and they are identical (apart from a very minor thing in cimgui.json which is correct now).

I only skimmed through it, but output quality is top-notch.

Ben: tangential: but I hadn't run dear-bindings in a long time, and using/installing "ply" is not immediately obvious to me. Maybe doc could be amended? (moved to #72)

[ocornut]

Additional feedback, sorry for spam:

(1) in README:

python dear_bindings.py -o cimgui ../imgui/imgui.h

Amend with:

python dear_bindings.py -o cimgui_internal --include ../imgui/imgui.h ../imgui/imgui_internal.h   // if you want internals

To provide an explicit example.

(2) Amend BuildAllBindings.bat accordingly.

(3) After switching to this branch I got an error with my install:

Python 3.8.10 (tags/v3.8.10:3d8993a, May  3 2021, 11:48:03) [MSC v.1928 64 bit (AMD64)] on win32

Traceback (most recent call last):
  File "dear_bindings.py", line 25, in <module>
    from src.modifiers import *
  File "C:\Omar\Work\dear_bindings\src\modifiers\__init__.py", line 13, in <module>
    from . import mod_flatten_templates
  File "C:\Omar\Work\dear_bindings\src\modifiers\mod_flatten_templates.py", line 43, in <module>
    def apply_single_iteration(dom_root, custom_type_fudges) -> list[code_dom.DOMTemplate]:
TypeError: 'type' object is not subscriptable

After updating Python it worked:

Python 3.11.8 (tags/v3.11.8:db85d51, Feb  6 2024, 22:03:32) [MSC v.1937 64 bit (AMD64)] on win32

Note that updating Python required reinstalling ply for me, which might be an additional vote to bundle ply in repo.

[ZimM-LostPolygon]

After updating Python it worked:

The minimum required Python version for the current codebase is 3.10 (which is what we use in GitHub Actions), let's note that in the readme

(2) Amend BuildAllBindings.bat accordingly.

Not directly related, but - shall we rewrite BuildAllBindings.bat with PowerShell? Simply because PowerShell is the only truly cross-platform shell. That way, Linux/macOS people would be able to use BuildAllBindings as well. (alternatively, we can create a separate BuildAllBindings.sh, but then it's two files to maintain)

[ZimM-LostPolygon]

Next stop - ImPlot 😛

[ShironekoBen]

Cool, I've merged that and updated the documentation to match. Shout if you spot any problems!

With regards to rewriting BuildAllBindingsbat, I have to confess to not being hugely familiar with PowerShell, but my concern would be that whilst it's cross-platform it's not actually a default install (as far as I'm aware) on OSX or most Linux distros... so it would become another dependency for people looking to get things working. A simple .sh file may be easier all-round, given that it's not a particularly complicated script. But I'm more than open to suggestions!

[ShironekoBen]

(ImPlot would be cool indeed! ^-^)

[ShironekoBen]

And thanks again for all your hard work on this!

[ocornut]

Thanks Serhii ! and Ben for taking the time to follow on it.