Split Texture Category List - Fixes and Improvements by xoxor4d · Pull Request #49 · NVIDIAGameWorks/dxvk-remix

xoxor4d · 2023-11-17T21:06:17Z

This pull request fixes a few issues related to the legacy (split) texture list and also adds a new toggleable setting that only shows textures in a category that are assigned to that category.
Unassigned textures will be displayed in a new list called "Uncategorized".

Why?

Categorizing textures in games with a huge amount of loaded textures can get pretty painful. Especially if the list you are working with does not shrink in size.
It can also get really tedious to find textures you once assigned to a category if that category turned out to be wrong.
Having scrolled hundreds of textures to only get your scrolling position changed by accidentally moving your mouse over the world made me think about creating this :D

Issues with the legacy list prior to this pull request:

Having 2 or more category lists open and clicking a texture would not open the category-assignment popup and just leave a little dark square at sometimes random places. Same applies to "world" clicks.
IIRC, in earlier versions of remix, having the "Split Texture Category List" setting enabled would not show the category-assignment popup and just assign the texture one clicked on to the category it was clicked in. This is no longer the case and it will always show the popup. That behaviour doesn't make much sense to me if each list shows every texture.

Changes to fix issue 1:

Add string lastOpenCategoryId to the texture_popup namespace. lastOpenCategoryId holds the uniqueId of the currently active category (either by opening it or by hovering a texture within it). lastOpenCategoryId is checked within function showTextureSelectionGrid so that only the active category that called showTextureSelectionGrid is allowed to use its embedded texture highlighting / texture assignment popup logic.
Add bool lastOpenCategoryActive to the texture_popup namespace. It is used to check if the category saved in lastOpenCategoryId remained the active category (after all subsequent showTextureSelectionGrid function calls).
Mainly used it to check if a category header was closed. It will then clear the lastOpenCategoryId string and logic within either showSetupWindow or showTextureSelectionGrid assigns a new active category within the next frame.

const bool wasWorldClick = !clickedOnTextureButton && !ImGui::GetIO().WantCaptureMouse 
          && (ImGui::IsMouseClicked(ImGuiMouseButton_Left) || ImGui::IsMouseClicked(ImGuiMouseButton_Right));

Add !WantCaptureMouse check because wasWorldClick is true when clicking anywhere within the GUI that is not a texture. (Not really related to any issue anymore)

Changes to fix issue 2:

Fixing issue 1 also fixed issue 2 so the legacy split texture list no longer opens a popup and instead adds or removes the category directly upon clicking the texture

TL;DR

Add a new toggle called Only Show Assigned Textures in Category Lists under Split Texture Category List that is only visible if split view is enabled
Categories will now only show the textures that are assigned to it
Also adds a new list at the bottom of the existing categories called Uncategorized where all textures with no assignments can be found

sultim-t-nv

Great improvements to the UX, thanks for debugging and providing the solution :)

There are some minor change requests to the code, plus:

Add yourself to the "Github Contributors" list in ImGuiAbout::Credits::Credits(). Contributors are listed alphabetically by the last name
After your commits are ready:
1. squash them into one in the same branch
2. push (force-with-lease) into this pull request

Then we will test it and try to merge

sultim-t-nv · 2023-11-21T17:25:06Z

src/dxvk/imgui/dxvk_imgui.cpp

    namespace texture_popup {
      constexpr char POPUP_NAME[] = "rtx_texture_selection_popup";

+      std::string lastOpenCategoryId;


Default initializer would be nice just for consistency ... = {};

sultim-t-nv · 2023-11-21T17:25:47Z

src/dxvk/imgui/dxvk_imgui.cpp

-        } else {
-          clickedOnTextureButton = true;
-        }
+        // removed "toggleTextureSelection" logic as it was not working as intended even before


Maybe just remove this comment, so it would not be confusing in the future what was here before?

I've restored the original logic here as I've fixed the root issue and removed the comment

sultim-t-nv · 2023-11-21T17:30:54Z

src/dxvk/imgui/dxvk_imgui.cpp

      }
+
+      if (legacyTextureGuiShowAssignedOnly()) {
+        if (std::string(uniqueId) != "textures") {


Changing std::string to strcmp(..) == 0 (or to std::string_view) is preferable to not create std::string instance.

(Optionally, might need to put assert(..) + return to ensure that uniqueId is not null at the beginning of the function, so it's more obvious when debugging)

I didn't know about std::string_view. Really useful thank you!

sultim-t-nv · 2023-11-21T17:34:21Z

src/dxvk/imgui/dxvk_imgui.cpp

      const bool wasUIClick = clickedOnTextureButton;
-      const bool wasWorldClick = !clickedOnTextureButton && (ImGui::IsMouseClicked(ImGuiMouseButton_Left) || ImGui::IsMouseClicked(ImGuiMouseButton_Right));
-
+      const bool wasWorldClick = !clickedOnTextureButton && !ImGui::GetIO().WantCaptureMouse && (ImGui::IsMouseClicked(ImGuiMouseButton_Left) || ImGui::IsMouseClicked(ImGuiMouseButton_Right));


(Would be nice to split the long line into 2-3 lines for readability)

xoxor4d · 2023-11-23T21:47:47Z

I've updated the PR post to reflect the latest changes. I've fixed the underlying popup issue and also restored the original legacy list functionality. Also fixes the same popup issue when selecting textures via the world (I did not in my initial PR).

I've kinda messed up my squash by merging the latest changes tho ... Is that an issue?
Gotta admit that I'm not a git expert 😵‍💫

sultim-t-nv · 2023-11-30T00:39:30Z

We have tested it, but now textures are being highlighted (in world and UI) only after a mouse click is done on a world / UI element. Before, the texture highlighting was activated immediately, without any click. Would you like to improve it?

- Fix showing the category assignment popup when using legacy list mode - Add a new option to legacy mode that only shows assigned textures per category and adds a new texture list with unassigned textures - Variable child size for texture categories (only legacy mode with assigned textures)

xoxor4d · 2023-12-01T17:09:12Z

Awww I somehow got so used to clicking once that I didn't even notice it wasn't there before. Sorry for causing so much work 😵‍💫

Fixing that issue took quite some time and caused a few headaches because of how the highlighting / texture popup logic is embedded within showTextureSelectionGrid. I thought about getting rid of the texture popup logic from within showTextureSelectionGrid and include it in showSetupWindow (which might still be a cleaner solution) but the way its handled now is not bad either.

It's no longer required to click a world or UI element to get the highlighting logic to work (after switching the category)
Added logic to dynamically change the child size of categories so its not always using the minimum height of 600 pixels
Added the text Empty to categories that have no textures assigned

I've updated the initial PR post + picture showing the variable child size in action. I've also managed to squash all of the pervious commits into one 👍🏻

sultim-t-nv · 2023-12-01T17:59:00Z

causing so much work

No-no, it's all good! Thank you, that you are trying to debug and fix it

PR

Great, we will retest, and from the code and overview it looks like a better UX. Thanks again :)

sultim-t-nv · 2023-12-08T14:21:16Z

Alright, we may have some improvements to the object selection algorithm (but not certainly); and this pull request will be tested as a part of it

sultim-t-nv · 2024-01-23T00:20:58Z

@xoxor4d The corresponding internal branch was merged 90bb5a0

Check it, and, if everything is fine, close the pull request :)

xoxor4d · 2024-01-23T17:30:07Z

Thank you for merging! There are some really nice additional changes in there that greatly enhance the UX 🙌
Looking forward to the next pull request 😜

sultim-t-nv reviewed Nov 21, 2023

View reviewed changes

xoxor4d force-pushed the gui-better-legacy-texture-lists branch from 267631f to 5c8ee64 Compare December 1, 2023 15:55

xoxor4d closed this Jan 23, 2024

xoxor4d deleted the gui-better-legacy-texture-lists branch November 1, 2024 18:25

Conversation

xoxor4d commented Nov 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why?

Issues with the legacy list prior to this pull request:

TL;DR

Uh oh!

sultim-t-nv left a comment

Choose a reason for hiding this comment

Uh oh!

sultim-t-nv Nov 21, 2023

Choose a reason for hiding this comment

Uh oh!

sultim-t-nv Nov 21, 2023

Choose a reason for hiding this comment

Uh oh!

xoxor4d Nov 23, 2023

Choose a reason for hiding this comment

Uh oh!

sultim-t-nv Nov 21, 2023

Choose a reason for hiding this comment

Uh oh!

xoxor4d Nov 23, 2023

Choose a reason for hiding this comment

Uh oh!

sultim-t-nv Nov 21, 2023

Choose a reason for hiding this comment

Uh oh!

xoxor4d commented Nov 23, 2023

Uh oh!

sultim-t-nv commented Nov 30, 2023

Uh oh!

xoxor4d commented Dec 1, 2023

Uh oh!

sultim-t-nv commented Dec 1, 2023

Uh oh!

sultim-t-nv commented Dec 8, 2023

Uh oh!

sultim-t-nv commented Jan 23, 2024

Uh oh!

xoxor4d commented Jan 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

xoxor4d commented Nov 17, 2023 •

edited

Loading