Migrate uilist to ImGui #73045
Migrate uilist to ImGui #73045
Conversation
Mostly in pretty good shape. * the loading progress indicators don’t look right * imgui layer doesn’t properly handle a ui_adaptor that is supposed to hide all other ui * a uilist that is reset doesn’t show any options * no second column text is drawn * uilist is now an imgui window with a title, so the "text" of the list has been split into a title and a help text; most just have a title with no help text * maybe other things
…his smaller, we can refactor uilist in a different PR
github-actions
bot
added
Info / User Interface
|
Lol, just yesterday I spent a few hours rebasing and cleaning up my patch; guess I wasn’t quick enough. On a quick skim, the first thing I notice are the commented out lines. A number of lines that used to call I’ll review it more thoroughly in the morning. |
Hey! Sorry, I actually tried to message you on Reddit to see if you were still interested in this work, never heard back. If you wanna keep going with this I'll put it down. |
|
The biggest minefield I am seeing with the work is the callback system, lots of places use the callback to overwrite the window and I have no clue how to handle it |
|
No, don't stop now that you have momentum! I really must sleep, but I will take a closer look in the morning. If I have fixes for anything that you haven’t already done, I’ll send you a PR. |
|
Man, I hadn’t even gotten as far as these callbacks. I think that anything using the callback just needs to do its own thing and call ImGui directly. However, since it seems that they mostly use it to put information to the left or right of the uilist’s list of choices, perhaps we should just put some empty widgets there with known IDs and let the caller fill them in if they want to. This would be less work than completely rewriting the Also, there's still some confusion in the code about ints vs uints: |
|
@db48x i jumped the gun with taking on this migration so soon after ImGui was enabled globally. i will be busy with imgui-related issues for a few weeks probably. if you have some interest you should definitely pick this up |
|
Cool. I made some progress this weekend; I’ll be able to keep all of the callbacks, re–implementing them in terms of ImGui commands. |
|
@db48x looks like you are making steady progress on this work on your own branch so I am going to close this PR unless there's a reason you want it to stay open. |
|
This one may as well stay open. My PR is in your repository, and updates this branch. And I would value your time, if you wanted to run your eye over my changes. I know that there will be bugs, and it would be nice to spot their causes early if we can :D But if you do prefer to close this PR then I can open my own; that’s no great trouble. |
There’s no need for it to be a method of cataimgui::window, since it accesses no state on the window. This will allow callers that are not derived classes.
This shouldn’t change the output except in the case where no wrapping is actually happening. When width was zero, the input was not actually being broken into lines at all, causing confusion later on.
This uilist is supposed to fill the screen, but for some reason it is too short. Also, the title has a color tag in it; probably that should be moved to the help text.
The uilist_impl derives from cataimgui::window, which creates and manages the ui_adaptor for us.
It was purely aesthetic. All it did was tweak the border of the uilist so that it blends more seamlessly with some other UI already on the screen, which is now unnecessary.
Drawing the map needed more work than I expected. It traverses the map in an odd order, causing it to be flipped or rotated 180° when printed naively. Thus both the original and the new implementation for ImGui calculate the position of each character and reposition the cursor for each one.
Now actually measure the menu items to find their width, and size the window reliably to fit without growing larger than the screen. (Will need to be tweaked when I fix the title bar though.)
There are some niggling little bugs left that I am too tired to work out at the moment.
ImGui already has stacks for those. Of course, it's still not working perfectly.
and a bunch of useless commented–out code that we have replaced.
Actually clicking to select an item doesn’t work for unknown reasons, however. It always selects the wrong item.
it must have been brought in by a transitive include
The selected menu item is now updated on hover, and the correct menu item now is activated on click. Really all that was missing was that parent.selected was not being updated.
oops, forgot about it again
github-actions
bot
added
Monsters
|
Once #73538 is merged, I’ll rebase this branch on top of it to clean up the code. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. Please do not bump or comment on this issue unless you are actively working on it. Stale issues, and stale issues that are closed are still considered. |
|
Looks like @db48x opened a new PR for this work so I'm closing this |
Summary
Infrastructure "Migrates the generic uilist class to ImGui"
Purpose of change
Continuing work on top-level UI elements and working my way down, uilist tends to show above everything so is a fair choice to migrate.
Describe the solution
Based on some work by @db48x , I took a bit of a different approach (using a private implementation class rather than having uilist extend from cataimgui::window) but I used all the ImGui/cataimgui calls from that code and moved them into the p_impl class. This allows us to interface with the p_impl just like a ui_adaptor
Describe alternatives you've considered
could have kept the original approach, but IMHO I really prefer to not include imgui-related headers from other headers, this is mostly based on my own pet peeve.
Testing
Load inventory UI, examine an item, see that the uilist draws properly. similar thing from the main menu -> load menu, see that uilist draws properly.
Additional context
Old:
New: