Update ImGui to 1.90.4

[DannyKaufman]

This update would be really handy! @nmwsharp : any downsides to updating ImGui in Polyscope with this now or is it an easy merge?

[nmwsharp]

Hi @jdumas and @DannyKaufman, thanks for filing this! I'm happy to update ImGUI to latest, just have a few questions to resolve about the PR (inline questions above and one more below).

[nmwsharp]

Can you try running the tests locally?

cd test
mkdir build && cd build
cmake -DCMAKE_BUILD_TYPE=Debug ..
make -j4 && ./bin/polyscope-test --gtest_catch_exceptions=0 backend=openGL3_glfw

on my machine this is failing. Unfortunately this is not caught by the CI tests, since they run on headless servers they use a 'mock' backend that doesn't actually initialize imgui, the tests need to actually be run locally.

I suspect there may be issues with the ImGui Context, possibly related to this change https://github.com/nmwsharp/polyscope/pull/260/files#diff-edffb23854079d0270fd0602f55895fb564f6bead31f7b88458e8e9c0f6a6c32R2328. Polyscope does some "hacky" imgui tricks storing contexts on a stack which you can see here.

This complexity is to support the case of "nested show" where:

• (1) the user clicks a button in the UI, which triggers some application code
• (2) that application code itself calls polyscope::show(), usually because the caller wants to launch the GUI to debug something inside the function that was called by the button
• (3) that inner polyscope::show() starts looping the main loop, which itself starts making ImGui commands from scratch, even though we were already in the midst of an ImGui frame in the outer context
• (4) eventually the inner show() concludes and we resume processing the outer context

It seems that ImGui isn't really meant to support this workflow, and the current (seemingly-working) setup was found by some trial & error & inspecting ImGui internals. It's pretty likely that any change to ImGui context handling (or even just a version bump of ImGui) might break this.

[jdumas]

Hi @nmwsharp, thanks for the review! Regarding the nested ImGui context, it looks like a combination of two issues:

• I was testing in my client code using the ImGui docking branch, which requires some additional shenanigans with ImGui::GetPlatformIO() to propagate the PlatformHandle to the nested context. I've fixed this issue in the latest revision.
• When testing locally the polyscope tests, it still fails on these two lines with what I think is an overzealous assertion on the ImGui side of things. I've mentioned the issue in this thread, but fixing it may require either waiting for an ImGui update or overwriting the IM_ASSERT_USER_ERROR macro... EDIT: Actually there's a simple workaround that doesn't require patching ImGui, so this should work out nicely.

(I suggest squashing this PR when you merge it into the main branch btw...)

[nmwsharp]

I made one more small change (pushed above) to just add the imgui obsolete check in our test & demo apps (personal preference, to avoid introducing a new cmake variable).

[nmwsharp]

I was about to merge this, however I locally built and tested the demo app (examples/demo-app), and for some reason it is totally unresponsive to user input after this change (observed on my local laptop, a recent ARM macbook w/ appleclang 15). Something is going wrong around the context push and main loop iteration.

I spent a while debugging but couldn't figure out what the problem is. Unfortunately I'm out of time to look at this tonight, can you reproduce the same behavior / do you have any idea what the cause might be?

[jdumas]

I made one more small change (pushed above) to just add the imgui obsolete check in our test & demo apps (personal preference, to avoid introducing a new cmake variable).

Good call, that's probably a better fix if we want to catch this via the CI/CD anyway.

I was about to merge this, however I locally built and tested the demo app (examples/demo-app), and for some reason it is totally unresponsive to user input after this change (observed on my local laptop, a recent ARM macbook w/ appleclang 15). Something is going wrong around the context push and main loop iteration.

Indeed. I tried to push a different fix. Tested locally on both the docking + master branch with both the polyscope-demo app and the unit tests. I can also test on my Windows machine tomorrow for good measure.

[nmwsharp]

Thanks for the fix! That also resolved the unresponsiveness on my end.

A final round of local testing revealed one more problem: scroll inputs weren't getting processed. After some debugging, it seems that ImGui has changed the order of input event processing. We need to read the input events while the frame is active or they would be cleared before reading.

I moved the processInputEvents() and other input handling to be right after ImGui::NewFrame(). That seems to resolve the issue. I'm slightly concerned that this will subtly break some other behavior, but I tested a bit and couldn't find anything, so I'm happy to go ahead and merge at this point.

[nmwsharp]

merged! thanks for the PR

[jdumas]

Thanks! I also compiled & ran the polyscope demo and unit tests on Windows just to be sure, and it worked, except for this error in the unit tests (which I also have with the old master branch, so I think it's an unrelated issue):

[nmwsharp]

Thanks for testing on Windows, I probably should have but it's always an awful lot of work to boot up my Windows machine.

That bug is indeed likely unrelated, there is some stubborn low-level bug that occasionally crops up in Windows which I have not been able to find yet. It also causes a hang in some unit tests which are currently skipped. You can see some of my efforts to debug here https://github.com/nmwsharp/polyscope/compare/test_windows_mesh_hang . One day I will get to it.... (or maybe someone else will debug it in the meantime 😄 )

[jdumas]

I probably should have but it's always an awful lot of work to boot up my Windows machine.

Ahah. I keep mine permanently turned on at the office, and just remote desktop into it. It's honestly not that bad :D

Anyhow, it's been a while since I've done any OpenGL so not sure I'll be able to help on this one. But I'll keep an eye out to see if it causes any inconvenience...