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

OpenPopup() overload with ImGuiID to ease opening modals from deeper stacks. #3993

Closed
wants to merge 2 commits into from

Conversation

zlash
Copy link

@zlash zlash commented Mar 31, 2021

As the title says, it's just a public overload for the OpenPopup() function with an ImGuiID parameter instead of string id.

Until a rework/rethinking of the popups is done, this helps to obtain the popup id at the same id stack level and then pass it down.

@ocornut ocornut added popups label/id and id stack implicit identifiers, pushid(), id stack labels Mar 31, 2021
@ocornut
Copy link
Owner

ocornut commented Mar 31, 2021

Hello,

Thanks for the PR.
Why not directly using OpenPopupEx() for now?

Right now it doesn't make total sense to be using ID based OpenPopup() without the matching BeginPopupEx().

We are contemplating exposing path-based ID stack manipulation, we have the code ready for it but I worry it would be confusing/error prone to only enable it on some functions, and I'm not sure we can afford to enable it on all functions. It consist in supporting three features:

  • One leading / in the resets the ID seed to 0 (so "/hello" is the same ID regardless of UI stack position)
  • One or more leading ../ resets the ID seed to parent, so "../hello" from a PushID scope is same as "hello" outside that scope. (That feature might be extended to support child->parent windows transition, unsure yet)
  • Mid-path / are ignored, use \\/ to encode a slash.

The third feature makes lots of sense for some use (we use it a lot in the dev/automation project) but much less useful for popups and comes with some issues and is the one that's has the most perf impact, so if we omit it we could more easily adopt the top 2 features. The tricky thing is shall we only adopt them for Popup functions.

Link #331

@zlash
Copy link
Author

zlash commented Mar 31, 2021

Thanks for the quick reply!

I added the overload because I thought it would be better style than exposing the currently private OpenPopupEx(), but you are completely right. I didn't realize that pretty much no functions use IDs directly.

Would it be ok to make public OpenPopupEx for now?

I have a tree that is created with a recursive function, and each node has a button that can open a modal. For this use case, generating the id at the same level as the tree and then passing it down seems the best solution. (I could keep a flag around, but minimizing user side state is best, right?)

About the path based ids, it sounds great. Just the first feature, having a global scope for for ids using "/" as leading characters would be super usefull. It falls on the user responsability to avoid global clashes but sounds reasonable. And performance wise would be quite cheap.

@ocornut
Copy link
Owner

ocornut commented Mar 31, 2021

Would it be ok to make public OpenPopupEx for now?

You can forward-declare it anywhere in your code or just include imgui_internal.h in the file using it.

namespace ImGui
{
IMGUI_API void  OpenPopupEx(ImGuiID id, ImGuiPopupFlags popup_flags = ImGuiPopupFlags_None);
}

I have a tree that is created with a recursive function

What's not clear to me is how you are calling BeginPopup then?

@zlash
Copy link
Author

zlash commented Mar 31, 2021

Something Like this:

doTree(ImGui::GetID("Add Node"));
if(ImGui::BeginPopupModal("Add Node", NULL, ImGuiWindowFlags_AlwaysAutoResize)) {
...
}

I added it to the header because I thought it could be useful for other people trying to do the same. Instead of opening an Issue asking I made the PR as it was nearly the same.

I'll keep using imgui_internal.h then.

Feel free to close the PR! Thanks!

ocornut added a commit that referenced this pull request Apr 1, 2021
@ocornut
Copy link
Owner

ocornut commented Apr 1, 2021

Instead of opening an Issue asking I made the PR as it was nearly the same.

You did totally right. Most PR are pointing at a meaningful issue.

I thought about it more and realized that even with the proposed future additions, this will be useful so I merged this now. Thank you!

@ocornut ocornut closed this Apr 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
label/id and id stack implicit identifiers, pushid(), id stack popups
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants