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

Add ImGui widgets #315

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from
Draft

Conversation

Cosine256
Copy link
Contributor

@Cosine256 Cosine256 commented Jul 10, 2023

Exposes some ImGui widgets and features to the Lua API, and changes some existing features:

  • Tooltips
  • Color editors
  • Tab bars
  • Menu bars
  • Child windows
  • Disabled widgets
  • Groups
  • Dummies
  • ImGui size and position calls (partially implemented, needs more work)
  • Added advanced window function with more parameters.
  • Added button size parameters.
  • Added more slider and drag input parameters.
  • Added collapsed parameter to the callback signature for both window functions.
  • Fixed bug where the window collapsed state would persist across sessions, even though position and size were not persisted. Simple windows now always spawn uncollapsed. Advanced windows can still persist the collapsed state with the right condition parameters.

Possible additions (might not add some of these):

  • Text colors
  • Multi-line text inputs

Known issues and concerns:

  • HD cursor slightly overlaps tooltips. OL UI tooltips avoid this by briefly changing the cursor scale for ImGui right before drawing the tooltip. Ideally this cursor scale would be set globally when the HD cursor is enabled, but I haven't looked into whether this would work or cause other problems.
  • Advanced window function is quite complicated, and is even more confusing if you don't understand the underlying ImGui behavior for windows and variable conditions. Most use cases can avoid it and use the simpler variant, but adding something like a menu bar forces you to use the full function.
  • Coordinate systems for ImGui size and position calls are inconsistent with OL/S2 screen coordinates.
  • Width parameters have undocumented and problematic special behavior when 0 < abs(width) < 1. Trying to set an item to 100% width requires a hacky value of 0.999999, because 1.0 will make it 1 pixel wide. Feature behaves poorly due to item padding, causing two 50% items to take up more than 100% width.

@Dregu
Copy link
Collaborator

Dregu commented Sep 29, 2023

I recently struggled a bit making good ui with my window api, so I'm just leaving a note here for someone to implement everything in this thread too. Not a request, just a reminder. You might enjoy #337 for the TAS2L btw.

@Cosine256
Copy link
Contributor Author

It looks like all that's needed for that feature is a BeginGroup/EndGroup call, so I think I can add that without any trouble. If there was anything else from ImGui that you or anybody else wanted, I can add it to my list if it isn't terribly complicated.

My design philosophy for these changes has been to make the GuiDrawContext API closely imitate the ImGui API. Is there any reason not to do it this way? I feel there is no reason to create our own facade for the underlying library. I'm still adding "simple" overloads for people who just want to cobble together a basic GUI using only the OL docs, while power users can look at ImGui docs if they want to fully understand the overloads with ImGui flags and other arguments.

Also, I saw the request for exposing a safe file directory and I definitely appreciate it. It'll be nice to not require unsafe mode for the TAS Tool.

@Dregu
Copy link
Collaborator

Dregu commented Oct 3, 2023

I didn't mean just some way to make a vertical group, I meant every function mentioned in the thread. The big buttons, way to draw more things over a cursorpos saved earlier etc.

Anyway, my idea is also to closely imitate the imgui C api, while not directly exposing any Begins and Ends, cause forgetting one of those would lead to a disaster. But you're already wrapping these to callbacks and know what I'm talking about. I guess some imgui features were just deemed unnecessarily complicated earlier, but hopefully those can still be fixed with overloads for advanced arguments.

@Cosine256
Copy link
Contributor Author

Why are borders disabled on windows even if the OL setting is enabled? It's difficult to read overlapping windows since they seamlessly blend into each other.

style.WindowBorderSize = 0; // meh

@Cosine256
Copy link
Contributor Author

I exposed some of the ImGui size and position calls, including the ones mentioned in that Stack Overflow thread. A problem I'm seeing with them is that they're in either ImGui screen or window coordinates (pixels). You can't easily use them with the GuiDrawContext:draw_* calls if you wanted to do something like draw a circle within the space created by a win_dummy, because those are all using OL screen coordinates. Should those calls convert to OL screen coordinates? Should there be some kind of way for the user to do that conversion themselves? I'm also considering renaming the "cursor" ones, since ImGui's name is confusing and refers to the ImGui output position instead of a mouse cursor.

@Dregu
Copy link
Collaborator

Dregu commented Nov 11, 2023

Why are borders disabled on windows even if the OL setting is enabled?

It's probably just a leftover from before the option was even added, and maybe I was going for no borders just on the OL menus, but disabled the completely wrong one and forgot about it. You can change that line to follow the UI option. Also we should add a safe way to use PushStyleVar and other Push-able styling functions in scripts i.e. automatically popping any stray pushes at the end of the gui context.

Should those calls convert to OL screen coordinates?

Generally yes, but there are probably always cases where pixel format would be useful, like using the window cursor pos in sameline, which uses pixels. Even worse that the imgui coordinates are desktop-based, not window-based like our api. Window coordinates might be fine as pixels?

Should there be some kind of way for the user to do that conversion themselves?

Yes, per previous answer. Adding a pixel variant for every function using screen coordinates surely isn't the way, so exposing converters is the least we can do.

I'm also considering renaming the "cursor"

I agree, but don't know what it would be. Maybe output_position like you said, or even input_position, cause it is used to position input fields usually.

Width parameters have undocumented and problematic special behavior

Yep seems like it's only documented in win_width, where it originated and was copied all over the place. Maybe changing it to <=1 would be better, or is there a need for 1 pixel wide elements too? Surely not sub-pixel wide elements at least. I can see how having to make sure some automatically calculated width is >1 might still be a problem though.

Actually seems like I have tried to fix the padding/width issue already, but only in win_width. That solution is also not pixel-perfect, but ending up with a bit less than 100% width is far better than more than 100%.

I think the best compromise to keep it simple would be to adopt that padding fix in all functions that use this <1 logic and make ==1 use 100%.

@Dregu
Copy link
Collaborator

Dregu commented Nov 11, 2023

I realized after the PushStyleVar comment that Begin stacks can also be counted in the gui ctx, so wrapping all of them in callbacks isn't the only way to make sure they are Ended properly at the end of the window or ctx. I can look into this later.

Also all gui callbacks need to be forwarded the ctx as first parameter (handle_function<...>(backend, callback, this)) in case the callback function is not inline and can't use the upvalue from the GUIFRAME callback. My fault, I forgot it in win_section too, but at least it's in window.

@Dregu
Copy link
Collaborator

Dregu commented Nov 11, 2023

Here's the idea from the previous comment with win_disabled to optionally use callbacks or just BeginDisabled that will be fixed automatically if forgotten by modder. Cosine256/overlunky@add-imgui-widgets...spelunky-fyi:overlunky:GuiStackFix

set_callback(function(ctx)
  ctx:win_disabled(true)
  ctx:window("even the window controls don't work", 0, 0, 0, 0, true, function(ctx2)
    ctx2:win_disabled(true)
    ctx2:win_input_text("twice disabled", "")
    ctx2:win_disabled(false)
  end)

  -- the first disable is still in the stack
  ctx:window("still disabled", 0, 0, 0, 0, true, function()
    ctx:win_input_text("still disabled from outside", "")
    -- this would leak outside the window, but will be fixed in the windows own GuiDrawContext destructor
    ctx:win_disabled(true)
  end)
  ctx:win_disabled(false)

  ctx:window("not disabled", 0, 0, 0, 0, true, function()
    ctx:win_input_text("this should not be disabled", "")
  end)

  -- this would leak outside the script, but will be fixed by the outer GuiDrawContext destructor
  ctx:win_disabled(true)
end, ON.GUIFRAME)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants