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

Integration of ImPlot, ImNodes and ImGuizmo #218

Merged
merged 12 commits into from Mar 21, 2021

Conversation

TillAlex
Copy link
Contributor

@TillAlex TillAlex commented Nov 28, 2020

This PR adds support for ImPlot(#191), ImNodes(#201) and ImGuizmo(#200).

What has been done:

  • Added ImPlot.NET, ImNodes.NET and ImGuizmo.NET projects
  • Modified CodeGenerator to generate code for cimplot, cimnodes and cimguizmo
  • Added definition files from cimplot, cimnodes and cimguizmo
  • Added ImPlot demo window to sample program

Until now I only did minimal testing. Binaries including cimplot, cimnodes and cimguizmo are missing. For testing I manually compiled a cimgui.dll including cimplot. I am not sure if you prefer one binary including all projects or separate binaries.

Please let me know what you think about this PR. Feel free to change this PR in any way that is necessary to include it. If you need me to change anything let me know.

@mellinoe
Copy link
Collaborator

This is fantastic stuff, thanks for putting this together @TillAlex. I definitely want to support this in some form, and in general most of the changes here look reasonable.

One question is how the native portions should be organized and delivered. As far as I can tell, all of these libraries are intended to be compiled and included together in a single native DLL. That's obviously possible here as well based on your testing, but it would be better if we could separate them out into 4 dynamic libraries (libcimgui, libimplot, libimnodes, and libimguizmo). This would allow users of ImGui.NET alone to avoid including extra libraries that they aren't interested in.

The new libraries would need to be built against a particular version of libcimgui (and thus would need to be used w/ a particular version of ImGui.NET), but I can't think of any technical reason why this wouldn't work. Functionally, this would mean expanding ImGui.NET-nativebuild to cover the new libraries, in a way that produced 4 compatible dynamic libs from source.

Long-term, I'd like to see the extra libraries moved out of this repo into separate ones, but I don't feel very strongly about that initially. The fact that all of them will need CodeGenerator means that a larger restructuring would be needed for that to work. Along these lines, it would be nice if CodeGenerator was a little less aware of the quirks of each individual project (e.g. now it knows about things like ImPlotContext specifically). CodeGenerator is filled with its fair share of hacks, a lot of this could probably be cleaned up and fixed at the same time. It would be great if CodeGenerator could be used as a utility for any cimgui-based library without needing tons of modifications to it.

Copy link
Collaborator

@mellinoe mellinoe left a comment

Choose a reason for hiding this comment

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

The changes to CodeGenerator are relatively minor, so I don't have any major issues with them. I would like to see CodeGenerator made more generic (e.g. instead of hard-coding all of these new quirks, it would be better if they were passed in somehow), but it's a larger piece of work. To get this merged, I think we should work towards the following:

  1. Get the end-to-end working w/ the native libraries. We can set up an auto-build of some other branch of ImGui.NET-nativebuild to test this before incorporating it into the main branch there. The new C# projects should include those native libraries exactly the same way that ImGui.NET does.
  2. Trim the additions down to just ImPlot and ImGuizmo. It turns out that there are 3 similar node editor extensions (imnodes, ImNodes, and imgui-node-editor). The latter seems to have the largest usage (judging by activity), but I don't think there's a cimgui binding for it. I don't really want to be in the business of choosing which one of those to support here. Long-term, I'd like all of this to live outside of the main repository here (including any future libraries that users want to wrap) so someone else could support it if they wanted. I'm okay with including ImPlot and ImGuizmo to prove this out for now.

src/CodeGenerator/Program.cs Show resolved Hide resolved

public static readonly List<string> WellKnownEnums = new List<string>()
{
"ImGuiMouseButton"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, I guess this is needed because one of the new projects uses ImGuiMouseButton, but the definitions.json file that is used for it doesn't include it (since it's part of cimgui). This might work as a quick fix, but it points to a deeper issue. It seems like the new libraries should reference the cimgui definitions while generating code.

@@ -0,0 +1,8 @@
namespace ImGuiNET
{
public enum MODE
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was a bit conflicted above, but seeing some of these types makes me think that these new libraries should be in their own namespaces. I don't necessarily want types like MODE and OPERATION to pollute the ImGuiNET namespace for regular users of ImGui.NET even if they have to opt-in to this library.

{
"cimgui" => "ImGui",
"cimplot" => "ImPlot",
"cimnodes" => "ImNodes",
Copy link
Collaborator

Choose a reason for hiding this comment

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

After looking into this more, cimnodes actually wraps the imnodes project, which is similar but different to another project called ImNodes. Using ImNodes as the class name (instea of imnodes) might be confusing because of that.

@TillAlex
Copy link
Contributor Author

Thanks for your comments!

On 1) I think you are totally right that split native libraries are the better solution and it shouldn't be to complicated to integrate that into ImGui.NET-nativebuild. I am not a cmake pro, but I will give it a try.

On 2) I understand your point about ImNodes, but I also think it's sad not to have a nodes editor for ImGui.NET. Perhaps we should just generate wrappers for all the cimxxx projects so that the user can choose from at least two of the three nodes editors. If @sonoro1234 decides to include imgui-node-editor as well we can add that as well.

Hopefully I will find some time this evening to work on some of the points from your comments.

@cbaal83
Copy link

cbaal83 commented Nov 29, 2020

Hi Alex,
could you please provide your pre-build native libs? I would love to test this but don't want to spent to much time on building the native libs :).

Best regards,
//cbaal

@TillAlex
Copy link
Contributor Author

@cbaal83 Here are x64 binaries I just built: binaries.zip

Included are separate binaries of the current versions of cimgui, cimplot, cimnodes and cimguizmo. I changed this PR to work with these. Until now I only tested the demo window of ImPlot used in ImGui.NET.SampleProgram. It works as long as you replace the original cimgui with the one in the zip, because cimplot.dll is built against ImGui 1.79.

I am not sure if my way of building separate binaries is correct. Let me know if they work for you.

…izmo_Integration

# Conflicts:
#	src/ImGui.NET.SampleProgram/ImGui.NET.SampleProgram.csproj
@cbaal83
Copy link

cbaal83 commented Nov 30, 2020

@TillAlex
Thank you very much! Will test today! If something is buggy i will post it here.
//cbaal

@cbaal83
Copy link

cbaal83 commented Nov 30, 2020

at ImGuiNET.ImPlotNative.ImPlot_BeginPlot(Byte* title_id, Byte* x_label, Byte* y_label, Vector2 size, ImPlotFlags flags, ImPlotAxisFlags x_flags, ImPlotAxisFlags y_flags, ImPlotAxisFlags y2_flags, ImPlotAxisFlags y3_flags)
at ImGuiNET.ImPlot.BeginPlot(String title_id, String x_label, String y_label) in S:\Dev\github\ImGui.NET\src\ImPlot.NET\Generated\ImPlot.gen.cs:line 377
at ImGuiNET.SampleProgram.XNA.SampleGame.ImGuiLayout() in S:\Dev\github\ImGui.NET\src\ImGui.NET.SampleProgram.XNA\SampleGame.cs:line 84
at ImGuiNET.SampleProgram.XNA.SampleGame.Draw(GameTime gameTime) in S:\Dev\github\ImGui.NET\src\ImGui.NET.SampleProgram.XNA\SampleGame.cs:line 63
at Microsoft.Xna.Framework.Game.DoDraw(GameTime gameTime)
at Microsoft.Xna.Framework.Game.Tick()
at Microsoft.Xna.Framework.SdlGamePlatform.RunLoop()
at Microsoft.Xna.Framework.Game.Run(GameRunBehavior runBehavior)
at ImGuiNET.SampleProgram.XNA.Program.Main(String[] args) in S:\Dev\github\ImGui.NET\src\ImGui.NET.SampleProgram.XNA\Program.cs:line 7

I have just added thoose 2 lines to the XNA SampleGame on line 84

ImPlot.BeginPlot("Testplot", "x", "y");
ImPlot.EndPlot();

I've also tested Implot.CreateContext() and implot.SetImGuiContext() (inside ImguiRenderer ctor), doesn't work.

Do i miss something obvious here?

//cbaal83

@TillAlex
Copy link
Contributor Author

Just tested the same and it works. Found two potential problems you could be running into:

  1. I had to add the following to ImGuiRenderer after line 46:
    IntPtr implotContext = ImPlot.CreateContext(); ImPlot.SetCurrentContext(implotContext); ImPlot.SetImGuiContext(context);

  2. Be sure to use the cimgui.dll I provided. It tends to be replaced every time you build.

@cbaal83
Copy link

cbaal83 commented Nov 30, 2020

Yep, i haven't done all three together. It's working now :).
Will test now! Thanks!
//cbaal

@cbaal83
Copy link

cbaal83 commented Nov 30, 2020

Ok, quick question:
How iam supposed to pass in the values? All of them accepting a "ref float" value, which should be an array.
I tried passing a pointer to it, still doesnt work like so:

var min = typed.Min(x => x.Price);
var max = typed.Max(x => x.Price);
var avg = typed.Average(x => x.Price);
float[] value = new float[] { min, max, (float)avg };
unsafe
{
     fixed (float* ptr = value)
     {
            ImPlot.PlotBars("values", ref *ptr, 3);
      }
}

@cbaal83
Copy link

cbaal83 commented Nov 30, 2020

Another issue:
Even if i pass in a single value like so:
ImPlot.PlotBars("min", ref min, 1);
Value of that bar is always 1 (min is NOT 1 ;) ).

I have the feel iam doing something wrong here :D.
I've checked the original demos, but can't find anything obvious wrong..

//cbaal

@cbaal83
Copy link

cbaal83 commented Nov 30, 2020

I feel stupid now, chart wasn't zoomed out...
Sorry!

@TillAlex
Copy link
Contributor Author

I can not check at the moment if this works, but you can try

ImPlot.PlotBars("values", ref value[0], 3);

@cbaal83
Copy link

cbaal83 commented Nov 30, 2020

Both works, the safe and unsafe way.
I was missing the following piece:
ImPlot.SetNextPlotLimits(0, 3, 0, _chartMax);

Which needs to be called before any Implot.BeginPlot
So far it works perfect (some api stuff is there which i dont like, but that is not the fault of this wrapper :) )

Thanks!

@bootzin
Copy link

bootzin commented Nov 30, 2020

Do these work with the docking/viewport branch of imgui?

@cbaal83
Copy link

cbaal83 commented Dec 1, 2020

@TillAlex
Im missing some functions which are defined in implot_internal.h:

Implot.BeginItem
Implot.EndItem,
Implot.GetCurrentItem,
Implot.FitThisFrame,
Implot.FitPoint

which are necessary for custom graphs.

Any chance you add them?
//cbaal

@TillAlex
Copy link
Contributor Author

TillAlex commented Dec 1, 2020

@cbaal83 As far as I understand cimplot does not wrap the internal api of implot. @sonoro1234 Is this correct?

@sonoro1234
Copy link

sonoro1234 commented Dec 1, 2020

@cbaal83 As far as I understand cimplot does not wrap the internal api of implot. @sonoro1234 Is this correct?

Yes, only imgui_internal is wrapped.

@cbaal83
Copy link

cbaal83 commented Dec 1, 2020

@sonoro1234
Any chance implot_internal will be added?

@sonoro1234
Copy link

sonoro1234 commented Dec 1, 2020

https://github.com/cimgui/cimplot/tree/internal just added
Main problem for using it is time_t being long in 32bits and long long in 64bits (with mingw-w64)

Main problem for compiling is

CMakeFiles\cimgui_glfw.dir/objects.a(cimplot.cpp.obj):cimplot.cpp:(.text+0x48f3): undefined reference to `ImPlot::LabelT
ickTime(ImPlotTick&, ImGuiTextBuffer&, ImPlotTime const&, ImPlotDateTimeFmt)'
CMakeFiles\cimgui_glfw.dir/objects.a(cimplot.cpp.obj):cimplot.cpp:(.text+0x49db): undefined reference to `ImPlot::AddTic
ksTime(ImPlotRange const&, int, ImPlotTickCollection&)'

I dont know why (LabelTickTime is inlined but AddTickTime is not inlined)
@epezent Any idea?

in implot.cpp
void AddTicksTime(const ImPlotRange& range, float plot_width, ImPlotTickCollection& ticks)
in implot_internal
IMPLOT_API void AddTicksTime(const ImPlotRange& range, int nMajor, ImPlotTickCollection& ticks);

so different signature

for LabelTickTime the cause seems to be inline in definition

Just pushed with

local cimgui_skipped = {
	 ImBufferWriter_Write = true,
	 ImPlot_LabelTickTime = true,
	 ImPlot_AddTicksTime = true
}

So that compilation succeds

@Vivian-A
Copy link

Vivian-A commented Dec 7, 2020

I took the precompiled binaries from your post earlier, where they're seperate dlls. Should I compile them myself?

@TillAlex
Copy link
Contributor Author

TillAlex commented Dec 7, 2020

You do not have to compile them yourself, but you really have to make sure you are using the precompiled cimgui.dll and cimplot.dll. The compiler tends to replace cimgui.dll with the one from the repository during compilation.

@Vivian-A
Copy link

Vivian-A commented Dec 8, 2020

Alright, I got it working! Thank you all so much!

@TillAlex
Copy link
Contributor Author

TillAlex commented Dec 8, 2020

What was the problem? Wrong DLL?

@Vivian-A
Copy link

Vivian-A commented Dec 8, 2020

Yeah, wrong dll. I had forgotten to replace it. Took some more tweaking with the Unity ImGui package I was using after that, but I've never been so happy to see a graph in my life. Thanks a ton for this PR man, truly appreciated!

@freakbyte
Copy link

Any progress on this?
tables was just merged into imgui master branch, seems like a good time to upgrade :>
Thank you for all your work, this is awesome!

@bootzin
Copy link

bootzin commented Feb 24, 2021

Hey guys
Just an update on this: Nelarius updated imnodes recently to include the SetImGuiContext() function, so we should probably be able to merge this in now without too much trouble

@sonoro1234
Copy link

cimplot wraps implot_internal by default now

@sonoro1234
Copy link

Nelarius updated imnodes recently to include the SetImGuiContext()

cimnodes also regenerated

@sonoro1234
Copy link

Cimgui 1.82 generated

@TillAlex
Copy link
Contributor Author

Recent native libraries can be found in the AppVeyor build artifacts of ImGuiNET/ImGui.NET-nativebuild#3.

@freakbyte
Copy link

Awesome :D Thank you!

@TillAlex
Copy link
Contributor Author

It seems that I took the non docking definitions.json. Will fix this later on....

@freakbyte
Copy link

Was just about to ask if you had it working with docking as well :)

@TillAlex
Copy link
Contributor Author

TillAlex commented Mar 19, 2021

Definitions for cimgui from docking_inter brach are used again. Just did a short test with docking enabled and it seems to work.

@TillAlex
Copy link
Contributor Author

TillAlex commented Mar 19, 2021

I switched my branch of ImGui.NET-nativebuild to cimgui master accidently. Now it's using docking_inter branch again. Here is the corresponding pull request:

ImGuiNET/ImGui.NET-nativebuild#3

The appveyor pipeline there generates working native libraries for windows and linux.

@mellinoe mellinoe merged commit 2d1fc76 into ImGuiNET:master Mar 21, 2021
@mellinoe
Copy link
Collaborator

mellinoe commented Mar 21, 2021

@TillAlex I've merged this with a few adjustments applied in follow-up commits. I have not pushed any official packages using this yet, but I'd like to do that soon for the core library, because it's overdue for an update.

EDIT: Just wanted to say that this is an awesome contribution, thanks again!

@TillAlex
Copy link
Contributor Author

@mellinoe Thanks for merging!

@viliwonka
Copy link

Ok I red whole thread, but I am still confused - how do I get to include Implot (and others) into docking version?

If I understand, everything is available already, just binaries are missing?

I wish to make this available for Unity, normal docking Imgui is working already :).

@viliwonka
Copy link

Ok, got it working on Windows/Unity by downloading this repo: https://github.com/TillAlex/ImGui.NET-nativebuild/tree/additional_libraries and running build-native.cmd

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.

None yet

9 participants