-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Implemented new UI framework backed by ImGui, ported keybindings screen, popup #65709
Implemented new UI framework backed by ImGui, ported keybindings screen, popup #65709
Conversation
This looks promising, thanks for taking the effort to do it! Could you share some screenshots of the new UIs? Also, does this allow windows and text in the SDL build to have more fine-grained positioning not restricted to a terminal grid? |
You've gotten the "Summary" section wrong. The tools demand an exact adherence to the format described in the comment or the PR validator rejects it, which in your case would be something like: Features "Implemented UI framework with inventory screen use case" or with a better comment. |
25c284f
to
9ab6563
Compare
Good catch, done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll also need to update the cmake and gradle builds. You can look at my flatbuffer pr stack #50143 for examples of how to do that. Specifically b157d7e
imgui/imtui licences need to go into LICENSE.txt
As others have mentioned, at minimum split it up so the clean source is its own commit, then any changes to made to them. Optionally although I recommend it, keep the integration of the third party code into the build as its own commit as well, followed by the changes to source to use the new code.
9a281d3
to
5265bd5
Compare
I'd love to get on with this and make some UI tiles as soon as this is ready 😃 Curious question (I don't understand programing), can this work outside the game itself? Like the game's main menu, character creation, maybe even add some logos/loadscreen images? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are some comments on the changes to ui_adaptor
. I'll review the remainder when I have more time.
I also took a quick look at the other UI code and it seems you removed the original ui_adaptor
resize callbacks. The resize callback is need by ui_adaptor
to do occlusion check and invalidation flag optimization, so removing it would break the logic of the ui_adaptor
stack.
Therefore I suggest you leave ui_adaptor
as is and use it in the imgui window classes and provide the approriate callbacks instead to avoid complictating the logic. (And also avoid confusion with the name, it is an adaptor and not the window itself after all).
And in case you missed the questions in my previous comment (because you didn't reply): Could you share some screenshots of the new UIs? Also, does this allow windows and text in the SDL build to have more fine-grained positioning not restricted to a terminal grid?
Do we have to include the full source code of the framework? Have there been edits made to it for Cataclysm? |
I wonder what's the accessibility situation with this PR? Is ImGui/ImTUI screen reader friendly? |
ImTui uses a special version of ImGui in its source, and ImTui itself has been heavily modified (see commits). As for ImGui itself, this is the easiest way to include it that works on all platforms. |
Hard to say. When I made these changes I was not initially aware of screen readers being used in CDDA. There will probably be significant work needed to support these. |
A few people have asked for screenshots now... I've included a few. |
If we do go ahead with implementation on this, I would prefer we host and compile the gui libraries elsewhere to keep the line count down. Maybe I'm an outlier, but I'm a little worried that this PR is 100k lines of code. |
I'm not a screen reader user myself but I know there are multiple blind/VI CDDA players, mostly because CDDA is very accessible to them (e.g. the V menu is supposedly extremely useful). So it's important this part of accessibility doesn't regress (The current inventory is, to my understanding, reader accessible) |
Currently screen readers are supported by placing the terminal cursor at the right position so the only potential problems I can think of are: 1. If ImTui does not place the cursor after the last drawn string, most of the windows that do not use If you can compile the curses version, you can test it by adding the |
So the vcpkg integration is kind of a headache. Both imgui and cdda depend on sdl but only cdda currently actually depends on it via vcpkg. Having a dependency on a stub target that has the vcpkg config in it doesn't actually block any downstream targets from building before vcpkg completes, afaict. I'll keep poking at it. |
I pulled the current changes and tested on SDL and Curses. I had to change a few lines of code for it to compile, so some of the results might not be entirely accurate, but here they are. On SDL it mostly worked, but as expected it only showed the ImGUI windows when they are topmost windows. On Curses it did not seem to display the color at all and sometimes input was completely ignored, including ctrl-c, so I had to restart the terminal. It could be different in other termianls, because I was using MSYS2 MSYS and ImTUI seems to use pdcurses instead of ncurses, but since the game worked before in MSYS2 MSYS this should probably be fixed nevertheless. Anyway, the display on my setup is shown in the following image, and the cursor is positioned in the top right corner (the pink square), whereas before these changes the cursor was in the bottom right corner, so ImTUI indeed changed the cursor position, but the original cursor position probably wasn't working for screen readers anyway. The trade UI on the other hand was not shown at all. There was also a crash when resizing the SDL window before the main menu was shown, and IIRC it wasn't happening before these changes. Full crash log
Another crash happened in the Curses build when I pressed up/down to navigate the list of saves in the main menu. Unfortunately I didn't compile with backtrace on so I don't have a stack trace. On a side note, the ImGUI windows are currently translucent in the SDL build, but It also seems a |
Alright, I took a stab at creating a new option in the "interface" group, a "Use ImGui?" option, with a default value of false. I did some testing, and it appears that the legacy UI for the popups and the keybindings are intact. Once I get all the checks ran successfully, I will rebase again. After that, maybe we can start to consider merging this to master? |
I noticed the build failure in macOS CI, so I tried to compile the branch locally on macOS, and found out that a newer version of GNU Make (I tried 4.4.1) is required to build it correctly. GNU Make 3.81 shipped in the system interprets the Makefile differently and feeds different compiler flags (specifically without |
Oh that's very odd, I don't understand how I got it to build before. Do you have any idea what it isn't liking? |
Your pull request was in draft mode so macOS build was actually skipped and showed a green mark (you can see the build only took a few seconds to finish). |
…oved some extra imgui-specific drawing code. Co-authored-by: Jianxiang Wang (王健翔) <[email protected]>
Co-authored-by: akrieger <[email protected]> Update src/sdltiles.h Co-authored-by: Jianxiang Wang (王健翔) <[email protected]> Update src/ui_manager.cpp Co-authored-by: Jianxiang Wang (王健翔) <[email protected]>
The invalidation logic is already there, we just have to set the `dimensions` so that it knows what we drew over.
Now that an ImGui window can draw arbitrary pixels on top of a normal “curses” window, the terminal_framebuffer is no longer adequate for keeping track of which characters in a “curses” window can be skipped. We just have to clear and redraw the whole window. This patch removes the terminal_framebuffer, the oversized_framebuffer, and several other cached values. Gone as well is the complexity of initializing, reinitializing, and invalidating these caches.
ceacc6e
to
f37380e
Compare
rebased, squashed a ton of commits. haven't rebased against master, gonna wait until @Brambor 's changes get merged. Builds should still be working unless something got lost. I skipped the commits for fixing the Gradle build, FYI. My current plan for Android is to force usage of the legacy UI until we have a more concrete plan to fix it. Haven't had time to build and test it, probably needs more work. I think the branch is ready otherwise. |
Compiled and worked! My screens looked like your test screens. Congratulations! |
|
why are you posting crash logs in a closed PR? please make an issue with some details |
Summary
Features "Integrated ImGui UI framework into Cataclysm, and ported inventory screens to the new framework as a proof-of-concept"
Purpose of change
Implementation of simple things like navigation between elements and mouse-based selections involve code which currently needs to be repeated in every UI that exists in Cataclysm. Integrating a common UI framework like ImGui allows much of that heavy lifting to be handled by the framework, allowing less code required to produce functional and modern dialogs/forms. Other things like modal dialogs are simple impossible with the current code base.
Describe the solution
For tiled/SDL builds, the latest ImGui has been integrated into CDDA, using the SDL renderer backend so that it works seamlessly alongside all existing drawing code. On the Curses side, the ImTui backend has been integrated, with heavy modification to fix issues drawing alongside the rest of the game's screens. ImGui windows can draw on top of the play area without any graphical glitches. In addition, there are new common classes under cata_imgui.h which allow easy ways to create an ImGui window, and add it to the ui_adaptor stack for later drawing, without needing to write extra code to do so.
Describe alternatives you've considered
Before starting this branch, the original solution was to create a common class to create dialogs that create the same NCurses-like windows, but in a more common fashion. This would be easier to switch to, but also would look much less modern.
Testing
I've spent countless hours testing each screen as I've ported them, still many bugs to be found I'm sure.
Additional context
this PR originally ported the inventory screens, but reviewers didn't like that there were some unavoidable graphical bugs related to ImGui screens not able to be sandwiched between the game board and a non-ImGui screen on top, so all of that work had to be thrown out in favor of only porting screens that could be drawn on the top-level, meaning the keybindings screen and query_popup.