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_column: improve sorting #53289

Closed
wants to merge 4 commits into from

Conversation

andrei8l
Copy link
Contributor

@andrei8l andrei8l commented Dec 8, 2021

Summary

None

Purpose of change

Items are sorted haphazardly in most windows based on inventory_selector (most notably in the pickup menu)
Fixes: #52947

Describe the solution

  1. Sort items at the lowest common level, i.e. sort them by their respective parents from the level below their lowest common ancestor
  2. Sort items by unprefixed name first (same as in the AIM)

Describe alternatives you've considered

  • Fixing and using order_by_parent(): I'd have to understand it first
  • Sorting in multiple steps: seems too crude

Testing

  • Confirm that player inventory can still be displayed in hierarchical mode
  • Stash a lot of items in nested containers, then drop the containers on the floor, then open the pickup menu
  • Collapse and un-collapse containers in the pick up menu. New items should be placed appropriately and the order should not get messed up (including in the player inventory window when in hierarchical mode)
  • Kill a lot of identical zombies with debug then move them to the same tile, then open the pickup menu. Items should show below their respective zombie corpse parent instead of all at the end
  • Spawn and drop 50 glass jars of sauerkraut then open the pickup menu and confirm that all the sauerkraut is sorted below the aggregated jars. Repeat with arbitrarily large numbers (machine dependent).
  • Item menus that don't indent (ex: Eat menu) should be unaffected

Additional context

before

github bad

after edit: this screenshot still contains the bug with alphabetic sorting, but it should be fixed now

github good

I realize that this solution is quite ugly. not so ugly anymore

ITEMS_WORN is now a sortable category like the rest so it looks a little different in the player inventory window.

@github-actions github-actions bot added astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions labels Dec 8, 2021
@andrei8l andrei8l force-pushed the inv_sorting branch 2 times, most recently from 155e10e to c83b3d7 Compare December 8, 2021 19:14
@Maleclypse Maleclypse added the <Bugfix> This is a fix for a bug (or closes open issue) label Dec 8, 2021
@github-actions github-actions bot added BasicBuildPassed This PR builds correctly, label assigned by github actions and removed BasicBuildPassed This PR builds correctly, label assigned by github actions labels Dec 8, 2021
@andrei8l andrei8l force-pushed the inv_sorting branch 3 times, most recently from 9a79cbf to ed9ac53 Compare December 8, 2021 22:49
@andrei8l andrei8l marked this pull request as ready for review December 8, 2021 22:53
@github-actions github-actions bot added BasicBuildPassed This PR builds correctly, label assigned by github actions and removed BasicBuildPassed This PR builds correctly, label assigned by github actions labels Dec 8, 2021
src/inventory_ui.cpp Outdated Show resolved Hide resolved
@wapcaplet wapcaplet added [C++] Changes (can be) made in C++. Previously named `Code` Info / User Interface Game - player communication, menus, etc. Inventory / AIM / Zones Inventory, Advanced Inventory Management or Zones labels Dec 9, 2021
@github-actions github-actions bot added BasicBuildPassed This PR builds correctly, label assigned by github actions and removed BasicBuildPassed This PR builds correctly, label assigned by github actions labels Dec 9, 2021
@andrei8l
Copy link
Contributor Author

andrei8l commented Dec 9, 2021

I've simplified the code a bit so it should loop less and be easier to understand.

src/item.h Outdated Show resolved Hide resolved
@andrei8l andrei8l marked this pull request as draft December 10, 2021 04:49
@andrei8l
Copy link
Contributor Author

andrei8l commented Dec 10, 2021

I've rebased away the bad commits and fixed alphabetic sorting so it actually works now and not just happens to be correct by accident.

The situation with identically named items is still not perfect - their contents are not placed correctly when they are uncollapsed - but I think I can live with that edge case for now.

@andrei8l andrei8l marked this pull request as ready for review December 10, 2021 09:13
@github-actions github-actions bot removed the astyled astyled PR, label is assigned by github actions label Dec 10, 2021
andrei8l and others added 4 commits December 10, 2021 11:44
@github-actions github-actions bot added astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions and removed BasicBuildPassed This PR builds correctly, label assigned by github actions labels Dec 10, 2021
@andrei8l andrei8l closed this Dec 11, 2021
@andrei8l andrei8l deleted the inv_sorting branch December 11, 2021 08:44
@Zireael07
Copy link
Contributor

why close?

@andrei8l
Copy link
Contributor Author

I don't want to push an incomplete solution and the two complete ones were rejected without suggestions for alternatives so I'm out of ideas.

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 <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. Inventory / AIM / Zones Inventory, Advanced Inventory Management or Zones json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pickup menu items are not displayed within their container
6 participants