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

Rebuild AIM to integrate it with the main inventory #50015

Open
I-am-Erk opened this issue Jul 18, 2021 · 9 comments
Open

Rebuild AIM to integrate it with the main inventory #50015

I-am-Erk opened this issue Jul 18, 2021 · 9 comments
Labels
<Enhancement / Feature> New features, or enhancements on existing Inventory / AIM / Zones Inventory, Advanced Inventory Management or Zones (P4 - Low) Low priority issues: things which are e.g exotic, minor and/or hard to encounter <Suggestion / Discussion> Talk it out before implementing

Comments

@I-am-Erk
Copy link
Member

Is your feature request related to a problem?

Advanced Inventory Manager (AIM) is a great tool that is definitely an asset to CDDA user friendliness.

It is also a horribly organized code mess that nobody can work with and that doesn't integrate with any other inventory systems.

Proposed Solution

We've talked this around several different ways but I think ultimately the answer to the problem is that we need to deconstruct what AIM does well, and add those functions to Inventory, then remove AIM.

What does AIM do well?

  1. Bulk inventory movement
  2. Searching nearby terrain
  3. Collapses nested containers

How to do those things?

For these, I suggest we make a new tab to the existing inventory panel, "Surroundings". We could base this on AIM a bit: there should be at least two panes, for example. I suggest we mess with the "tiles around you" default, though, and instead have a number of presets:

  • your inventory
  • all tiles around you
  • tiles adjacent to you
  • all vehicle tiles near you
  • all furniture tiles near you that meet the requirements for automatically drawing materials for crafting
  • nearby faction NPCs or pets inventories
  • a popup list of specific furniture and vehicle tiles around you meeting those criteria

Note that any "show all" options should be restricted by range, visibility, ownership - all the filters that allow/prevent you from using items as crafting components.

The panels themselves should resemble the regular inventory a bit more than AIM, but should allow toggles of:

  • show items by category or in one list
  • show/hide wielded/worn items
  • sort by alphabet, weight, value, length, number, other factors
  • show/hide contents of containers

Then we should have a similar transfer interface as AIM.

We might also consider further interactions in this interface, like the ability to select a nested container or stack of containers in the target inventory and bulk transfer items into that container specifically. This would be a nice way to fill multiple water bottles from a Jerry can, eg.

Other considerations

Ideally this implementation would be set up to be accessed in other ways. For example, a bulk transfer dialogue could be used as a trading interface, integrating trading with inventory systems. Potentially it could even automatically open up when accessing storage furniture and vehicle storage spaces.

@I-am-Erk I-am-Erk added <Enhancement / Feature> New features, or enhancements on existing <Suggestion / Discussion> Talk it out before implementing Inventory / AIM / Zones Inventory, Advanced Inventory Management or Zones labels Jul 18, 2021
@RoyBerube
Copy link
Contributor

What you are describing sounds like the crafting menu screen. Different tabs for different selection options.

I agree that it would be wise to move AIM functionality into the inventory screen. It would reduce the amount of bugs we have to fix involving inventory management by not having to support the two approaches. Simplicity over complexity.

@andrei8l

This comment was marked as off-topic.

@I-am-Erk
Copy link
Member Author

I-am-Erk commented Jul 18, 2021

My rewrite already did all of this but you refused to review it. The containers bit was missing but I had ideas for that too.

That's... Not exactly what happened. Thanks for reminding me about that PR. What I said was:

hey, I am not at all qualified to comment on what you're doing with code, but I would caution that this PR has gotten untenably enormous. Is there any way to split this into multiple separate PRs so it can be tackled bit by bit? Obviously it's held by feature freeze, but a code rewrite that's 4000 lines long is just about impossible to review.

I am not qualified to review your PR because I don't know any of this code, I opened with that so you'd know I wasn't talking about any of the pieces of it. I also wasn't telling you it wasn't something we wanted or would appreciate, just that doing it all in one go was going to be hard for people to read, and if you were able to split portions out it would help.

If you want to go back and reopen #45243, please do. I wasn't asking you to close it, that's why I'm opening an issue asking someone else to pick up the torch. You'll note like three or more different major code contributors all recommended you not close the PR.

@andrei8l

This comment has been minimized.

@Maleclypse
Copy link
Member

Qrox is, unless I'm seriously confused, a senior dev on the project who's opinion would carry significant weight about what is and isn't mergeable.

@I-am-Erk
Copy link
Member Author

While I appreciate the support of other contributors and was especially looking forward to their review, they do not ultimately decide what's mergeable.

On a PR like that, neither do I. That's what I'm telling you, that stuff is out of my field, my opinion there is not at all the most important one to listen to. That's why I've led with that both times. The most knowledgeable person in that thread is qrox.

@stale
Copy link

stale bot commented Sep 3, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. Please do not 'bump' or comment on this issue unless you are actively working on it. Stale issues, and stale issues that are closed are still considered.

@stale stale bot added the stale Closed for lack of activity, but still valid. label Sep 3, 2021
@I-am-Erk I-am-Erk added the (P4 - Low) Low priority issues: things which are e.g exotic, minor and/or hard to encounter label Sep 3, 2021
@stale stale bot removed the stale Closed for lack of activity, but still valid. label Sep 3, 2021
@andrei8l
Copy link
Contributor

andrei8l commented Dec 14, 2021

In your proposal, I don't quite understand how you'd move items between tiles or to your dragged cart if the presets are only "all X" and such. Can you clarify that part, or are you planning to drop such functionality?

EDIT: Am I supposed to "open a popup list (...)" for each pane every time I want to use this thing? That sounds really awkward

@I-am-Erk
Copy link
Member Author

In my proposal I'm still picturing something basically the same as AIM, just with a few more filter options. So, to move something from a tile to the south to the tile to the north, you'd use the tiles adjacent to you option. I'd suggest the UI for selecting a tile should remain pretty much exactly as it is before, maybe with one or two more hotkeys like V to select all vehicle tiles, or something... and then maybe + or similar to get a popup for additional, less-often-needed filter options.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
<Enhancement / Feature> New features, or enhancements on existing Inventory / AIM / Zones Inventory, Advanced Inventory Management or Zones (P4 - Low) Low priority issues: things which are e.g exotic, minor and/or hard to encounter <Suggestion / Discussion> Talk it out before implementing
Projects
None yet
Development

No branches or pull requests

4 participants