[WIP] Docking

[nlbuescher]

New PR for docking branch. Will flesh out further in a bit

[nlbuescher]

Some thoughts:

1. ImGuiID common constructor doesn't work because of different implementation on jvm vs native (long vs uint). Could be done with a default constructor, but I can't get kotlinpoet to actually generate those two parentheses next to the class name...
2. ImGuiID default value only occurs in a single internal function. Are the definitions from imgui_internal necessary in the client API?

[Dominaezzz]

I initially didn't want to expose imgui_internal methods but there wasn't an easy way to skip internal functions. There's a recent commit of cimgui that now has this information in the json files, so we can skip them now. I was thinking of using this to mark the internal methods as "RequiresOptIn" but if removing them save us trouble then I'd prefer that.

[nlbuescher]

I agree. They're functions that in imgui are intended for calling internally, and in cimgui are exposed for wrapper writers to be able to mess with the internals as needed for their wrappers, as I recall from reading an issue in their repo.

[nlbuescher]

I just checked, and that change is incorporated into the docking_inter branch. I'll see about adding the check for "location": "internal" to the codegen in my branch

[nlbuescher]

I just ran into an issue that the current imgui:docking is out of sync (in API) with cimgui:docking_inter due to some function renaming, (as is to be expected of a WIP feature branch). Could be fixed by add a gradle task to run the cimgui generator.

[nlbuescher]

I added a task for generating cimgui from the downloaded imgui version, but it requires lua, and understandably breaks CI. Not sure what to do about that one yet. Running the generator task with the build would also require running swig as part of the build, and that'll need to be added to the CI too.

[Dominaezzz]

I'm a bit hesitant about installing Lua (Mostly from lack of experience). It's a bit out there but we could setup a separate CI task (on just Ubuntu) to run the cimgui generator and publish the zipped results as an artefact, which can be downloaded by a Gradle task.

On second thought, maybe this should be done upstream in cimgui itself.

Too much?

[nlbuescher]

I think that's a great idea actually. installing lua is trivial in ubuntu with apt, and then all the other builds can benefit.

I'm not sure what you mean by "done upstream"? the cimgui branches already contain the generated files, but that doesn't matter if they're out of sync with the imgui version that we download separately (which we have to do because we can't synchronize git submodules with an archive zip). We could try, rather than downloading the archive, cloning the git repository and initializing the submodules with exactly the version of imgui that the cimgui version was generated with, but that would create a build dependency on git.

I think I should do some reading up on GitHub actions. It feels like adding dependencies to common technologies in projects should be a simple matter for a CI system, no matter the platform. Maybe we don't need to make this as hard as it seems.

[Dominaezzz]

By "done upstream", I mean the cimgui repo itself should publish a zip of the ouput folder for each commit (on selected branches, tags or something reasonable) instead of committing the generated files.

That way, each commit would always have up-to-date builds and it could even publish different generation configs e.g. with internals, without internals, without impls, etc.

[nlbuescher]

That makes sense. However, tagged archives currently also don't include imgui, which they would have to in order to facilitate any usefulness in this regard.

Good news is: it looks like we can actually rely on the host environments to have git installed: https://help.github.com/en/actions/reference/software-installed-on-github-hosted-runners

We can also use miniconda to install swig on windows. I'll give it a go

[Dominaezzz]

they're out of sync with the imgui version that we download separately

I just read this again. What's wrong with using a specific imgui commit?

[nlbuescher]

Nothing per se. Currently the cimgui docking_inter branch is behind the imgui docking branch. That causes compile errors. The cimgui branch itself has a reference to the exact imgui commit that was used to generate the bindings, but that information isn't available if you download the zipped archive like we do in master.

The reason getting the current version of the docking branch is necessary is that there are no tags created for the docking branch versions in either cimgui nor imgui, like there are for stable releases.

So to get the exact right imgui version, the best approach (for the docking branch), is to clone the cimgui docking branch and initialize the imgui submodule to get the exact commit the cimgui branch uses.

[nlbuescher]

That took way too long to get right.

[Dominaezzz]

Happens to the best of us. It's gotten to a point where I use "take 1", "take 2", etc in my commit messages. Usually I try to do them in low-key branches that aren't PR'ed and squash it all, pretending I got it right the first time. :rofl:

[nlbuescher]

What are your thoughts now that it builds and passes CI?

[Dominaezzz]

Doesn't docking need changes to the ImGuiGLFW backend?

[nlbuescher]

Good catch. I hadn't thought of that. I'll compare the master and docking sources in imgui and make those changes

[nlbuescher]

So the ImGui_ImplGlfw_ and other impl functions have definitions in a json as part of the generator output in cimgui. Is there a reason for not using those? I'm seeing a lot of messiness trying to port the code from imgui because of the lack of a Kotlin API in GLFW to manipulate Monitors and I suspect there will be problems (read "unexpected differences") on Windows because of the missing platform specific code. The functionality is exactly the same as the imgui examples.

[Dominaezzz]

At the time, I wasn't sure why I was convinced that I had to write it from scratch in Kotlin. (Other bindings seem to do this as well).

About doing it now, not sure if it will work across shared/static libraries. Also setting up SWIG to work across projects looks like a pain as it's fairly stiff.

[nlbuescher]

I'll give it a go and see what happens. I suspect that getting the correct glfw version of the headers for the cimgui compilation will be a topic (to make sure it matches with kgl), but SWIG itself shouldn't be a problem since it's only used for the cimgui cinterop (which would be the part that's added to).

Side-note: I haven't gotten kgl-glfw to work without the -static counterpart (although I haven't tried in a while)

[Dominaezzz]

Found a good reason! The imgui_glfw employs compile time compilation of features but someone using kgl-glfw may be dynamically (or even statically) linking any version of GLFW! We will have to deal with this at runtime.

This is an unfortunate consequence of imgui encouraging source compilation and having compile time config.

[nlbuescher]

Have you been able to get kgl-glfw to work at all? I can't get a project without the -static variant to link on macOS, nor on Linux, despite having glfw installed

[Dominaezzz]

Without the -static you have to specify linkerOpts to point to the dynamic glfw lib.

You mentioned that you use Arch (so do I), so linkerOpts("-L/usr/lib", "-lglfw") should work for you.

[nlbuescher]

linkerOpts

Where would I do that? I had that thought, but since the cinterop configuration isn't defined in my project I'm not sure where to do that.

EDIT: I found linker opts in the binary settings. Gonna try that

I see what you mean though: because the imgui implementation is choosing which features to use at compile time through the preprocessor based on the glfw version, those are checks we have to replicate in Kotlin at runtime (there are ways to change source sets based on Gradle properties and have different actual sources defined at build time, but that doesn't work for libraries).

The part I was confused about at first is what happens if someone links a version of GLFW that's newer than what the library was built for. I realize now that that would be undefined, but the version checks would theoretically enable using older versions of GLFW with kgl-glfw. I am curious though if that's actually the case given how Kotlin does compilation. If we try to link against an older I suspect it will try to link functions from the newer header against the older dynamic lib and fail.

[Dominaezzz]

Where would I do that?

linkerOpts can be specific in cinterop or in executable block. https://kotlinlang.org/docs/reference/building-mpp-with-gradle.html#configuring-binaries Libraries don't have to do this. Unless they want to run unit tests.... If you still have issues with this, message me on Slack and kgl README can be updated afterwards.

We can do multiple sourceSet per version (or feature level? idk) but consuming such library is quite painful. I've been trying to merge kgl-glfw and kgl-glfw-static (which pretty much have the same source) for months and the end result was never worth it. Maybe I'm just bad at Gradle module metadata.

I've actually tested this (with kotlin-sqlite though) and fortunately it behaves the same way as C/C++ compilers. It will only link (or resolve) functions that are reachable from entry points e.g. main, unit test methods, etc. So if you don't call a function from a newer version of GLFW than you've attempted to link, there isn't too much of an issue.

[nlbuescher]

Yeah I stumbled upon that option probably as you were typing. It works now, thanks!

Different source sets for a library (since it would require different publications) is just a pain and definitely not worth it. It's something I looked into doing to basically emulate profiling preprocessor macros as part of my debug build, but then I realized that the biggest problem with the performance was probably the 10000 matrix calculations I'm doing every draw loop that aren't using SIMD...

That's really good to hear, although it cements the fact that I'll definitely have to manually port the implementation code. At least I learned something today. 👌

[nlbuescher]

So I've spent the last 10 days trying to port the implementations from the Docking branch. On the first attempt, I got a crash (segfault) when trying to drag an ImGui window outside the main window. On the second attempt, I get nothing. No error message, no window. The executable starts running, then stops, and nothing happens. There's something I'm doing wrong, but I'm not sure what it is. Any advice from when you ported the current implementations?

[Dominaezzz]

When I did the initial implementation, I didn't have any issues with GLFW. I only had issues with OpenGL, so much so that you even fixed one of the issues months later after publishing.

Have you also made the relevant changes to the ImGuiOpenGL?

[nlbuescher]

I did. That was the very first (what I would consider) half-attempt. First full attempt includes the OpenGL implementation. Big difference between docking and initial impl is that the docking impl creates new windows on the fly and sets various listeners for them.

I'm also not sure if I'm doing the compile-time checks correctly with OpenGL. it checks of GL_CLIP_ORIGIN and GL_POLYGON_MODE and such are defined, which I took to mean, "check if these features are supported based on the version of OpenGL found". I noticed in your initial impl that you made those parameters to a constructor, but I actually had to make the implementations objects again because of an "inner" class for ImGuiGlfwViewportData that stores the viewport's window instance that needed to be constructed in a staticCFunction (I found out that way that actually inner classes are "captured" and that's not allowed with staticCFunction)

[Dominaezzz]

Push your changes to a branch I can take a look. A second pair of eyes might help.

About the compile-time checks, I've been a bit optimistic about them and haven't tested them at all. I did have the same understanding though. Do you really need an inner class? The object path worries me because of Kotlin/Native's memory model. If the staticCFunction doesn't take a user pointer or similar then it is worth opening an issue in imgui itself.

[nlbuescher]

Do you really need an inner class? The object path worries me because of Kotlin/Native's memory model.

unfortunately, yes, because the Viewport's createWindow function creates new windows as a child of the main window, which requires a reference to the global window given to the implementation's init function (constructor in your example).

The memory model isn't an issue because all properties are stored top-level (ie globally). The object is simply there to encapsulate the functions in a sort of namespace (so I don't have to prefix all the functions with ImGuiGlfw_)

If the staticCFunction doesn't take a user pointer or similar then it is worth opening an issue in imgui itself.

the user pointer is stored in the viewport object passed to the staticCFunction by ImGui, but constructing an inner class inside a staticCFunction to set that user pointer is what "captures" the inner class.

I'll try to restore the most working example and push that for reference.

[nlbuescher]

Note:

1. The Kotlinic public functions in the impls sometimes call private functions that take a CPointer because those functions are also used by the ImGui managed viewport windows which don't have kgl Window objects.
2. This is the second attempt. I can't restore the first attempt because I overwrote it without saving it.
3. I'll have to add a flag and check for OpenGL ES again later. I initially left it out because the native targets only include desktop, but I then remembered Android for JVM (and possibly WebGL later on)
4. I have 0 clue what to do with JVM, so those implementations are unchanged.

[Dominaezzz]

About the memory model, global objects (or instances rather) have to either be frozen or thread local. If we have a class then user can decide which model but if we have an object then we have to decide.

After reading through, I'm starting to shiver at the thought of making this also work on the JVM. Haven't run it yet but the first thing I'll say is you don't need the global object at least for GLFW. All the methods that take viewport: CPointer<ImGuiViewport>? as the first argument and passed to platformIO can be made members of ImGuiGlfwViewportData.

These here,

private fun getWindowFocus(viewport: CPointer<ImGuiViewport>?): Boolean
private fun getWindowMinimized(viewport: CPointer<ImGuiViewport>?): Boolean
private fun setWindowAlpha(viewport: CPointer<ImGuiViewport>?, alpha: Float)

would now become

fun ImGuiGlfwViewportData.getWindowFocus(): Boolean
fun ImGuiGlfwViewportData.getWindowMinimized(): Boolean
fun ImGuiGlfwViewportData.setWindowAlpha(alpha: Float)

then val data = viewport!!.pointed.PlatformUserData!!.asStableRef<ImGuiGlfwViewportData>().get() can be done in the individual staticCFunctions and the corresponding member can be called. This would also make things easier for multiplatform as we'll be able to isolate the uncommon parts.

[nlbuescher]

About the memory model: properties inside global objects are required to be either threadlocal or frozen, but top-level (not inside an object) properties are static and do not have this requirement.

The issue is the use of the mainWindow in createWindow(CPointer<ImGuiViewport>?) exact line for reference.

If (1) ImGuiGlfw is a class, and stores the window as a member, createWindow has to be inside the class to access it, and would need to capture the this reference. If (2) we instead move createWindow into the ImGuiViewportData class, the class needs to inner to access the mainWindow member.

Circumventing that by making the mainWindow property global defeats the purpose of having it be a class.

EDIT: To be clear, I had that approach in my initial, better-working attempt, and that's where I ran into the issue of capturing because ImGuiGLFW was a class

EDIT 2: Formatting

[Dominaezzz]

We can store mainWindow (or even the entire ImGuiGLFW if necessary) in this field https://github.com/ocornut/imgui/blob/a1d2c6fad96ec7a3e444094a1522dc1796ab68fe/imgui.h#L1500 .

[nlbuescher]

We could do that, although it has the same effect as storing it in a global property, so I'm not sure what the benefit is.

With a class, it's possible to create multiple instances, which would be problematic with a global storage like that. Should multiple instances even be supported (read allowed)? In order to avoid clashing with the memory model, all we need is to not have data directly in the object.

Also, with global storage, the benefit of having the user be able to choose the memory model for the implementation goes out the window. Why does the native memory model have to be so confusing? I love Kotlin, but there are some things that just make me go ???

[Dominaezzz]

:grin: Yeah I guess so.

The only benefit is making writing multiplatform code a bit easier.

Oooooh, I just had a thought. What if we use ImGui.getMainViewport()?

EDIT:

With a class, it's possible to create multiple instances, which would be problematic with a global storage like that. Should multiple instances even be supported (read allowed)? In order to avoid clashing with the memory model, all we need is to not have data directly in the object.

Good point. I think this should be done in a separate commit/PR then.

[nlbuescher]

We would need the main viewport to hold a reference to the Kotlin Window object rather than just a pointer to the GLFWwindow, but it could work. Could you elaborate on what you mean?

EDIT:

Good point. I think this should be done in a separate commit/PR then.

Fair enough. I'll see if I can get it working without.

[Dominaezzz]

We can make ImGuiGlfwViewportData hold on the Window object.

Just replace this line with ImGui.getMainViewport().ptr.pointed.PlatformUserData!!.asStableRef<ImGuiGlfwViewportData>().get().window. Then we can do the member delegation thing.

[nlbuescher]

I'll give that a try. Thanks for the input

[nlbuescher]

So I got the changes added, and it compiles and runs right up until you try and drag an ImGui window out of the main window. GLFW error, Cannot create OpenGL Context. I suspect this has to do with the abstraction around the plain GLFWwindow. Needs further looking in to.

[Dominaezzz]

Cool! I'll try running it now.

[Dominaezzz]

I'm guessing you ran into this.

/home/dominaezzz/IdeaProjects/kotlin-imgui/samples/build/bin/linuxX64/debugExecutable/samples.kexe ""

Glfw Error 65537: The GLFW library is not initialized
samples.kexe: /home/runner/work/kgl/kgl/glfw-3.3/src/posix_thread.c:62: _glfwPlatformGetTls: Assertion `tls->posix.allocated == GLFW_TRUE' failed.

[nlbuescher]

actually, no. I'm running on macOS right now.

[nlbuescher]

but as soon as I drag the window outside of the main window:

Glfw Error 65543: NSGL: Failed to create OpenGL context
Assertion failed: (viewport->RendererUserData == __null && viewport->PlatformUserData == __null && viewport->PlatformHandle == __null), function DestroyPlatformWindow, file /Users/nico/Developer/IdeaProjects/kotlin-imgui/cimgui/build/downloads/cimgui-docking_inter/imgui/imgui.cpp, line 11561.

[Dominaezzz]

Oops, didn't think this would bite and I forgot about it.

Comment out the window.close() and try running again. On my setup it creates the new window and doesn't crash. It doesn't work properly for me, probably because I'm using i3.

[nlbuescher]

very oops. That doesn't seem to be the issue on macOS though. Commenting out the window.close() in data.destroyWindow() has no effect. It can't create a new context for the new window for some reason. Might have to look into the macOS specific code that's currently ignored from the imgui source.

[nlbuescher]

I found this searching for the error.

[Dominaezzz]

I can't find anything obvious and I don't have a Mac to test. There are only 3 things I think I would try if I have a mac, you may have done these already

• See if upstream imgui docking example works
• See if you can create shared windows in an isolated test using kgl-glfw.
• Try copying the window hints used for the main window and using it for the viewport window.

As I typed the last one, I realised that the imgui version doesn't call glfwDefaultWindowHints() but kgl-glfw does internally. Hahahaha. I think that's it.

[nlbuescher]

Well, I'm glad we're finding all these bugs while we're at it 😂