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

inventory_ui: only hide entries added recursively #52709

Merged
merged 4 commits into from
Nov 10, 2021

Conversation

andrei8l
Copy link
Contributor

@andrei8l andrei8l commented Nov 8, 2021

Summary

SUMMARY: None

Purpose of change

Items in collapsed containers are hidden in inventory UIs even if their parent isn't in the list
Fixes: #52706
Items in in nested containers are still shown even if the top container is collapsed
Fixes: #52699
Fixes: #52722

Describe the solution

Track the topmost visible parent and gate visibility checks on its presence in the list.

Describe alternatives you've considered

Looping over all entries in inventory_selector::prepare_paging(): I get sick just thinking about this

Testing

For #52706:
Start with a backpack, a loaded flashlight, a 3L jar, a fruit jam, and a bandage. Insert the jam in the 3L jar, then the bandage and 3L jar in the backpack, then collapse the backpack.

  1. The fruit jam and bandage should still show in the Eat menu
  2. The flashlight should still show in the activate menu, but not in the Uunload menu (unless you un-collapse the backpack first)

Try again after dropping the backpack on the floor or stashing it in a vehicle, or in another container.

For #52699:
Start with a backpack and collapse it. Insert a fruit jam into a 3L jar, make sure the jar is not collapsed, then insert the jar into the backpack. The fruit jam should be hidden in the player inventory menu. Both the jam and the jar should show in the Open item menu AND you should be able to independently (un)collapse the jar.

Additional context

This only works if items are added in a top-down order, which should be the case everywhere at the moment.

@kevingranade
Copy link
Member

Ok good I was looking at this and had come up with scanning for the parent and a terrible "disable it on a per-menu basis", but that turns out to be an equally bad tarpit. Your approach here seems workable. My other thought in this direction is adding a parents field to inventory entry and populating that... somehow.

I might be missing something here, but the fact that entries can aggregate across several different items seems problematic, if the first item listed in an entry is hidden, the entire entry is hidden, that seems wrong and difficult to fix. Hopefully I'm just missing something about the preconditions enforced by class construction.

@andrei8l
Copy link
Contributor Author

andrei8l commented Nov 8, 2021

if the first item listed in an entry is hidden, the entire entry is hidden, that seems wrong and difficult to fix

AFAICT contained items don't aggregate across pockets, but let me know if you have a specific demo case. You're right that the fix wouldn't be easy (I'm specifically trying to avoid looping or building parent links in inventory_entry)

@andrei8l andrei8l marked this pull request as ready for review November 8, 2021 07:46
@andrei8l andrei8l force-pushed the collapse_hide_recursive branch 2 times, most recently from 59b8082 to 4e17456 Compare November 8, 2021 09:03
@andrei8l

This comment has been minimized.

@Maleclypse Maleclypse added the <Bugfix> This is a fix for a bug (or closes open issue) label Nov 8, 2021
@andrei8l
Copy link
Contributor Author

andrei8l commented Nov 9, 2021

After some consideration, I was able to address both #52706 and #52699 with only a minor change to the base idea; please see the updated description.

EDIT: everything should be working fine now, or your money back!

@andrei8l andrei8l marked this pull request as draft November 9, 2021 14:19
@andrei8l andrei8l force-pushed the collapse_hide_recursive branch from 1135b2a to 634a1c9 Compare November 9, 2021 16:16
@andrei8l andrei8l marked this pull request as ready for review November 9, 2021 16:16
nested items are properly hidden now so there's no need to collapse
the entire chain
@andrei8l andrei8l force-pushed the collapse_hide_recursive branch from 634a1c9 to 67ebfda Compare November 9, 2021 16:23
@andrei8l
Copy link
Contributor Author

andrei8l commented Nov 9, 2021

I just noticed a new bug with (un)collapsing after filtering items. I have a preliminary fix here but I think I'll open a separate PR for it if this is accepted.

@wapcaplet wapcaplet added [C++] Changes (can be) made in C++. Previously named `Code` Info / User Interface Game - player communication, menus, etc. Items: Containers Things that hold other things labels Nov 10, 2021
@kevingranade kevingranade merged commit 6db9695 into CleverRaven:master Nov 10, 2021
@andrei8l andrei8l deleted the collapse_hide_recursive branch November 10, 2021 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
<Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Info / User Interface Game - player communication, menus, etc. Items: Containers Things that hold other things
Projects
None yet
4 participants