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

vehicle inventory does not notify of 4096 item limit #58966

Closed
bouchacha opened this issue Jul 4, 2022 · 13 comments
Closed

vehicle inventory does not notify of 4096 item limit #58966

bouchacha opened this issue Jul 4, 2022 · 13 comments
Labels
(S1 - Need confirmation) Report waiting on confirmation of reproducibility stale Closed for lack of activity, but still valid.

Comments

@bouchacha
Copy link

Describe the bug

Moving items onto a cargo shelving with plenty of free space (961L out of 2000) nevertheless generates a message indicating "the cargo shelving is full, so it fell to the grass". The game doesn't even prompt you with the typical "there isn't enough room" warning you would otherwise get.

Steps to reproduce

  1. Move items onto cargo shelving
  2. At some indeterminate point the items will fall onto the grass

Expected behavior

You'd expect the inventory volume indicator to be accurate. If volume is not maxed out, there's no reason items should fall to the ground.

Screenshots

image
image

Versions and configuration

  • OS: Windows
    • OS Version: 10.0.19044.1806 (21H2)
  • Game Version: cccb059 [64-bit]
  • Graphics Version: Tiles
  • Game Language: System language []
  • Mods loaded: [
    Dark Days Ahead [dda],
    Disable NPC Needs [no_npc_food],
    No Fungal Growth [no_fungal_growth],
    Bionic Professions [package_bionic_professions],
    Blaze Industries [blazeindustries],
    No Hope [no_hope],
    Stats Through Kills [stats_through_kills],
    Stats Through Skills [StatsThroughSkills]
    ]

Additional context

No response

@bouchacha bouchacha added the (S1 - Need confirmation) Report waiting on confirmation of reproducibility label Jul 4, 2022
@anothersimulacrum
Copy link
Member

Only 4096 items can be stored in a tile.

@bouchacha
Copy link
Author

That's good to know, the game does not tell you at all. What counts as an "item"? I notice that some items have the quantity described inline (e.g. "flour (130)"), while others have the quantity in a separate column (e.g. "solar panel 4"). I never understood why there were two ways of counting but I guess that's a separate question.

@anothersimulacrum
Copy link
Member

The first type of items are called count by charges items, and they represent multiple items with a single item and it's "charges", whereas other items represent multiple items with multiple literal items. We want to gradually remove count by charges items for technical reasons.

@Zireael07
Copy link
Contributor

AIM should probably tell you somehow that you're hitting the 4096 items limit.

@PatrikLundell
Copy link
Contributor

Isn't cargo shelving a game mod vehicle part? If so, this post should probably be tagged with the appropriate mod?

@Zireael07
Copy link
Contributor

Not sure, but either way the crux of the issue is in core game code.

@bouchacha
Copy link
Author

I just tested this without any mods at all. I spawned 10,000 ear plugs (0.1 L volume) and there's no issue if you're just moving items between ground tiles. The max number of items is indeed 4096, but if you try to move more onto that tile using AIM it will stop you and give this message:

image

The problem is that it doesn't give you this message when you're moving items onto vehicles, it just drops them to the ground without any explanation:

image

I've been playing this game for years and had no idea there was any item limit whatsoever. 4096 cotton patches is not completely unfathomable and is only 25L of volume, and vehicle inventory management is a core feature of the game, so this issue is bound to come up many times and cause countless confusion due to the lack of any message.
image

  1. There should at least be a message. "X is full" without a reference to the hardcoded item limit is extremely ambiguous and confusing.
  2. Why does the 4096 limit even exist? If we're moving away from count by charges items, we'd only be accelerating how often this limit becomes an issue.

@bouchacha
Copy link
Author

I'd like to edit the title of this issue because it's not specific to cargo shelving at all. Is that bad form?

@Zireael07
Copy link
Contributor

@bouchacha: Feel free to. It's the opposite of bad form, it'll make it clear that the problem is more general.

Why does the 4096 limit even exist? If we're moving away from count by charges items, we'd only be accelerating how often this limit becomes an issue.

Agreed re the limit becoming an issue in the future

@bouchacha bouchacha changed the title cargo shelving seems to have a hidden volume/weight cap vehicle inventory does not notify of 4096 item limit Jul 5, 2022
@Qrox
Copy link
Contributor

Qrox commented Jul 6, 2022

According to this comment #20483 (comment) the 4096 limit was to work around the performance issue of iterating through a std::list to find map items. However we already switched to using cata::colony to store map items and cata:colony should have an average random-access complexity of approximately O(logN), so it may be possible to remove or relax this limit. (Although it seems item_location still unpacks by iterating through the item stack which results in O(N) random access complexity, so that needs to be updated first).

@anothersimulacrum
Copy link
Member

#35411 (comment)

@Qrox
Copy link
Contributor

Qrox commented Jul 6, 2022

Well, we can at least tune the limit up if removing it is not an option. I'm not sure what overflow problem the comments in that issue are referring to. Considering that volume is always limited in a tile and should not overflow in any case, maybe they are refering to the weight? Although in that case you can still cause an overflow by having a stack of very many items counted by charge, nested containers, or a misbehaving mod that adds mini-blackholes to the game, so we probably want to add weight limit to a tile to prevent weight overflow.

gradually remove count by charges items for technical reasons.

In addition to causing the item limit to be hit more often as @bouchacha pointed out, this might also cause higher memory consumption if the items are not stored with an RLE-like data structure (especially with items such as thread which the player usually has thousands after some reasonable time). But that is more or less equivalent to keeping the items counted by charge but displaying them the same as a stack of items in UIs.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 6, 2023

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.

@github-actions github-actions bot added the stale Closed for lack of activity, but still valid. label Mar 6, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
(S1 - Need confirmation) Report waiting on confirmation of reproducibility stale Closed for lack of activity, but still valid.
Projects
None yet
Development

No branches or pull requests

5 participants