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

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

Open
sethk opened this issue Sep 21, 2018 · 61 comments

Comments

@sethk
Copy link

commented Sep 21, 2018

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.

@ocornut

This comment has been minimized.

Copy link
Owner

commented Sep 21, 2018

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 :(

@sethk

This comment has been minimized.

Copy link
Author

commented Sep 21, 2018

I think I'll take a crack at it and post something here for people to critique.

@maxkunes

This comment has been minimized.

Copy link

commented Sep 23, 2018

Something I put together a while back, only handles PushStyleVar and PushStyleColor, would probably be nice to improve it to handle other ImGui push/pop methods.

Header (ImStyle_RAII.h):

#pragma once
#include "imgui.h"

class ImStyle_RAII {
public:

	ImStyle_RAII(ImGuiStyleVar idx, const ImVec2& val);

	ImStyle_RAII(ImGuiStyleVar idx, const float& val);

	ImStyle_RAII(ImGuiCol idx, const ImU32& col);

	~ImStyle_RAII();

private:
	bool bClr;
};

Source (ImStyle_RAII.cpp):

#include "ImStyle_RAII.h"

ImStyle_RAII::ImStyle_RAII(ImGuiStyleVar idx, const ImVec2& val)
{
	bClr = false;
	ImGui::PushStyleVar(idx, val);
}

ImStyle_RAII::ImStyle_RAII(ImGuiStyleVar idx, const float& val)
{
	bClr = false;
	ImGui::PushStyleVar(idx, val);
}

ImStyle_RAII::ImStyle_RAII(ImGuiCol idx, const ImU32 & col)
{
	bClr = true;
	ImGui::PushStyleColor(idx, col);
}

ImStyle_RAII::~ImStyle_RAII()
{
	if (bClr)
		ImGui::PopStyleColor();
	else
		ImGui::PopStyleVar();
}

@meshula

This comment has been minimized.

Copy link

commented Sep 23, 2018

My most used RAII object for Imgui:

class SetFont
{
public:
	SetFont(ImFont* f) { ImGui::PushFont(f); }
	~SetFont() { ImGui::PopFont(); }
};
@sethk

This comment has been minimized.

Copy link
Author

commented Sep 24, 2018

Looks like there's some demand for this! Here's what I'm working with so far:

The only part I can imagine being controversial is that I'm providing operator bool() so that you can say for instance ImWindow window("Blah"); if (window) ....

#include "imgui.h"

#pragma once

class ImWindow
{
public:
    bool IsOpen;

    ImWindow(const char* name, bool* p_open = NULL, ImGuiWindowFlags flags = 0) { IsOpen = ImGui::Begin(name, p_open, flags); }
    ~ImWindow() { if (IsOpen) ImGui::End(); }

    operator bool() { return IsOpen; }
};

class ImPushID
{
public:
    ImPushID(const char* str_id) { ImGui::PushID(str_id); }
    ImPushID(const char* str_id_begin, const char* str_id_end) { ImGui::PushID(str_id_begin, str_id_end); }
    ImPushID(const void* ptr_id) { ImGui::PushID(ptr_id); }
    ImPushID(int int_id) { ImGui::PushID(int_id); }
    ~ImPushID() { ImGui::PopID(); }
};

class ImTreeNode
{
public:
    bool IsOpen;

    ImTreeNode(const char* label) { IsOpen = ImGui::TreeNode(label); }
    ImTreeNode(const char* str_id, const char* fmt, ...) IM_FMTARGS(3) { va_list ap; va_start(ap, fmt); IsOpen = ImGui::TreeNodeV(str_id, fmt, ap); va_end(ap); }
    ImTreeNode(const void* ptr_id, const char* fmt, ...) IM_FMTARGS(3) { va_list ap; va_start(ap, fmt); IsOpen = ImGui::TreeNodeV(ptr_id, fmt, ap); va_end(ap); }
    ~ImTreeNode() { if (IsOpen) ImGui::TreePop(); }

    operator bool() { return IsOpen; }
};

class ImTreeNodeV
{
public:
    bool IsOpen;

    ImTreeNodeV(const char* str_id, const char* fmt, va_list args) IM_FMTLIST(3) { IsOpen = ImGui::TreeNodeV(str_id, fmt, args); }
    ImTreeNodeV(const void* ptr_id, const char* fmt, va_list args) IM_FMTLIST(3) { IsOpen = ImGui::TreeNodeV(ptr_id, fmt, args); }
    ~ImTreeNodeV() { if (IsOpen) ImGui::TreePop(); }

    operator bool() { return IsOpen; }
};

class ImTreeNodeEx
{
public:
    bool IsOpen;

    ImTreeNodeEx(const char* label, ImGuiTreeNodeFlags flags = 0) { IsOpen = ImGui::TreeNodeEx(label, flags); }
    ImTreeNodeEx(const char* str_id, ImGuiTreeNodeFlags flags, const char* fmt, ...) IM_FMTARGS(4) { va_list ap; va_start(ap, fmt); IsOpen = ImGui::TreeNodeExV(str_id, flags, fmt, ap); va_end(ap); }
    ImTreeNodeEx(const void* ptr_id, ImGuiTreeNodeFlags flags, const char* fmt, ...) IM_FMTARGS(4) { va_list ap; va_start(ap, fmt); IsOpen = ImGui::TreeNodeExV(ptr_id, flags, fmt, ap); va_end(ap); }
    ~ImTreeNodeEx() { if (IsOpen) ImGui::TreePop(); }

    operator bool() { return IsOpen; }
};

class ImTreeNodeExV
{
public:
    bool IsOpen;

    ImTreeNodeExV(const char* str_id, ImGuiTreeNodeFlags flags, const char* fmt, va_list args) IM_FMTLIST(4) { IsOpen = ImGui::TreeNodeExV(str_id, flags, fmt, args); }
    ImTreeNodeExV(const void* ptr_id, ImGuiTreeNodeFlags flags, const char* fmt, va_list args) IM_FMTLIST(4) { IsOpen = ImGui::TreeNodeExV(ptr_id, flags, fmt, args); }
    ~ImTreeNodeExV() { if (IsOpen) ImGui::TreePop(); }

    operator bool() { return IsOpen; }
};
@maxkunes

This comment has been minimized.

Copy link

commented Sep 24, 2018

@sethk Interesting. I have wrapped ImGui children and windows in a similar fashion but without RAII. I'm currently using lamda's for this use.

ImWindowHelper and ImChildHelper implicitly calls begin/end accordingly.

Ex Pseudo :

ImWindowHelper(str_id ..., [&]() {

    ImChildHelper(str_id, size, ..., [&]() {
        ImGui::Text("Child 1");
    });

    ImChildHelper(str_id, size, ..., [&]() {
        ImGui::Text("Child 2");
    });

});

@ice1000

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2018

Consider

{
  ImWindowHelper(str_id, ...);
  {
    ImChildHelper(str_id, ...);
    ImGui::Text("Child 1");
  }
}

which is less noisy.

@maxkunes

This comment has been minimized.

Copy link

commented Sep 24, 2018

@ice1000
Are the destructors garenteed to happen after that call to Text? I'm quite sure you would need to store those raii instances in an actual variable for that code to work correctly. Not sure the standard spec on that. That is the reason I chose the lamda method as I don't need to worry much about scope and how the compiler may treat the code.

@ice1000

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2018

It works with clang++-6.0 but I didn't lookup the standard spec.

@ice1000

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2018

Confirmed: if you don't throw exceptions in the constructor, it's destructed at the time as we expected in my code snippet above.

From https://en.cppreference.com/w/cpp/language/destructor :

The destructor is called whenever an object's lifetime ends, which includes

end of scope, for objects with automatic storage duration and for temporaries whose life was extended by binding to a reference

From http://eel.is/c++draft/class.dtor#:destructor,implicit_call :

A destructor is invoked implicitly

for a constructed object with automatic storage duration ([basic.stc.auto]) when the block in which an object is created exits ([stmt.dcl]),

@maxkunes

This comment has been minimized.

Copy link

commented Sep 25, 2018

Confirmed: if you don't throw exceptions in the constructor, it's destructed at the time as we expected in my code snippet above.

From https://en.cppreference.com/w/cpp/language/destructor :

The destructor is called whenever an object's lifetime ends, which includes
end of scope, for objects with automatic storage duration and for temporaries whose life was extended by binding to a reference

From http://eel.is/c++draft/class.dtor#:destructor,implicit_call :

A destructor is invoked implicitly
for a constructed object with automatic storage duration ([basic.stc.auto]) when the block in which an object is created exits ([stmt.dcl]),

@ice1000 MSVC doesn't like what your doing which suggests to me the standard isn't actually saying
what you think it is, that is unless I'm making a mistake or MSVC isn't following the standard although I believe the former is more likely.

{
		ImWindowHelper("BaseMenu", NULL, ImGuiWindowFlags_NoTitleBar | ImGuiWindowFlags_NoResize | ImGuiWindowFlags_NoSavedSettings);

		BeginChildLamda("Menu", menuSize, false, false, [&]() {
			renderTabs();
			renderContent();
			renderBanner();
		});

}

Relevant ImWindowHelper class

ImWindowHelper::ImWindowHelper(const char* name, bool* p_open, ImGuiWindowFlags flags)
{
	ImGui::Begin(name, p_open, flags);
}

ImWindowHelper::~ImWindowHelper()
{
	ImGui::End();
}

And it compiles on MSVC v141 to this as I suspected :

ImWindowHelper::ImWindowHelper((ImWindowHelper *)&_This.tabWidthPercentage + 3, "BaseMenu", 0, 259);
  ImWindowHelper::~ImWindowHelper((ImWindowHelper *)&_This.tabWidthPercentage + 3);

Essentially it gets instantly destructed after it gets constructed, simply doing this :

{
		auto baseMenuWin = ImWindowHelper("BaseMenu", NULL, ImGuiWindowFlags_NoTitleBar | ImGuiWindowFlags_NoResize | ImGuiWindowFlags_NoSavedSettings);

		BeginChildLamda("Menu", menuSize, false, false, [&]() {
			renderTabs();
			renderContent();
			renderBanner();
		});
//baseMenuWin will be destructed here.
}

Would work as expected and would be destructed where you want it to be. This is precisely why I chose lamdas, because you can definately ensure what code gets called when.

@sethk

This comment has been minimized.

Copy link
Author

commented Sep 25, 2018

@ice1000 That syntax reminds me of something I saw in a Facebook C++ talk about some very insidious bugs they had when RAII initializations were missing variable names and thus turned into side-effect free declarations. The compactness is nice if it works, but it loses the ability to bypass code when windows are collapsed or clipped, etc. In any case, that syntax should be possible with my wrappers, if it works for you. What C++ standard are you targetting with Clang?

@maxkunes I like the idea of lambdas as well. Are there any drawbacks? I haven't used C++ lambdas much. Is there any runtime overhead when they're used as coroutines like this?

@maxkunes

This comment has been minimized.

Copy link

commented Sep 25, 2018

@sethk Regarding the question about RAII you referenced @ice1000 I would direct you to my post above yours where I show that at the very least, MSVC will not work as he expects, my guess is as good as yours if it works other places.

EDIT: Through more research from https://godbolt.org/ seems many compilers will call the destructor instantly after constructor when not allocated locally.

Regarding lamdas, to my knowledge, they don't have many drawbacks, but I heard somewhere that std::function has a little bit of overhead (should fact check that), but for example, if you use function pointers with lamdas, I don't think there is any overhead.

@meshula

This comment has been minimized.

Copy link

commented Sep 26, 2018

class RAII {
public:
    RAII() { printf("A");}
    ~RAII() { printf("~A");}
};
class RAII2 {
    public:
    RAII2() { printf("B");}
    ~RAII2() { printf("~B");}
};

void bad() { RAII(); RAII2(); } // destruct instantly because scope is itself
void good() { RAII a; RAII2 b; } // destruct according to containing scope
@sethk

This comment has been minimized.

Copy link
Author

commented Oct 5, 2018

Just to illustrate my point, I totally forgot that ImGui::End() needs to be called regardless of the return value of ImGui::Begin(), because that's how you handle the case where the window is open but collapsed. Anyway, if you want to play along at home I'll be gradually updating this header in this branch as I test it:

https://github.com/sethk/imgui/blob/raii/misc/raii/imgui_raii.h

@ocornut

This comment has been minimized.

Copy link
Owner

commented Oct 5, 2018

@sethk Looking good!

Some pieces of feedback:

  • Because some people would want to avoid <string> but may want to use this, this should probably be kept in a separate file than the existing imgui_stl.h. I will rename the existing folder to misc/cpp to prepare for the possibility to adding more files in there.
  • IsExpanded should be IsContentsVisible to be more accurate and explanatory.
  • Even though it is practical, it is a incorrect using the Im prefix instead of ImGui here. In the codebase, Im is reserved for things that don't sit on top of ImGui functions or context.
  • Since everything in your class is public, you could use struct and this is what we do everywhere in imgui codebase (I also like it because it challenges C++ dogmas a little :)

ocornut added a commit that referenced this issue Oct 12, 2018

Renamed misc/stl/imgui_stl.h,.cpp to misc/cpp/imgui_stdlib.h in previ…
…sion for other files.(#2035, #2096)

Added misc/README file.
@jdumas

This comment has been minimized.

Copy link
Contributor

commented Oct 12, 2018

This looks good! Another minor comment I have is: we should probably remove the copy/move constructors of those classes, even though it's unlikely someone will make the mistake of copying those guys. A simple macro can facilitate the job for that:

#define IMGUI_DELETE_MOVE_COPY(Base)     \
    Base(Base&&) = delete;                 \
    Base& operator=(Base&&) = delete;      \
    Base(const Base&) = delete;            \
    Base& operator=(const Base&) = delete; \

(If you want to stay pre-C++11 you can declare them private and not define them instead of deleting them.)

@ocornut

This comment has been minimized.

Copy link
Owner

commented Oct 12, 2018

If you want to stay pre-C++11

I'm happy with enforcing C++11 as a requirement for those C++ extension.
(In fact, I don't rule out doing the same for core imgui maybe next year.. will see.)

@sethk

This comment has been minimized.

Copy link
Author

commented Oct 18, 2018

  • IsExpanded should be IsContentsVisible to be more accurate and explanatory.

How about IsContentVisible? I'm only objecting to the grammar...

  • Even though it is practical, it is a incorrect using the Im prefix instead of ImGui here. In the codebase, Im is reserved for things that don't sit on top of ImGui functions or context.

Sounds good, but there is already an ImGuiWindow struct in the global namespace due to imgui_internal.h. What name should I use for a Begin()/End() wrapper?

@oberrich

This comment has been minimized.

Copy link

commented Oct 30, 2018

I could imagine using std::unique_ptr to create so-called finally-actions rather than writing a class for each function pair. It makes it a lot more trivial to implement the individual guards.

See this proof of concept for reference

// Original 'finally' function from https://softwareengineering.stackexchange.com/a/308747
template<typename F>
[[nodiscard]] auto im_finally(F f, bool active = true) noexcept(noexcept(F(std::move(f)))) {
  auto x = [f = std::move(f)](void*){ f(); };
  return std::unique_ptr<void, decltype(x)>((void*)(active), std::move(x));
}

[[nodiscard]] inline auto im_window(char const *name, bool *open = nullptr, ImGuiWindowFlags flags = 0) {
  return im_finally([]{ ImGui::End(); }, ImGui::Begin(name, open, flags));
}

// Usage

  if (auto window = im_window("cpp_im_gui", ...)) {

    // add buttons or whatever

  } // ImGui::Begin == true -> unique_ptr.get() != nullptr -> call deleter
    // ImGui::Begin == false -> unique_ptr.get() == nullptr -> dont call deleter

Here is a working demo of the concept https://ideone.com/KDJz25.
More elaborate tests here https://godbolt.org/z/KPcgP8

@ocornut I would love to hear your thoughts on this approach

@meshula

This comment has been minimized.

Copy link

commented Oct 30, 2018

@obermayrrichard Using unique_ptr like that is clever! For reference, I use the finally in Microsoft's GSL implementation - https://github.com/Microsoft/GSL/blob/master/include/gsl/gsl_util

@ocornut

This comment has been minimized.

Copy link
Owner

commented Oct 30, 2018

@sethk Apologies for later answer.

How about IsContentVisible? I'm only objecting to the grammar...

Sure.

Sounds good, but there is already an ImGuiWindow struct in the global namespace due to imgui_internal.h. What name should I use for a Begin()/End() wrapper?

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..

@obermayrrichard tbh I find this solution really unnecessary and overkill - harder to understand, harder to debug/step into, probably slower in non-optimized builds, it raises the user C++ minimum proficiency requirements, drag unnecessary dependencies; and none of those things are in the spirit of dear imgui. I see no value in it considering the solution proposed initially is so simple and obvious to write and maintain.

@ratchetfreak

This comment has been minimized.

Copy link

commented Oct 30, 2018

And the solution breaks down a bit if you do:

auto win1 = im_window("cpp_im_gui", ...);
if(win1){

}

//win1 isn't closed at this point
auto win2 = im_window("cpp_im_gui", ...);
if(win2){

}

because win1 doesn't get destroyed before win2 gets created.

Also if the user doesn't create the variable it gets immediately destroyed after the condition is checked with no warning.

if(im_window("cpp_im_gui", ...)){
    //window is already closed
}

IOW a bit too fragile in normal use for my tastes.

@jdumas

This comment has been minimized.

Copy link
Contributor

commented Oct 30, 2018

But that's an issue with both RAII and the unique_ptr no? To be honest I have no idea how this unique_ptr solution works, and I've been coding in C++ for 10 years...

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..

How about Impp, or ImGuipp or something like this, to denote the C++ side of it. Or maybe ImStdlib like the filename? Or use its own namespace to be more C++y?

@ratchetfreak

This comment has been minimized.

Copy link

commented Oct 30, 2018

Actually it's more similar how a scope_guard works for a mutex. You need to create a local variable for the guard and make sure it goes out of scope at the correct time.

With std::unique_ptr it's less of an issue because the unique_ptr itself is your handle to the resource it guards so you are not going to forget making a variable for it.

@oberrich

This comment has been minimized.

Copy link

commented Oct 30, 2018

Writing a scope-guard class and using a finally action like this is roughly equivalent, imo it's just less code and less painful to implement the individual.

Both writing an RAII class and finally have the same drawbacks in terms of pitfalls users can run into.
They are both RAII classes in the end, the one being written manually, the other one taking unique_ptrs existing code, abusing the internal pointer as our bool variable and moving the deleter into it which gets called by its dtor if ptr/our bool is zero.

I don't see any obvious way of how we could prevent users doing if (scope_guard{}).
I have tried doing [[nodiscard]] operator bool() {...}.
The only way to explicitly disallow this sort of behavior I can think of is removing the bool conversion operator altogether.

@jdumas

This comment has been minimized.

Copy link
Contributor

commented Oct 30, 2018

Well if it's the same thing then why not stick to the version with no template porn?

@oberrich

This comment has been minimized.

Copy link

commented Oct 30, 2018

Well if it's the same thing then why not stick to the version with no template porn?

As I said previously

imo it's just less code and less painful to implement the individual [function pairs].

In the end it comes down to preference. I don't really care which version will be implemented since I'm not actively involved in imgui's development.
I don't think it's worth discussing my proposal any further since it will only end in a pointless discussion like tabs vs. spaces.

@ocornut ocornut added the cplusplus label Nov 9, 2018

@oberrich

This comment has been minimized.

Copy link

commented Nov 9, 2018

With the deleted copy ctor and assignment ops the constructors should behave correctly, right. I wrote the response from my phone so I didn't bother to read through the entire source. My mistake. Using explicit constructors does however make the error messages when trying to construct via implicit conversion a lot clearer and more understandable imo.

As for the bool: "explicit operator boolean() const" is just the recommended signature for what we want: contextually convertable to bool. Also const correctness is important, at the very least the operator should have const at the end.

Explicitally deleting the move constructor shouldn't cause any bugs but it bloats the file quote a lot. This one surely is purely a matter of taste. It should be apparent to anyone who cares to look at the ctors and assignment operators that a deleted copy ctor implies a deleted move ctor however. Normal users who lack a more complete understanding of C++ will ignore them and live happily ever after anyways^^.

@jdumas

This comment has been minimized.

Copy link
Contributor

commented Nov 9, 2018

Explicitally deleting the move constructor shouldn't cause any bugs but it bloats the file quote a lot.

Yeah, that's why we should use a macro like I suggested earlier in the thread. I personally prefer to avoid relying on arcane knowledge of C++ to infer what is implicitly defined or not in my code, but that's just my opinion.

As for the bool: "explicit operator boolean() const" is just the recommended signature for what we want: contextually convertable to bool.

Agreed for the const correctness. But do you have any example of a bad behavior that could happen without the explicit keyword? E.g. it means you cannot write ImScoped::Window foo; foo() + 1 ?

@oberrich

This comment has been minimized.

Copy link

commented Nov 9, 2018

@jdumas without explicit:

bool b = window{}; // allowed, bad/might lead to bugs
window wnd{};
bool c = wnd; // allowed, useful

with explicit

bool b = window{}; // disallowed, good
window wnd{};
bool c = wnd; // disallowed, neutral/taste
bool d = (bool)wnd; // allowed, good

for both versions this works as well

imwindow window{};
if (window) {
  //...
}

The explicit version is a bit more safer imo. Depends on what behavior you consider more useful.
edit Also the "Safe Bool Problem" might be an issue to consider here (https://i.imgur.com/LQrHzIg.png). Disclaimer: I haven't thought About this longer than a few seconds, I might be completely wrong on this one. I will investigate further when I'm home again on Sunday.

@jdumas

This comment has been minimized.

Copy link
Contributor

commented Nov 9, 2018

Ok, interesting!

@sethk

This comment has been minimized.

Copy link
Author

commented Nov 14, 2018

@ocornut Turns out I wasn't extracting methods without arguments, so I missed a few calls: Group, MainMenuBar, MenuBar, Tooltip, DragDropTarget. Those are useful to wrap, right?

With regard to IsItemXXX(), won't this work?:

ImScoped::TreeNode node(...);
bool is_hovered = ImGui::IsItemHovered();
if (node.IsOpen) // or just if (node)
{
   ...
}

Is it confusing because we're capturing the IsOpen state but other IsXXX state isn't available?

I will make a pull request. It would be great to have more testing and suggestions.

@ocornut

This comment has been minimized.

Copy link
Owner

commented Nov 14, 2018

Those are useful to wrap, right?

Yes.

With regard to IsItemXXX(), won't this work?:

This seems to work but then if you use it this way, how do you handle multiple tree nodes? If they are defined in the same scope they need destruction at the right time, so users needs to declare only 1 tree node per-scope and explicitly add braces, which feels like it is defeating a little the purpose of this idea.
It makes a little unclear how exactly those wrapper would be used in real code, perhaps you could try converting a few meaningful chunks of the demo so see how it would work?

Is it confusing because we're capturing the IsOpen state but other IsXXX state isn't available?

This looks right, because this state is the single one returned from the low-level function.
We could softly mark IsOpen or equivalent as "[internal]" in the comment to emphasis that the bool test should be used.

I will make a pull request. It would be great to have more testing and suggestions.

Yes. For me the most important aspect of merging such PR would be: will you or someone be willing to maintain it, do you use dear imgui regularly and does your project allows you to keep up to date (both those factors would increase the likehood of maintenance). It is a bit chicken and egg-ey, but the more I have evidence of people actively using it (you included) the more I'll be comfortable with merging because I'll know issues will be reported. So I would say it is best to make the PR to incentive users but don't expect an immediate merge.

(I forgot to link to #1651 which is not talking about the same feature at all, but emerge from a similar need of facilitating the handling of stacked elements. To return back to the top-most post, the fact that Begin/BeginChild are inconsistent is a legacy issue that is confusing for users but also a hurdle to implement some features correctly. I'm expecting a major release e.g. 1.70/1.80/2.00 may change this and/or provide a configuration flag to allow the user to change the global behavior to smooth out the transition. It's mostly up to us to provide nicer error reporting and recovery)

@sethk

This comment has been minimized.

Copy link
Author

commented Nov 14, 2018

edit 2 Your single argument constructors should be marked explicit, I see a lot of likely scenarios for bugs there.

@obermayrrichard Is there a situation currently where this can happen? This would be if somebody created a method that took an ImScoped::XXX instance, and then passed a value with the single argument type instead? This seems like it would still lead to the desired behavior, right?

@oberrich

This comment has been minimized.

Copy link

commented Nov 14, 2018

@sethk From what I see there shouldn't be any issues with the ctors even if the copy ctor/assign wasn't deleted.
The worst that could happen is the class being able to get constructed from the parameters type. Best to just write some tests and see for yourself.

@sethk

This comment has been minimized.

Copy link
Author

commented Nov 17, 2018

This seems to work but then if you use it this way, how do you handle multiple tree nodes? If they are defined in the same scope they need destruction at the right time, so users needs to declare only 1 tree node per-scope and explicitly add braces, which feels like it is defeating a little the purpose of this idea.

Yes, you definitely need a scope per tree node, but as @obermayrrichard showed above, you can use a braced initializer to both create and test a scoped wrapper at the beginning of a block (this is definitely new to me). Here are some tree nodes from the demo app:

        if (ImGui::TreeNode("Configuration##2"))
        {
            ImGui::CheckboxFlags("io.ConfigFlags: NavEnableKeyboard", (unsigned int *)&io.ConfigFlags, ImGuiConfigFlags_NavEnableKeyboard);
            ImGui::CheckboxFlags("io.ConfigFlags: NavEnableGamepad", (unsigned int *)&io.ConfigFlags, ImGuiConfigFlags_NavEnableGamepad);
            ...
            ImGui::TreePop();
            ImGui::Separator();
        }

        if (ImGui::TreeNode("Backend Flags"))
        {
            ImGuiBackendFlags backend_flags = io.BackendFlags; // Make a local copy to avoid modifying the back-end flags.
            ImGui::CheckboxFlags("io.BackendFlags: HasGamepad", (unsigned int *)&backend_flags, ImGuiBackendFlags_HasGamepad);
            ...
            ImGui::TreePop();
            ImGui::Separator();
        }

        if (ImGui::TreeNode("Style"))
        {
            ImGui::ShowStyleEditor();
            ImGui::TreePop();
            ImGui::Separator();
        }

And here's what it looks like with my wrappers:

        if (ImScoped::TreeNode node{"Configuration##2"})
        {
            ImGui::CheckboxFlags("io.ConfigFlags: NavEnableKeyboard", (unsigned int *)&io.ConfigFlags, ImGuiConfigFlags_NavEnableKeyboard);
            ImGui::CheckboxFlags("io.ConfigFlags: NavEnableGamepad", (unsigned int *)&io.ConfigFlags, ImGuiConfigFlags_NavEnableGamepad);
            ...
            ImGui::Separator();
        }

        if (ImScoped::TreeNode node{"Backend Flags"})
        {
            ImGuiBackendFlags backend_flags = io.BackendFlags; // Make a local copy to avoid modifying the back-end flags.
            ImGui::CheckboxFlags("io.BackendFlags: HasGamepad", (unsigned int *)&backend_flags, ImGuiBackendFlags_HasGamepad);
            ...
            ImGui::Separator();
        }

        if (ImScoped::TreeNode node{"Style"})
        {
            ImGui::ShowStyleEditor();
            ImGui::Separator();
        }

This works as expected. The one huge drawback is that you have to name your wrapper objects. Doing this:

        if (ImScoped::TreeNode{"Configuration##2"})
        {

compiles, but pops the tree before the block is executed. That's a pretty annoying pitfall, although possibly a little easier to diagnose than an unmatched call to TreeNode() because it doesn't crash, and you can visually see where things aren't being pushed.

Is it confusing because we're capturing the IsOpen state but other IsXXX state isn't available?

This looks right, because this state is the single one returned from the low-level function.
We could softly mark IsOpen or equivalent as "[internal]" in the comment to emphasis that the bool test should be used.

Yes, or we could use classes and mark the instance variables private. =) I think I see your more general point though, that it's confusing how some state is captured by the return value of the Begin|Push|Tree functions, but you have to call context-dependent functions like IsItemXXX to access other state.

I will make a pull request. It would be great to have more testing and suggestions.

Yes. For me the most important aspect of merging such PR would be: will you or someone be willing to maintain it, do you use dear imgui regularly and does your project allows you to keep up to date (both those factors would increase the likehood of maintenance). It is a bit chicken and egg-ey, but the more I have evidence of people actively using it (you included) the more I'll be comfortable with merging because I'll know issues will be reported. So I would say it is best to make the PR to incentive users but don't expect an immediate merge.

Yeah, that makes sense. I have used Dear ImGui for several small projects recently, but I am not the type of person to update my open source dependencies unless there are strong incentives to do so. I also am in between jobs (trying to switch to realtime graphics), and it's not clear whether what I land on will allow me to use ImGui professionally, so I can't say what my personal commitment will be.

What I would like to see are these wrappers being used by the people who like me would naturally feel the need to make their own versions as shorthand, and possibly having that influence the design of future ImGui APIs. From that perspective, it's not too important that it's merged quickly or in its current form.

@jdumas

This comment has been minimized.

Copy link
Contributor

commented Nov 17, 2018

Regarding unnamed variables, in this SO thread they mention an interesting trick with a macro that expands into a static_assert when using TreeNode(x), but call the correct constructor when using TreeNode foo(x).

@sethk

This comment has been minimized.

Copy link
Author

commented Nov 27, 2018

@jdumas That is a neat trick. It's a shame it probably won't work with brace initializers, such that the form of initializing a wrapper inside a conditional wouldn't trigger it.

@Person-93

This comment has been minimized.

Copy link

commented Jan 9, 2019

I was actually thinking about this all day yesterday. I didn't get around to seeing if someone else had written about it until now.

There were a couple of points of concern that I had, some of them I came up with workarounds some I didn't.

  • When conditionally applying a style, if the style object was made in an if block, it would go out of scope at the end of the block and it wouldn't be applied. I though of two possible solutions.

    1. Overload the constructor for the style object so that the first argument can be a boolean.
    2. Allow creating an empty style object and adding styles to it later.
      I like the second one better because it allows multiple stylings to be in one object, so they can be easily grouped.
  • The pop style method pops off the last style, that would not necessarily be the one that the style object was crated for.

@Person-93

This comment has been minimized.

Copy link

commented Jan 9, 2019

@jdumas

This looks good! Another minor comment I have is: we should probably remove the copy/move constructors of those classes, even though it's unlikely someone will make the mistake of copying those guys. A simple macro can facilitate the job for that:

#define IMGUI_DELETE_MOVE_COPY(Base)     \
    Base(Base&&) = delete;                 \
    Base& operator=(Base&&) = delete;      \
    Base(const Base&) = delete;            \
    Base& operator=(const Base&) = delete; \

(If you want to stay pre-C++11 you can declare them private and not define them instead of deleting them.)

I think that deleting the copy constructor is a good idea. It doesn't make sense to apply the style twice. But I think the move constructor should stay. If there is a particular style or set of styles that I make very often, I might have them returned from a method. If there is no copy or move constructor, that would not work. Move-assign can be useful as well. It can pop whatever style is already there, and then the destructor would pop whatver style it's replaced with.

@jdumas

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

Well but you can only push a single style var at a time, even with this RAII stuff. If you want to wrap up a custom style, you might as well create your own RAII struct that push and pop whatever combination of styles you like no? Call me old school for not willing to deal with all this move/assign business, but I think keeping it would make implementation more complex and error-prone. E.g. even if you move an object, the destructor of the moved object will be called no? So you'd need to keep a flag to make sure not to pop() if it has been moved, make sure you don't move it twice, etc.

@Person-93

This comment has been minimized.

Copy link

commented Jan 9, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

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

This comment has been minimized.

Copy link

commented Jan 9, 2019

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

This comment has been minimized.

Copy link
Author

commented Jan 13, 2019

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

This comment has been minimized.

Copy link

commented May 18, 2019

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

This comment has been minimized.

Copy link
Author

commented May 19, 2019

@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

This comment has been minimized.

Copy link

commented May 20, 2019

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

@edsDev

This comment has been minimized.

Copy link

commented May 25, 2019

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

This comment has been minimized.

Copy link
Author

commented May 25, 2019

@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: #2096 (comment)

@oberrich

This comment has been minimized.

Copy link

commented May 25, 2019

@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

This comment has been minimized.

Copy link

commented Jul 5, 2019

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"))
{ ... }
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.