Imgui integration #91

z-adams · 2021-03-06T06:31:04Z

Add ImGui, ImPlot, and a simple SysGui to test them

Implement Magnum's ImGui integration, and also include ImPlot (a handy plotting library for ImGui). ImPlot is not part of the ImGui integration, so its context must be managed manually. Add a simple FPS counter in OSPMagnum for now; need a better GUI system

src/test_application/OSPMagnum.cpp

EhWhoAmI · 2021-03-08T01:11:38Z

Have you tested if ImPlot works? I tested it, and it doesn't work on my computer.

z-adams · 2021-03-08T01:22:44Z

Good point, I'll test it tonight. I tested ImGui and it worked, and the ImPlot stuff was just copied from my other branch where it also works. I'll let you know.

z-adams · 2021-03-08T07:28:26Z

@EhWhoAmI I tested it and it works fine, by default the plot scales are weird so double click on the plot area and it should auto-fit the data. I added a graph to the fps counter window, if you can't see it then expand the window until it's visible.

EhWhoAmI · 2021-03-09T01:51:36Z

Alright, great!

jonesmz · 2021-03-10T05:24:11Z

modules/FindMagnumIntegration.cmake

@@ -0,0 +1,309 @@
+#.rst:


No way to avoid copying these two files into our repo? We can't use the ones in the magnum repo directly somehow?

jonesmz · 2021-03-10T05:25:13Z

src/CMakeLists.txt

@@ -93,6 +98,9 @@ endif()
 # Include ENTT (header only lib)
 target_include_directories(osp-magnum PRIVATE ../3rdparty/entt/src)

+# Include ImPlot (simple library that depends on magnum-integration's imgui)
+target_include_directories(osp-magnum PRIVATE ../3rdparty/implot)


Doesn't this automatically get picked up when you add the magnum-imgui library to the list of libraries to link to?

I believe that implot is separate from ImGui, so you'll have to include it separately

That's true, but it's not what I was asking about.

Normally in CMake, when you have a library, even if it's header-only, you create a "library" in cmake that has all the include directories for the library tied to it.

Then, instead of having your own project use "target_include_directories", you simply do "target_link_libraries" and that automatically populates any shared libraries, static libraries, dependencies, and include directories for you.

So my confusion was "Why aren't we using the cmake "library" for imgui/implot, instead of directly adding their include directories?" If we're doing it this way because imgui/implot don't have cmake "libraries", then that's terrible and we should probably send them patches to fix their broken.

I can't remember why I did it this way, but generally implot/imgui are meant to be included directly since they're small libraries with no dependencies. Feel free to explain in more detail how I should go about trying what you're suggesting.

You should make an "interface" library (or use the one that imgui and implot define, in their own cmake file), and then use "target_link_library(MyImPlotInterfaceLib)" to add that library.

The interface library doesn't have any cpp files or shared librarys / static libraries. It just exists to provide an ergnomic way to refer to a header-only-library and all of it's dependencies.

jonesmz

Great start!

EhWhoAmI · 2021-03-08T01:16:53Z

src/test_application/flight.cpp

+        {
+            using namespace Magnum;
+
+            ImGui::Begin("Debug");


Change the window name to Application Status so that it's clearer what it is?

EhWhoAmI · 2021-03-09T02:06:02Z

src/test_application/OSPMagnum.cpp

        // temporary: draw using first camera component found
        scene.draw(scene.get_registry().view<osp::active::ACompCamera>().front());
    }

-
    // TODO: GUI and stuff


You can take out this comment

Capital-Asterisk

I'm a bit skeptical of how well this will work out further on; it doesn't seem to fit in the ECS that well. I think the main issue is how the state of the gui stored in OSPMagnum, but are all processed from ActiveScene. It looks like a similar case to how SysNewton was originally designed, where it previously stored the physics world in the system which led to spaghetti. Also consider how GUIs should be renderable onto textures and stuff.

Some other possible scheme I thought about:

Entity 1:
- ACompImguiContext: component that stores the Imgui context
- ACompGuiToScreen: tag component to tell a system to render this context to the screen
Entity 2:
- ACompGui: refers to an entity that contains a ACompImguiContext (entity 1)
- ACompGuiDebug: tag struct to render the "Debug" window here
- ACompGuiShipStatus: tag struct to render "Ship Status" window here

Capital-Asterisk · 2021-03-11T06:58:56Z

src/osp/Active/SysGUI.h

+
+struct ACompGUIWindow
+{
+    std::function<void(ActiveScene&)> m_function;


Components should be state, and this is storing behaviors. Why not just put these directly into the render order?

So get rid of the GUI system, and just insert the lambdas directly? I guess that could work.

Capital-Asterisk · 2021-03-11T06:59:45Z

src/osp/Active/SysGUI.cpp

+    for (auto [ent, describeElement]
+        : rScene.get_registry().view<ACompGUIWindow>().each())
+    {
+        describeElement.m_function(rScene);


So FunctionOrder is a list of function that happens to call this function, which calls another list of functions.

Capital-Asterisk · 2021-03-11T08:12:14Z

src/test_application/OSPMagnum.h

@@ -75,6 +77,10 @@ class OSPMagnum : public Magnum::Platform::Application
    constexpr MapActiveScene_t& get_scenes() { return m_scenes; }

 private:
+    Magnum::ImGuiIntegration::Context m_imgui{Magnum::NoCreate};
+    ImPlotContext* m_implot;


Is there any problem with storing these contexts in the scene instead?

Yes, OSPMagnum is where the key events are polled (see OSPMagnum.cpp), and need to be passed directly to the m_imgui member. The ImGui context is more akin to the OpenGL context (similarly stored internal to Magnum::Platform::Application) and doesn't belong to any particular scene.

z-adams · 2021-03-11T19:41:21Z

@Capital-Asterisk part of the weirdness comes from the way ImGui works in general. It's completely stateless and has a global context, so the weird std::function components do actually store all the GUI state (in mutable capture variables), they just also have to store the instructions that draw the pane. I guess it would be worth thinking about how a non-immediate GUI system should be structured though, and see if we can cram imgui into that framework.

Drawing to textures has nothing to do with ImGui as it will just draw to whatever framebuffer is actively bound, does our existing renderer have support for that yet in general? We might need to do that first before we can worry about it for the GUIs.

Capital-Asterisk · 2021-03-11T22:41:33Z

@z-adams

It's completely stateless and has a global context, ...

Imgui is very not stateless; its current context and window is stored as global variables. There can be multiple contexts that can be switched between using SetCurrentContext.

the weird std::function components do actually store all the GUI state (in mutable capture variables), they just also have to store the instructions that draw the pane.

The state of the windows (window size, position, and stuff) is stored in the context. The windows are identified by their title. I don't see any lambda captures in the std::functions, and I really doubt they store anything.

Drawing to textures has nothing to do with ImGui as it will just draw to whatever framebuffer is actively bound....

This barebones GUI doesn't need all the features yet, but should at least be structured in a way that supports them. Your current implementation doesn't specify where the GUIs are going to be drawn, and rendering onto virtual instrument screens is one of the intended features. This is why I suggested storing contexts in components, and having a ACompGuiToScreen tag to specify where the GUI is rendered to.

Capital-Asterisk · 2021-03-11T23:16:54Z

The way Magnum Integration encapsulates the Imgui contexts might make the changes I suggested a bit more difficult. Magnum::ImGuiIntegration::Context can be thrown around as a component, but it also bundles a full-on application with it, with OpenGL dependent resources. It looks possible to handle and switch between multiple instances of Context. The on-screen gui component would have to store a reference to OSPMagnum.

Like the physics world, contexts can be temporary states that can be deleted and just get regenerated if they're not present. Window states can be saved some other way that Imgui probably has a solution for.

z-adams · 2021-03-12T00:04:50Z

@Capital-Asterisk Is window position and size the only thing that the ImGui context stores? Values within the windows (collapse state, tickmark states, slider values, etc) are all stored by the user, which is what I have in my functions (the std::arrays initialized in the FPS counter definition capture).

If what you're saying is true, then I'm sort of tempted to either ditch Magnum's ImGui integration, or just start writing our own GUI. For the MVP we literally need like 5 numbers as far as GUI goes, so we aren't really in immediate (lol) need of much.

Ditching Magnum's ImGui integration might be the best option as it would allow us to keep using ImGui but retain full control of the contexts. I've already done it this way for ImPlot, so it wouldn't be much different. Thoughts?

z-adams · 2021-03-21T04:45:21Z

Just pushed a commit (most recent one) with a sort of intermediate experiment in decoupling the GUI from OSPMagnum a bit. I left the event handling inside OSPMagnum, but moved the GUI contexts to components which live in the ActiveScene. The GUI system activates the relevant contexts at the beginning of its update, and then calls the lambdas in the components I had before. It's pretty rough still; in particular, I'm not quite sure where OSPMagnum should decide which scene's GUI contexts to act on (ultimately this would be determined by whichever scene the mouse is currently captured by). I made a function in ActiveScene to fetch the GUI context, but for now I just manually assign it to OSPMagnum's member pointer in flight.cpp.

I have some cleanup to do and I just realized I can move more of the GUI rendering code from OSPMagnum to SysGUI (the frame clearing and drawing stuff), but maybe the overall scheme will stimulate some ideas. I suspect @Capital-Asterisk won't like OSPMagnum extracting the GUI information from ActiveScene this way but it's pretty convenient and I haven't figured out a clean way to freeze the control inputs which should be handled by ImGui from within the ActiveScene. I'm open to suggestions though.

Capital-Asterisk

Considering rendering GUIs to textures and instrument screens, there should be components to determine which context the ACompGUIWindows are being rendered to. Either a vector stored along side the context that lists entities containing windows, or components in the window entities that point to a context. I'm not quite sure if it's that important to decided that right now though.

Capital-Asterisk · 2021-05-02T02:11:09Z

src/test_application/OSPMagnum.h

    void drawEvent() override;

    osp::UserInputHandler m_userInput;

+    Magnum::ImGuiIntegration::Context* m_activeImgui{nullptr};


This doesn't seem to point to stable memory

jonesmz · 2021-06-24T19:04:06Z

@z-adams I know you're not really actively working on this, but if you have time, could you rebase this PR on the latest code so that the compile errors are taken care of?

z-adams · 2021-06-25T02:08:46Z

@jonesmz This was last updated before the current rendering system was made so it'll take a little while to bring up to date. I may have time to do it this weekend, I just can't do it at the moment

jonesmz · 2021-09-04T16:28:28Z

@z-adams Closing, because it's been open for several months without action.

Feel free to re-open if you decide to work on the imgui stuff again, but I know your FPS game is taking most of your time.

z-adams added 7 commits March 5, 2021 22:23

basic flight info

a87c028

tmp stash old sysgui

70fe7ec

Slightly less abominable GUI system

4fde368

fix erronious OSPMagnum commit

525b568

pre-pr fixes

ac90694

hopefully fix include

163a154

z-adams requested review from jonesmz, Capital-Asterisk and EhWhoAmI March 6, 2021 06:31

z-adams commented Mar 6, 2021

View reviewed changes

src/test_application/OSPMagnum.cpp Outdated Show resolved Hide resolved

z-adams linked an issue Mar 6, 2021 that may be closed by this pull request

Add barebones HUD #82

Open

ImPlot fps example

93d18de

jonesmz reviewed Mar 10, 2021

View reviewed changes

jonesmz approved these changes Mar 10, 2021

View reviewed changes

EhWhoAmI approved these changes Mar 11, 2021

View reviewed changes

Capital-Asterisk reviewed Mar 11, 2021

View reviewed changes

z-adams added 2 commits March 20, 2021 14:32

Quick custom deleter

081c718

decoupling from OSPMagnum (a bit)

e8d36f6

Cleanup

88c956a

PR cleanup stuff

5ac7633

Capital-Asterisk reviewed May 2, 2021

View reviewed changes

jonesmz closed this Sep 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Imgui integration #91

Imgui integration #91

z-adams commented Mar 6, 2021

EhWhoAmI commented Mar 8, 2021

z-adams commented Mar 8, 2021

z-adams commented Mar 8, 2021

EhWhoAmI commented Mar 9, 2021

jonesmz Mar 10, 2021

jonesmz Mar 10, 2021

EhWhoAmI Mar 11, 2021

jonesmz Mar 11, 2021

z-adams Mar 11, 2021

jonesmz Mar 25, 2021

jonesmz left a comment

EhWhoAmI Mar 8, 2021

EhWhoAmI Mar 9, 2021

Capital-Asterisk left a comment •

edited

Loading

Capital-Asterisk Mar 11, 2021

z-adams Mar 11, 2021

Capital-Asterisk Mar 11, 2021

Capital-Asterisk Mar 11, 2021

z-adams Mar 11, 2021

z-adams commented Mar 11, 2021

Capital-Asterisk commented Mar 11, 2021

Capital-Asterisk commented Mar 11, 2021

z-adams commented Mar 12, 2021 •

edited

Loading

z-adams commented Mar 21, 2021

Capital-Asterisk left a comment

Capital-Asterisk May 2, 2021

jonesmz commented Jun 24, 2021

z-adams commented Jun 25, 2021

jonesmz commented Sep 4, 2021

Imgui integration #91

Imgui integration #91

Conversation

z-adams commented Mar 6, 2021

EhWhoAmI commented Mar 8, 2021

z-adams commented Mar 8, 2021

z-adams commented Mar 8, 2021

EhWhoAmI commented Mar 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonesmz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Capital-Asterisk left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

z-adams commented Mar 11, 2021

Capital-Asterisk commented Mar 11, 2021

Capital-Asterisk commented Mar 11, 2021

z-adams commented Mar 12, 2021 • edited Loading

z-adams commented Mar 21, 2021

Capital-Asterisk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonesmz commented Jun 24, 2021

z-adams commented Jun 25, 2021

jonesmz commented Sep 4, 2021

Capital-Asterisk left a comment •

edited

Loading

z-adams commented Mar 12, 2021 •

edited

Loading