search

CleverRaven / Cataclysm-DDA

Cataclysm - Dark Days Ahead. A turn-based survival game set in a post-apocalyptic world.
http://cataclysmdda.org
Other
10.26k stars 4.12k forks source link

Remove border and re-align ImGui UI to more consistently apply design principles #76373

Closed CLIDragon closed 1 day ago

CLIDragon commented 5 days ago

Is your feature request related to a problem? Please describe.

The big, glowing blue border is unattractive, causes weird sizing recalculations and entirely unnecessary. image

Moving the mouse does not change the selected text in the UI, duplicating the selected text. I also don't think there is a need for a separate highlight colour for mouse and keyboard selections. video

Solution you would like.

Re-align parts of the UI so that each element is evenly spaced from the previous in the list, the same colour is used for mouse and keyboard selection, the underline doesn't stretch weirdly past the title, and the highlighted region extends the full length of the underline.

UI Mockup: video

Describe alternatives you have considered.

No response

Additional context

No response

CoroNaut commented 5 days ago

I would love imgui to be more responsive as well. I can definitely notice a split second delay between my 'down' keypress and the list selection bar moving down. The previous UI didn't have such a slow response. It throws me off a bit whenever I'm doing fast key presses and have to use both the new imgui and previous UI one after the other.

CLIDragon commented 5 days ago

I would love imgui to be more responsive as well. I can definitely notice a split second delay between my 'down' keypress and the list selection bar moving down. The previous UI didn't have such a slow response. It throws me off a bit whenever I'm doing fast key presses and have to use both the new imgui and previous UI one after the other.

I think this is because ImGui doesn't render every frame, instead waiting for input.

CLIDragon commented 5 days ago

After spending several hours looking into this, I think that a lot is beyond my ability to fix. To summarise my findings:

  1. The blue border around the window. This can be fixed by passing ImGuiWindowFlags_NavFlattened into ImGui::BeginChild. The reason for the border is because the table is identified as a new element, not a child element like it should be. Flattening the nav solves this. Alternatively, because we handle keyboard input outside of ImGui, passing ImGuiWindowFlags_NoNav also works.
  2. The misaligned separator under the header. This can be fixed by either upgrading to v1.9.0 or later, or by changing the calculation of the separator rectangle to start at cursorPos() and finish at cursorPos() + contentSize() - 2 * WindowBorder() - 1.
  3. Text size is calculated as one larger than it actually should be. I suspect this is a MSVC specific problem, and it is caused by the implementation of rounding when calculating the text size in CalcTextSize(). This should be easy to fix in a local patch to ImGui.
  4. The indentation between the start of the outer widget and the rows. I have no clue why this occurs. It's definitely the call to BeginChild as the table that wraps it has no indentation from the window (beyond padding, of course).
  5. The separation between the selected row and the highlighted row. ~No clue why this occurs. It might have something to do with the selected row being changed outside of ImGui? In which case, there's a simple fix of setting the highlight to the selected row.~ This does indeed fix it, with the exception of leaving the mouse hovering and pressing buttons not taking the focus from the mouse.
  6. Highlight does not move with the selection on mouse hover. This can be fixed by just setting the selection when you set the hover highlight, and vice-versa.
  7. Padding between entries. I honestly haven't looked at this, but it could be as simple as setting the PadY attribute of the cells.
  8. The close button. This should also a simple fix, simply pass ImGuiWindowFlags_NoClose to the constructor.
db48x commented 5 days ago

Yea, the blue border is annoying but so far has been low enough priority that I‌ hadn’t tried very hard to get rid of it. Thanks for finding the solution. I had also considered removing the child window and making the table itself scrollable. I might do that just to simplify the code. I wish I had thought of it when I added the table :)

An upgrade to ImGui is already planned, but the fellow who was doing it has been busy or distracted or sleeping or whatever.

I‌ am going to keep the separation between the selected row and the highlighted row, because combining them made people complain and file bug reports. Plus, on menus with descriptions that show up in the footer, or extra information that shows up in a left or right sidebar, it is important that the footer and sidebar be updated based on the hover state, but that the keyboard still be able to override the mouse if the mouse is left hovering over the menu. There are eight or nine menus in the game that have a sidebar, but the most dramatic are the read spell book and spell casting menus from the Magiclysm mod. In the base game the advanced inventory manager will prompt for a location when you move an item out of inventory to the ground if the other column is showing all neighboring tiles rather than a specific one. The rest are mostly debug menus.

CLIDragon commented 4 days ago

Yea, the blue border is annoying but so far has been low enough priority that I‌ hadn’t tried very hard to get rid of it. Thanks for finding the solution. I had also considered removing the child window and making the table itself scrollable. I might do that just to simplify the code. I wish I had thought of it when I added the table :)

I briefly considered it, but I couldn't find any examples of where the left and right sidebars were used so I couldn't check the validity. EDIT: Nesting tables seems like it could run into issues, see https://github.com/ocornut/imgui/issues/6586 for example.

I‌ am going to keep the separation between the selected row and the highlighted row, because combining them made people complain and file bug reports. Plus, on menus with descriptions that show up in the footer, or extra information that shows up in a left or right sidebar, it is important that the footer and sidebar be updated based on the hover state, but that the keyboard still be able to override the mouse if the mouse is left hovering over the menu.

Oh, I didn't consider that. My problem with keeping them separated is that you can end up in a situation where the highlighted row and the selected row are distinct and the highlighted row moves with the cursor. If it can be fixed so that highlighted only moves when the mouse moves, that would solve my problem without combining it.

CLIDragon commented 4 days ago

Okay, I've looked into it a little bit more. It's definitely possible to create a nested table without the child window, but passing the ScrollY flag (to allow scrolling) just creates a new child window behind the scenes anyway, so the creation may as well be explicit.

I managed to fix the indentation. It was due to the left and right columns being created with a minimum width of 4, instead of 1. Disabling the columns when the calculated size is zero fixes the issue.

db48x commented 4 days ago

Awesome :)

Create a PR and I will approve it.