ocornut / imgui

Dear ImGui: Bloat-free Graphical User interface for C++ with minimal dependencies
MIT License
59.76k stars 10.17k forks source link

It would be nice to use RAII for pushing/popping #2096

Open sethk opened 6 years ago

sethk commented 6 years ago

This is especially true because some things need to be popped even when they aren't open (e.g. child regions), but it's difficult to remember that.

Person-93 commented 5 years ago

I think there'd have to be a flag anyways. Even with RAII, I'd still want the option to explicitly pop the style. In practice that would probably be a method that calls pop and sets the flag.

jdumas commented 5 years ago

If you pop it manually, why use RAII at all? In any case, I think some of this can be implemented in a base, to avoid repeating the complexity all over the different variants (maybe using CRTP or something to accommodate for the different pop_ commands that are available).

Person-93 commented 5 years ago

I would want it for something like this.


{
    style_object mySuperCoolStyle( ... );
    // a bunch of code here
    if( cond ) mySuperCoolStyle.dismiss();
    // a bunch more code
    if( otherCond ) mySuperCoolStyle.dismiss();
    // ...
} // if it wan't popped or if an exception was thrown, it will be popped in the destructor
sethk commented 5 years ago

I would want it for something like this.

{
    style_object mySuperCoolStyle( ... );
    // a bunch of code here
    if( cond ) mySuperCoolStyle.dismiss();
    // a bunch more code
    if( otherCond ) mySuperCoolStyle.dismiss();
    // ...
} // if it wan't popped or if an exception was thrown, it will be popped in the destructor

Similarly to what @jdumas says, if we allow a style object to be popped in the middle of a block, then we've broken the connection between the style's lifetime and the block, meaning that there's no guarantee we're popping the correct style when dismiss() is called.

In general, my intent was not to achieve exception safety with RAII, but to solve a similar problem of forgetting to call the appropriate pop method manually. In order to have exception safety, you would need to use RAII for every possible push/pop of any type of context, and it seems like at that point you might as well start maintaining stateful UI elements instead of using immediate mode.

psocolovsky commented 5 years ago

can I ask why you do not merge ImTreeNode with TreeNodeEx and ImTreeNodeV with ImTreeNodeExV ? I don't see why having multiple classes when the constructors are different. the API is supposed to be a helper, the simpler the better I guess...

sethk commented 5 years ago

@psocolovsky Are you sure the constructors are unambiguous? If so, why are they different functions within the ImGui namespace instead of being overloaded? I guess it could just be a design choice. I actually didn't put much thought into this, and generated the wrappers with a script, so it didn't seem obvious that they should be combined.

psocolovsky commented 5 years ago

Hi sethk, if you look in the source code both TreeNode and TreeNodeEx end up calling the TreeNodeExV version. the V version is useless in the RAII wrapper, all of the calls made using your wrapper will be the TreeNode and TreeNodeEx versions, and if someone is using the variadic function ever is probably working at low level and RAII is unlikely to be desireable.

The TreeNode and TreeNodeEx have different function names probably because it is not rare to play around with a function and then change from ImGui::TreeNodeEx( x, ImGuiTreeNodeFlags_Selected, "hello"); to ImGui::TreeNodeEx( x, 0, "hello");

if TreeNodeEx is overloaded with TreeNode signature the compiler may either complain about ambiguity or call the wrong function passing 0 to const char* fmt triggering an assert failure or crashing at first string pointer dereference I think this is the most reasonable explanation of why having two different calls, however you should ask ocornut... maybe he just like it like that :D

daiyousei-qz commented 5 years ago

Hi, why not conditionally adds [[nodiscard]] attribute to those RAII wrapper structs for c++17 compatible compilers to pitfalls aforementioned.

if (ImScoped::Window("name")) {
    // codes ...
}
sethk commented 5 years ago

@edsDev Apparently if you do this, the compiler still considers the call to operator bool() as consuming the result of the initialization. See oberrich's comment above: https://github.com/ocornut/imgui/issues/2096#issuecomment-434303510

oberrich commented 5 years ago

@edsDev You would need a nodiscard on the constructor which isn't possible unfortunately. Alternatively you would have to make functions returning an RAII wrapper and mark them nodiscard but iirc there are problems with that as well (probably what @sethk is referring to)

gotocoffee1 commented 5 years ago

i find this macro (i know we all hate macros ;) ) quite useful:

#define CONCAT_(x,y) x##y
#define CONCAT(x,y) CONCAT_(x,y)
#define GUI(x, ...) ImScoped::x CONCAT(_gui_element, __COUNTER__) {__VA_ARGS__}

if (GUI(Window, "Test Window"))
{ ... }
kfsone commented 3 years ago

I've done something like this in my own CMake/C++ bindings for ImGui:

#include "imguiwrap.dear.h"

void windowFn()
{
  dear::Begin{"My Window"} && [] {
    dear::TabBar{"##TabBar"} && [] {
      dear::TabItem{"About"} && [] {
        ImGui::Text("About what?");
      };
      dear::TabItem("Help"} && [] {
        ImGui::Text("Abandon all hope");
      };
    };
  };
}

for the cmake part, I've made it trivial to introduce imgui to a new project using FetchContent or CPM; I configure the targets properly so that as long as you do target_link_libraries(myprogram PUBLIC imguiwrapper) you'll have everything you need configured.

https://github.com/kfsone/imguiwrap

I'm still adding wrappers, and mac/linux builds will be next.

ocornut commented 3 years ago

FYI dear::Begin{"My Window"} && [] { This is currently not the right way to use Begin() (it is known to be inconsistent with other API, we have the fix but it's a rather painful transition tho we have helpers for it)

kfsone commented 3 years ago

That's one of the things I wanted to encompass in the abstraction; imguiwrap handles Begin and BeginChild correctly (it will always call End, as opposed to say MenuBar, which will only call EndMenuBar if BMB returned true), and Group where BeginGroup is group, so it behaves as though BeginGroup returns true.

mnesarco commented 3 years ago

Hi Friends, I like the ideas from @sethk, but the link is broken (https://github.com/sethk/imgui/blob/raii/misc/raii/imgui_raii.h), how can i get it for testing?

maxkunes commented 3 years ago

@mnesarco no clue if this is up to date, but this is mentioned in the readme for imgui/misc/cpp

https://github.com/ocornut/imgui/pull/2197

sethk commented 3 years ago

@mnesarco Try here: https://github.com/sethk/imgui/blob/raii/misc/cpp/imgui_scoped.h

May be a bit out of date because the branch is a few years old now.

mnesarco commented 3 years ago

@sethk Thank you.

mnesarco commented 3 years ago

@sethk @ocornut

Hi Friends, I have learned a lot from this thread, and in the spirit of sharing my approach, i have published my own version :D. It would be great to hear your opinions.

This is not a proposal at all. I am just sharing my personal approach, I know that c++17 is almost banned here :D and macros should be hated to death, but this is just another view for the discussion.

https://github.com/mnesarco/imgui_sugar

Cheers.

mnesarco commented 3 years ago

I have changed imgui_sugar to c++11 :)

TerensTare commented 3 years ago

Good question, I don't have the answer to this unfortunately. Let us think about it .. or I wonder if instead we could opt for a specific prefix to denote the type of functions we are discussing here..

I know I'm some years late, but how about RaiiWindow/ScopedWindow and such? As for the naming issue, I think we can safely write

if (RaiiWindow _("MyWindow")) // notice the _
// blah blah

which at least to me looks acceptable

mnesarco commented 3 years ago

The problem with

if (RaiiWindow _("MyWindow")) // notice the _
// blah blah

Is that if you forgot to write the "_" it will compile without warning but won't work:

if (RaiiWindow ("MyWindow")) // notice the missing _
// blah blah

That is why I prefer the macro defined scope:

with_Window("My Window") {
  // blah blah
}

It is just a syntactic sugar, generates the same code but ensures that the RAII guard is named.

TerensTare commented 3 years ago

The problem with

if (RaiiWindow _("MyWindow")) // notice the _
// blah blah

Is that if you forgot to write the "_" it will compile without warning but won't work:

Right, that's a valid issue, but can be fixed by adding ref-qualifiers to the conversion operator. Please see https://godbolt.org/z/PeGGvqn6G.

The solution you propose is really nice (in syntax and hard-to-misuse terms) but I personally prefer avoiding macros, since the rest of the functions avoid macros.

mnesarco commented 3 years ago

Right, that's a valid issue, but can be fixed by adding ref-qualifiers to the conversion operator. Please see https://godbolt.org/z/PeGGvqn6G.

Yes you are right, I just added ref qualifiers to my guards yesterday ;)

The solution you propose is really nice (in syntax and hard-to-misuse terms) but I personally prefer avoiding macros, since the rest of the functions avoid macros.

Yes it is a matter of user preferences, and i know that macros can be evil, but...

  1. The macros used are very innocent:
#define with_Whatever(...) if (Raii _ = Raii(Whatever(__VA_ARGS__)))

Internally I used other macros just to not repeat the same for every function. So i get a very small (<200 lines of code) header (including license and comments).

  1. The generated code is exactly the same
if (Raii _ = Raii(Whatever(args...))) 
  // blah blah
  1. The syntax is very terse
with_Window("Test") 
{
  with_MenuBar 
  {
    with_Menu("File") 
    {
        with_MenuItem("Edit")
            // blah blah
        with_MenuItem("Save")
           // blah blah
    }
  }
}
  1. I feel that the pros overcomes the cons, but that's my personal opinion of course .

So yes, it is a matter of personal preferences. 👍

kfsone commented 3 years ago

The solution you propose is really nice (in syntax and hard-to-misuse terms) but I personally prefer avoiding macros, since the rest of the functions avoid macros.

Because ImGui is written in pseudo-C, it's very easy to forget how the language you're trying to work in actually works. Unless your constraints preclude the use of more modern C++, you can entirely avoid macros and just rely on good-old C++ object lifetimes by using method-on-a-temporary

https://gcc.godbolt.org/z/1xq737d1Y

and for the scopes, lambdas. This is how I was able to get pure RAII scoping with https://github.com/kfsone/imguiwrap

#include "imguiwrap.dear.h"
#include <array>
#include <string>

ImGuiWrapperReturnType
render_fn()  // opt-in functionality that gets called in a loop.
{
  bool quitting { false };
  dear::Begin("Window 1") && [&quitting](){   // or make quitting a static so capture not required.
    dear::Text("This is window 1");
    dear::Selectable("Click me to quit", &quitting);
  };
  if (quitting)
    return 0;

  dear::Begin("Window 2", nullptr, ImGuiWindowFlags_AlwaysAutoResize) && [](){
    static constexpr size_t boarddim = 3;
    static std::array<std::string, boarddim * boarddim> board { "X", "O", "O", "O", "X", "O", "O", "X", " " };
    dear::Table("0s and Xs", 3, ImGuiTableFlags_Borders) && [](){
      for (const auto& box : board) {
        ImGui::TableNextColumn();
        dear::Text(box);
      }
    };
  };
}

int main(int argc, const char* argv[])
{
    return imgui_main(argv, argv, my_render_fn);
}
GasimGasimzada commented 2 years ago

You can create C++ helpers for doing just that, they would be a few lines to implement. I am open the idea of providing an official .h file with those helpers if they are designed carefully.

Begin/BeginChild are inconsistent with other API for historical reasons unfortunately :(

Is there any information documentation or reference about this? Looking at the imgui_demo, I do not fully understand when the End statements need to be inside the Begin condition scope vs outside of it:

if (ImGui::Begin(...)) {

}

// https://github.com/ocornut/imgui/blob/master/imgui_demo.cpp#L2479
ImGui::End();

if (ImGui::BeginChild(...)) {

}

// https://github.com/ocornut/imgui/blob/master/imgui_demo.cpp#L3107
ImGui::EndChild();

if (ImGui::BeginTable(...)) {

  // https://github.com/ocornut/imgui/blob/master/imgui_demo.cpp#L553
  ImGui::EndTable();
}

if (ImGui::MenuBar(...)) {

  // https://github.com/ocornut/imgui/blob/master/imgui_demo.cpp#L2977
  ImGui::EndMenuBar();
}

if (ImGui::MainMenuBar(...)) {

  // https://github.com/ocornut/imgui/blob/master/imgui_demo.cpp#L6421
  ImGui::EndMainMenuBar();
}

I am thinking of implementing my own RAII implementation and these two implementations matter in terms of when to apply this:

class Window {
public:
   Window(...) { mOpen = ImGui::BeginWindow(...); }

   ~Window() { ImGui::EndWindow(); }
private:
  bool mOpen = false;
};

class MenuBar {
public:
   MenuBar(...) { mOpen = ImGui::BeginMenuBar(...); }

   ~MenuBar() {
     if (mOpen)
       ImGui::EndMenuBar();
    }
private:
  bool mOpen = false;
};
ocornut commented 2 years ago

Begin and BeginChild are the only inconsistent ones. It is all commented in imgui.h and effectively the demo.

mnesarco commented 2 years ago

Please don't implement RAII guards again and again and again.... There are many attempts already. I implemented one of them myself, but not much people use it:

https://github.com/mnesarco/imgui_sugar

#include <imgui/imgui.h>
#include <imgui_sugar.hpp>

// ...

    static int left = 0, right = 0;
    ImGui::SetNextWindowPos(ImVec2(30, 50), ImGuiCond_FirstUseEver);
    set_StyleColor(ImGuiCol_WindowBg, ImVec4{0.88f, 0.88f, 0.88f, 1.0f});        
    set_StyleColor(ImGuiCol_Text, 0xff000000);

    with_Window("Test Window", nullptr, ImGuiWindowFlags_AlwaysAutoResize) {

        ImGui::Text("Hello");

        with_Group {
            ImGui::Text("Left %d", left);
            if (ImGui::Button("Incr Left"))
                ++left;
        }

        ImGui::SameLine();

        with_Group {
            set_StyleColor(ImGuiCol_Text, 0xffff0000);

            ImGui::Text("Right %d", right);

            if (ImGui::Button("Incr Right"))
                ++right;

            with_Child("##scrolling", ImVec2(200, 80)) {

                ImGui::Text("More text ...");
                ImGui::Text("More text ...");
                ImGui::Text("More text ...");

                with_StyleColor(ImGuiCol_Text, ImVec4{ 0, 0.5f, 0, 1.0f })
                    ImGui::Text("More text ...");

                ImGui::Text("More text ...");

                with_StyleColor(ImGuiCol_Text, ImVec4{ 0.5f, 0.0f, 0, 1.0f }) {
                    ImGui::Text("More text ...");
                    ImGui::Text("More text ...");
                }
            }
        }

        ImGui::Text("Bye...");
    }    

// ...

There are different approaches, Lambdas, Macros, ... Use one of the existent projects...

GasimGasimzada commented 2 years ago

I have created my own RAII implementation that I am really happy with. It works in the following way:

if (auto table = Table("MyTable", 3)) {
  // covers 95% of the use-cases for table
  // for me
  table.row("I am a text", glm::vec3(2.5f), 25.0f);

  // Custom implementation
  ImGui::TableNextRow();
  ImGui::TableNextColumn();
  ImGui::Button("I want a button inside this column");
  // ..other cols as well
}

if (auto _ = Window("Window title", open)) {
  ImGui::Text("Inside the window");
}

I may create a Macro around the API to make it cleaner but it is not important to me at the moment and it has already fixed one of the error-prone things for me, which was mismatching ends, especially when you have lots of UI code and it is easy to miss.

sugrob9000 commented 1 year ago

In C++17 (I think), you can get C++ to generate most of the above API for you like so:

namespace ImScoped {
namespace detail {
template <auto Begin, auto End, bool UnconditionalEnd = false> class Widget {
    bool shown;
public:
    explicit Widget (auto&&... a)
        : shown{
            [] (auto&&... aa) {
                return Begin(std::forward<decltype(aa)>(aa)...);
            } (std::forward<decltype(a)>(a)...)
        } {}
    ~Widget () { if (UnconditionalEnd || shown) End(); }
    explicit operator bool () const& { return shown; }
    explicit operator bool () && = delete;
};
} // namespace detail

using Window = detail::Widget<ImGui::Begin, ImGui::End, true>;
using TabBar = detail::Widget<ImGui::BeginTabBar, ImGui::EndTabBar>;
using TabItem = detail::Widget<ImGui::BeginTabItem, ImGui::EndTabItem>;
using Table = detail::Widget<ImGui::BeginTable, ImGui::EndTable>;
// etc.
} // namespace ImScoped

Deleting the rvalue-ref overload of operator bool protects you from accidentally writing

if (ImScoped::Window(...)) // Wrong! the temporary would be destroyed immediately

If you wish to add methods, you can use

struct Table: detail::Widget<ImGui::Begin, ImGui::End> {
    using Widget::Widget;
    void row ();
    // etc.
};

Regular parameter pack forwarding fails when omitting defaulted parameters, but wrapping it another time through a variadic lambda works.

One downside is that, ironically, this approach fails for when the function has actual overloads, because you can't bind the non-type template parameter Begin to an overload set. You can make it compile by disambiguating the overload set with static_cast, but that yet again loses information about defaulted arguments. As far as I can see, only BeginChild is out.