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

Internal functions not marked as such #16

Closed
ocornut opened this issue Oct 9, 2024 · 11 comments
Closed

Internal functions not marked as such #16

ocornut opened this issue Oct 9, 2024 · 11 comments

Comments

@ocornut
Copy link

ocornut commented Oct 9, 2024

Hello,

It would be good, and imho important that function/symbols from imgui_internal.h are clearly separated and marked as such.
Right now people may be incentivized to use unstable/undocumented API, which is an issue both for me as a maintainer and for the users.

As cimgui doesn't provide this information, a possible way to do it would be to run cimgui twice, with and without imgui_internal.h, and infer which symbols are internals?

FYI the fact that cimgui doesn't tag internals (cimgui/cimgui#124) has been for me a very big concern to me, and is one of the main reason I have suggested the development of dear bindings. dear bindings (https://github.com/dearimgui/dear_bindings) now supports imgui_internal.h parsing and output are separated.

If the suggested workaround (comparing two runs of cimgui) is not possible, I would suggest that you either (1) push or work on cimgui to get this fixed, or (2) switch to dear bindings.

My two cents!

@ocornut
Copy link
Author

ocornut commented Oct 9, 2024

I realized that even though the cimgui.h output combines both public and internal API, the location is indeed available in the metadata:

"location": "imgui_internal:2963",

Therefore AFAIK the data is available to you.

I think it would be good if you can move those to a separate namespace e.g. ImGuiInternals?

(see e.g ocornut/imgui#8047 feedback from someone using an internal function, possibly without realizing it, because the info is not visible to the C# user)

@JunaMeinhold
Copy link
Member

JunaMeinhold commented Oct 9, 2024

Hi,
I don't use the metadata for generating the bindings I use cpp ast and parse the headers directly. And i use the metadata for naming and comments.

Additionally changing it now would be a nightmare for my custom widgets.

like this one https://github.com/HexaEngine/HexaEngine/blob/main/HexaEngine.Core/UI/ImGuiColorTrackballWidget.cs
image

@J8-8N
Copy link

J8-8N commented Oct 9, 2024

(see e.g ocornut/imgui#8047 feedback from someone using an internal function, possibly without realizing it, because the info is not visible to the C# user)

Hello, no I realize it very well. And I'd rather have these internal functions in the bindings. Users are free to use them or not. For example, part of the ImPlot more complex demos require internals which ImGui/ImPlot bindings prior to HexaEngine didn't provide. So CPP users could do stuff that CSharp users couldn't.

@ocornut
Copy link
Author

ocornut commented Oct 9, 2024

Additionally changing it now would be a nightmare for my custom widgets.

Why would it be a nightmare? Wouldn't it be a matter of adding a using directive and/or changing some ImGui. into ImGuiInternal or whatever is emitted by the bindings?

Hello, no I realize it very well.

OK! But we had this problems for other users. cimgui by default doesn't distinguish them.

I'm not arguing against having internal functions exposed in bindings! In fact, people who tell me they don't have access to internals I tell them that their binding should give them access. So I am glad this project does.

But they should clearly be marked as much, and ideally separated (different header file, namespace, comments etc. whatever makes sense in the language).

@JunaMeinhold
Copy link
Member

I managed to isolate the internals api, i still need some time to patch the function pointer table, but I'm unsure how to name it ImGuiInternals is a bit long maybe ImGuiInt, idk, any suggestions?

@ocornut
Copy link
Author

ocornut commented Oct 9, 2024

Thanks, that's great!!

My own code occasionally uses a P suffix, to mean "private".
Technically this doesn't align with the "internals" terminology but I think it may be a good choice to use "ImGuiP".

@J8-8N
Copy link

J8-8N commented Oct 11, 2024

Hello, hijacking this a little but do you think backends should/could be included in bindings too? IIRC CImGui has an option to embed these. So a CSharp user could just reuse the battle-tested Win/Mac/Linux examples instead of brewing his own from other CSharp bindings found here and there. Thank you

@JunaMeinhold
Copy link
Member

JunaMeinhold commented Oct 11, 2024

Hello,
That's not possible because then the lib(s) wouldn't be dependency free. Not everyone uses Silk.NET there are different libs for the same stuff like OpenTK.

(Or insert any lib or using void pointers or nint (untyped pointers), either you have deps or users who don't know what they are doing)

If someone needs help with creating a backend for a lib then I'm happy to help and include it later in the examples.

@ocornut
Copy link
Author

ocornut commented Oct 11, 2024

If you can provide bindings for standard backends that'd be a good idea. They are more likely to be full featured e.g. include tested support for multi-viewports.

Because the backends have very few entry points, it may even be ok to create the bindings manually if that's easier.
But as mentioned, they should be separate/standalone libs to not drag extra dependencies.

@JunaMeinhold
Copy link
Member

JunaMeinhold commented Oct 11, 2024

It's hard to do, cimgui doesn't provide access to all backends and managing platform dependent code is always a problem especially with C -> C# bindings, but i could port the rest of them to C# too like i did with the SDL2, D3D11, OpenGL3 backends.

(they are almost 1:1 to their cpp counterparts from docking branch)
ImGuiD3D11Renderer.cs
ImGuiOpenGL3Renderer.cs
ImGuiSDL2Platform.cs

@JunaMeinhold
Copy link
Member

Moving this topic to #17.

Closing this issue as done. See latest release.

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

No branches or pull requests

3 participants