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

Add option for handling internals #364

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Conversation

Thraka
Copy link
Contributor

@Thraka Thraka commented Nov 23, 2022

Hi @mellinoe I know you're crazy busy with life and other projects, but I wanted to throw this out there.

(Separate ping to @JulianVallee thank you for your other PR. I used it as inspiration for this PR (and I took a few things here or there), but I reduced the scope to improve the likelihood that it's merged or at least looked at, since it's a bit simpler.)

This PR is similar to #273 except it contains just what is necessary to get something working. It's not trying to actually update the release build in any way. I've tested omitting the --internal command line parameter and it outputs exactly the same content (according to git) as your current master branch.

This PR does the following:

  • Adds a better command line parser. This is fluff and doesn't change the operation of the generator, but it does make it easier to understand how to use it as it provides output descriptions.
  • Supports pulling in the internal items defined by imgui.

The way this works is

  1. Sets up three parameters that can be set on the command line. Here is the --help info:

    Description:
      Generates code for the ImGui.NET libraries based on the cimgui definition files.
    
    Usage:
      CodeGenerator [options]
    
    Options:
      --internal                                         When set to true, includes the internal header file.
      -o, --outputDir <outputDir>                        The directory to place generated code files. [default: current directory]
      -l, --library <cimgui|cimguizmo|cimnodes|cimplot>  The library to read parse. [default: cimgui]
      --version                                          Show version information
      -?, -h, --help                                     Show help and usage information
    
  2. The variables used by Program.cs remain but are initialized by the command line parser for default values: outputPath and libraryName.

  3. A new variable is introduced, useInternals, which is set to true when the --internal parameter is set.

  4. ImGuiDefinitions.LoadFrom now has a parameter to indicate that internal members should be loaded.

  5. When Enums, Types, and Functions are loaded, they're checked for the internal flag in the definition file and set on the corresponding class definition types.

  6. When processing all of the loaded definitions, types and enums that are internal are added to the ImGuiNET.Internal namespace.

  7. When processing the main ImGui type, it's done twice, first excluding any internal member, then a second time if the internal flag was set, adding these to the ImGuiNET.Internal.ImGui class.

Processing the file in this way keeps the public facing API ImGuiNET.ImGui clean and free of any internal members. I think this is a good compromise so people can understand what they're doing and what they're accessing.

When the code generator runs with the internal mode on, it doesn't generate a compilable project. There are all sorts of little things to handle, some done by the other PR I linked to #273. I'm not sure how you want to further enhance the code generator to handle this. I also don't know a ton about C++ and I'm unsure what to do to handle a lot of things. I don't know how you would want to handle the two types that have union members for example.

{
string outputPath;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm unsure why github has decided to diff this in this way, but this is all new code apart from the declarations of the outputPath and libraryName variables which was moved up from below. You can see this code in raw format in this comment. This does a few things:

  1. Creates three parameters
  2. Adds them to the command line parser
  3. Runs the parser
  4. If the parser passes, it moves on to the normal logic you had in this file which now starts on line 89

https://github.com/mellinoe/ImGui.NET/blob/70a7b35459b260377b7487981b65839a02e3a8c6/src/CodeGenerator/Program.cs#L21-L87

Comment on lines 363 to 425
foreach (FunctionDefinition fd in defs.Functions)
EmitFunctions(false);
writer.PopBlock();
writer.PopBlock();

if (useInternals)
{
if (TypeInfo.SkippedFunctions.Contains(fd.Name)) { continue; }
writer.PushBlock($"namespace {projectNamespace}{InternalNamespace}");
writer.PushBlock($"public static unsafe partial class {classPrefix}");
EmitFunctions(true);
writer.PopBlock();
writer.PopBlock();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This runs the function overload processing logic twice, once for non-internals, to put things in the ImGuiNET.ImGui class, and then a second time for internals which go into the ImGuiNET.Internal.ImGui class.


foreach (OverloadDefinition overload in fd.Overloads)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code was promoted to a local function so it could run twice, with the internal flag set or not. The code is the same except for line 433. Originally this method does a ton of processing on the member data only to do a final check, if it isn't a member function (and thus belongs on the ImGui class) then returns. I'm unsure why that was done at the end of all the logic, so I moved it here to just filter them out at the start. Now this loop ONLY processes non-member functions.


for (int i = overload.DefaultValues.Count; i >= 0; i--)
{
if (overload.IsMemberFunction) { continue; }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the line I removed and added to the main loop to filter out member functions.

Comment on lines 58 to 68
// internals
{ "char[5]", "byte*"},
{ "ImGuiDir*", "IntPtr" },
//{ "ImGuiStoragePair", "IntPtr" },
{ "ImGuiDockRequest", "IntPtr" },
{ "ImGuiDockNodeSettings", "IntPtr" },
{ "ImGuiTableColumnIdx", "sbyte" },
{ "ImGuiTableDrawChannelIdx", "byte"},
{ "ImGuiContextHookCallback", "IntPtr" },
{ "ImGuiErrorLogCallback", "IntPtr" },
//{ "ImGuiSizeCallback", "IntPtr"}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was taken from #273 and I'm not totally sure how it all works or if it's correct.

Copy link

@JulianVallee JulianVallee Nov 24, 2022

Choose a reason for hiding this comment

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

I only remember vaguely, but I think I found those by manual "trial and error". Added them here as they were causing erors in the generated internals C# code.

I found these values by looking at the existing TypeInfos, was pretty straight forward to define the mappings:

  • Structs and pointers were mapped to IntPtr
  • IDs were mapped to byte/sbyte (might work if both are sbyte, don't remember if the unsigned byte was intentional)
  • Char array was mapped to byte pointer, as chars are represented as bytes and arrays need to be pointers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know what should be added to these sections as the PR is 2 years old and I don't know what new types need to be defined.

@JulianVallee
Copy link

JulianVallee commented Nov 24, 2022

Hi @Thraka, glad that you could use something from the PR, that's what I uploaded it for after all! Didn't have much time to follow up unfortunately, switched jobs and all that jazz

@Thraka
Copy link
Contributor Author

Thraka commented Nov 25, 2022

One thing that I don't understand is when generating internals, it creates ImRect with a vector min/max field. It seems these are reversed when accessed on something like the Window object. The Max is 0,0 and the Min is a number,number I have no idea why this would happen.

Even so, I generated the internals and copied over a bunch of the source files to my project just so I could access GetCurrentWindow and then I just fixed ImRect directly by reversing the fields (I hate that). I then tested it by porting a GroupBox widget from the ImGui forums:

ocornut/imgui#1496 (comment)

image

@copygirl
Copy link

copygirl commented Dec 17, 2022

I've also been looking into getting access to ImGui's internals, which are often being advertised as solutions or at least workarounds to questions people have in ImGui's issue tracker. In my case, docking, clearing an element's focus, and modifying the selection of a text input are all not possible with the current C# bindings, since the necessary functions are not exposed.

After much messing around, I was able to get a modified version of Thraka's branch working. Official support would be of course very much appreciated. However I wouldn't have made a post to just say this.

In this PR, it appears that internal functions are placed in ImGuiNET.Internal.ImGui, which ends up colliding with the regular ImGui class. Would it be feasible to place the functions in ImGuiInternal or a subclass ImGui.Internal, instead?

@Thraka
Copy link
Contributor Author

Thraka commented Dec 18, 2022

I put everything into a separate namespace to mimic what ImGui does, the internals are only ever pulled in when you reference the internals header. I thought this would be a decent enough separation where you can easily see what is internal and what is non-internal.

I do like the ImGui.Internal class idea.

What I've been doing is just importing the class directly as ImGuiInternal

using ImGuiInternal = ImGuiNET.Internal.ImGui;

@deccer
Copy link

deccer commented Feb 11, 2024

@zaafar is there a chance that this can be shipped? Idk what your role is in this project, but you seem to be merging things when eric is not around? :)

@zaafar
Copy link
Collaborator

zaafar commented Feb 11, 2024

@deccer i forgot about this PR, give me few days I will get it merged.

@Thraka
Copy link
Contributor Author

Thraka commented Feb 12, 2024

@zaafar if you're actually going to spend time on this PR, I'll schedule some time to pull down the latest locally and make sure everything still works. I've just been using the version I compiled this for, so I've not had a need to revisit it. Let me know.

I fixed the merge conflict at least.

@Thraka
Copy link
Contributor Author

Thraka commented Mar 20, 2024

This update is proving to be a lot more difficult and past my ability.... I don't know what to do with these very complex internal types that there's no code generation definitions for. Some of these types evolved from the previous 1.88 version which the code generator could handle with internals.


Some examples:
https://github.com/ocornut/imgui/blob/v1.90.1/imgui_internal.h#L788-L794

struct ImDrawDataBuilder
{
    ImVector<ImDrawList*>*  Layers[2];      // Pointers to global layers for: regular, tooltip. LayersP[0] is owned by DrawData.
    ImVector<ImDrawList*>   LayerData1;

    ImDrawDataBuilder()                     { memset(this, 0, sizeof(*this)); }
};

In the generated ImDrawDataBuilderPtr type, you get this invalid code (can't use a ImVector* in the RangeAccessor:

        public RangeAccessor<ImVector*> Layers => new RangeAccessor<ImVector*>(&NativePtr->Layers_0, 2);

Also, the ImGui CPP code is using unions in some structures such as
https://github.com/ocornut/imgui/blob/v1.90.1/imgui_internal.h#L1033C21-L1040

struct ImGuiStyleMod
{
    ImGuiStyleVar   VarIdx;
    union           { int BackupInt[2]; float BackupFloat[2]; };
    ImGuiStyleMod(ImGuiStyleVar idx, int v)     { VarIdx = idx; BackupInt[0] = v; }
    ImGuiStyleMod(ImGuiStyleVar idx, float v)   { VarIdx = idx; BackupFloat[0] = v; }
    ImGuiStyleMod(ImGuiStyleVar idx, ImVec2 v)  { VarIdx = idx; BackupFloat[0] = v.x; BackupFloat[1] = v.y; }
};

And the current code generation system doesn't know what to do with that.


There are also these helpers from the ImGui code that don't get translated. I'm not sure what to do with these. For example, take ImSpan and ImBitArray.

The structs_and_enums.json file has these under templated_structs and templated_done which isn't processed by the code generator at all.

Generated code:

    public unsafe partial struct ImGuiTable
    {
        ...
        public ImSpan_ImGuiTableColumn Columns;
        public ImSpan_ImGuiTableColumnIdx DisplayOrderToIndex;
        public ImSpan_ImGuiTableCellData RowCellData;
        public ImBitArrayPtr EnabledMaskByDisplayOrder;
        public ImBitArrayPtr EnabledMaskByIndex;
        public ImBitArrayPtr VisibleMaskByIndex;
        ...

@Thraka
Copy link
Contributor Author

Thraka commented Mar 22, 2024

@zaafar Please read my previous post, I've updated it if you already read it.

As this PR stands now, it has code changes to restructure the command line system of the code generator, and handles internals when --internals is passed. Normal code generation still works and generates code that exactly matches the current master branch. If you accept this PR, you can still generate the exact code that exists. I did test generating non-internal code for all 4 cim* libraries and everything worked.

Handling the actual code generation for internals, however, doesn't work anymore. It worked with 1.88 but with 1.90.1, there are few cases the code generation just doesn't handle, as detailed in the previous post.

The quicker this gets accepted, the better 😁 just so I don't have to keep coming back and tweaking. Right now I'm stuck on 1.88 because I use some internals that it could generate cleanly which won't generate now.

Cheers!

--- Edit, additional ---
I just realized that technically, even at 1.88, I wasn't able to generate 100% accurate internal code simply because there are a few odd types in 1.88 that generated invalid code. So I was just picking the bits and pieces I needed from the internal generated code into my own library. For the latest 1.9x whatever version, the internal types I was using have changed and now are generated inaccurately, so I can't upgrade because of that.

@AhmedZero
Copy link

any news?

@zaafar
Copy link
Collaborator

zaafar commented Apr 10, 2024

sorry about that, I got busy in job.
I do not have any ETA on this one.

i want to merge this one but it can only happen once I (or someone with push access) can review it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants