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

Declutter inventory UIs #56630

Merged
merged 6 commits into from
Apr 20, 2022
Merged

Conversation

andrei8l
Copy link
Contributor

@andrei8l andrei8l commented Apr 6, 2022

Summary

None

Purpose of change

Pickup menu is typically cluttered with useless entries such as pen ink, duplicated information such as ammo counts, and this fugly hidden marker at the end of collapsed items.
#56427 included a ham-handed fix that unconditionally hid all items inside magazines and was not well received
Closes: #56621
Invalidates (mostly): #53437

Describe the solution

  1. show items in magazines again with a preset switch
  2. allow collapsing magazines too instead of just container pockets
  3. collapse all magazines by default (for new items only). You can still quickly select the contents by filtering for ammo, etc (ex: c:ammo)
  4. only show the hidden marker for collapsed container pockets

Describe alternatives you've considered

instead of 1: include that functionality in inv_sel_preset::is_shown() instead. That needs a lot of boilerplate...
instead of 3: add COLLAPSE_CONTENTS to all the items whose default contents are subjectively useless. This is a lot of churn for the same effect
instead of 4: replace the marker with a red +. I haven't found any case where that information would be useful outside of inventory_selector. It's a minor change though if it turns out to be needed.

Testing

before #56427

Screenshot from 2022-04-06 21-18-29

Item names are sometimes improperly expanded
Screenshot from 2022-04-20 21-30-19

after this patch

Screenshot from 2022-04-19 06-27-27

Chevron isn't shown if the contents aren't allowed by the preset
Screenshot from 2022-04-19 06-50-03
(from player inventory window)

Nested magazines aren't expanded anymore:
Screenshot from 2022-04-20 21-30-57

The backpack/harness was collapsed manually and all the other items were newly spawned.

Additional context

Astyle really doesn't know how to break lines nicely.

@github-actions github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions labels Apr 6, 2022
@Maleclypse Maleclypse added the Info / User Interface Game - player communication, menus, etc. label Apr 6, 2022
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Apr 6, 2022
@PatrikLundell
Copy link
Contributor

I dislike the removal of the hidden indicator, as you now can chase around the UI to locate the blasted items that are in a container where it doesn't belong, but not find them because they're hidden. I want to be told specifically that the stuff in some containers are actually suppressed from view, rather than have the (new) player guess that you probably can't see items in first aid kits and wallets, but find that you can see them in STEM kits and the like.

Also, the testing images show only dedicated container type items, not "normal" containers, such as a box of toast-ems and a box of toast-ems where the pickup logic has also crammed e.g. a bottle of beer into the box. I assume those are intended to be unchanged, but it would be nice to verify that this actually is the case.

@Zireael07
Copy link
Contributor

I agree that removing the indicator isn't a good step.

@Theawesomeboophis
Copy link
Contributor

Is this optional, as I personally like the old Ui more as it contains more information.

@andrei8l
Copy link
Contributor Author

andrei8l commented Apr 7, 2022

I want to be told specifically that the stuff in some containers are actually suppressed from view

I agree that removing the indicator isn't a good step.

Isn't the chevron enough? It's pointing the wrong way (compared to here on github), but it also shows the key needed to uncollapse the item

and I don't want to mess with people's muscle memory

I could also add a keybinding hint in this block of text maybe

Screenshot from 2022-04-07 13-49-23

as you now can chase around the UI to locate the blasted items that are in a container where it doesn't belong,

Items that match the filter will show up even if hidden, since #53444

Is this optional, as I personally like the old Ui more as it contains more information.

Sorry, I can't make it optional with the current implementation of collapsing, unless I give up on saving the collapsed status. (EDIT: well I probably can, but it's looking pretty ugly and needs a global option). I don't think the old behavior gives you any extra information
Screenshot from 2022-04-07 13-42-33
red pen ink is written twice and you can't even interact directly with the second one. You can always uncollapse the red pen if you really want as this PR only changes the default status (and makes it possible to change in the first place for magazines).

@andrei8l
Copy link
Contributor Author

andrei8l commented Apr 7, 2022

I've re-added the hidden marker, but only for container pockets and inline with the rest of the text
output

@github-actions github-actions bot removed the BasicBuildPassed This PR builds correctly, label assigned by github actions label Apr 7, 2022
@andrei8l andrei8l force-pushed the inv_ui-declutter branch 2 times, most recently from d73f56a to 2d306eb Compare April 7, 2022 13:22
@mqrause
Copy link
Contributor

mqrause commented Apr 7, 2022

I like the chevron myself, maybe it's enough to put a bit more emphasis on it? Or instead of the hidden marker, a different text/background color for the entry could also be an option. The goal should be to be able to tell on a glance that an entry is collapsed, and having to actually read a word for that, which can be in different places so you have to read even more words to find it, doesn't do that well in my opinion.

@andrei8l
Copy link
Contributor Author

andrei8l commented Apr 7, 2022

I can use full black triangles for the chevrons
output
which should be more intuitive because this style is used literally everywhere, but it doesn't look so great when the contents are in a different column:
Screenshot from 2022-04-07 18-21-53
or aren't shown at all due to the preset:
I don't immediately see a good solution for these.

@mqrause
Copy link
Contributor

mqrause commented Apr 7, 2022

I like that. I'd consider placing it in front of the invlet, but I'm not entirely sure about that.

it doesn't look so great when the contents [...] aren't shown at all due to the preset:

Ideally it doesn't even get treated as a container in such cases.

@andrei8l

This comment was marked as outdated.

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Apr 7, 2022
@Theawesomeboophis
Copy link
Contributor

I want to be told specifically that the stuff in some containers are actually suppressed from view

I agree that removing the indicator isn't a good step.

Isn't the chevron enough? It's pointing the wrong way (compared to here on github), but it also shows the key needed to uncollapse the item
I could also add a keybinding hint in this block of text maybe
Screenshot from 2022-04-07 13-49-23

as you now can chase around the UI to locate the blasted items that are in a container where it doesn't belong,

Items that match the filter will show up even if hidden, since #53444

Is this optional, as I personally like the old Ui more as it contains more information.

Sorry, I can't make it optional with the current implementation of collapsing, unless I give up on saving the collapsed status. (EDIT: well I probably can, but it's looking pretty ugly and needs a global option). I don't think the old behavior gives you any extra information Screenshot from 2022-04-07 13-42-33 red pen ink is written twice and you can't even interact directly with the second one. You can always uncollapse the red pen if you really want as this PR only changes the default status (and makes it possible to change in the first place for magazines).

Oh, so I can still get all the info by uncollapsing the item? What I was talking about was the gun magazine not appearing, just realized second photo is collapsed, this seems like a great change now that I realized what it actually does, sorry for the confusion.

@andrei8l
Copy link
Contributor Author

I've fixed it so that the chevron only shows if the item's contents aren't hidden by the preset.

@andrei8l andrei8l force-pushed the inv_ui-declutter branch 2 times, most recently from 837d58f to 6081b92 Compare April 16, 2022 20:20
inv_ui: (un)collapse all standard pocket types

game: allow collapsing all standard pockets
only show it for container pockets
revert to: don't indent sort if column is forced to not indent

this caused far more issues than it solved
@kevingranade kevingranade merged commit 500e264 into CleverRaven:master Apr 20, 2022
@andrei8l
Copy link
Contributor Author

Thanks for merging.

@andrei8l andrei8l deleted the inv_ui-declutter branch April 20, 2022 20:19
@andrei8l andrei8l mentioned this pull request Jan 8, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions [C++] Changes (can be) made in C++. Previously named `Code` Info / User Interface Game - player communication, menus, etc. json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't fast-loot loaded contents of items anymore
7 participants