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

Expand 4096 item limit per tile #69898

Closed
KHeket opened this issue Dec 1, 2023 · 6 comments
Closed

Expand 4096 item limit per tile #69898

KHeket opened this issue Dec 1, 2023 · 6 comments
Labels
stale Closed for lack of activity, but still valid. <Suggestion / Discussion> Talk it out before implementing

Comments

@KHeket
Copy link
Contributor

KHeket commented Dec 1, 2023

Is your feature request related to a problem? Please describe.

Because of removing charges from lots of items - lots of problems was created

  1. Cargo space of cars and other container like vehicle parts cant contain more, than 4096 items
  2. Each tile can hold lesser items, than it should be

Solution you would like.

  1. Expand by 5-10 time current limit
  2. Or make different type of terrain/tiles hold different limit of items. Because as for logic - if you make big pile of salt, as example, at the grounds - it should crumbles to nearby tiles, when pile will be too big (similar to what we have, when trying to place more, than 4096 salt units). But if you put all your salt to cargo space, placed big carboard box, put it to the pit and etc - you should store more items because of logic, because this containers will prevent items from crumbling

Describe alternatives you have considered.

You can put items into containers, and than put it together to bypass the limit, but:

  1. Its unrealistic
  2. As example, when a have car with 3x3 cargo square, and trying to put all salt in one cargo - my character cannot do it

Additional context

No response

@KHeket KHeket added the <Suggestion / Discussion> Talk it out before implementing label Dec 1, 2023
@CoroNaut
Copy link

CoroNaut commented Dec 1, 2023

High-amount items like cotton patches, aspirin, paper, etc should really be combined somehow. If so, we would barely run into the 4096 limit and have much better performance. I hate that there is a draft for removing charges, because the 4096 limit will be absolutely laughable. So if that goes through, upgrading tile limit will be required.

Also, there are some ways that piles 'crumble' to nearby tiles if full. The more precise ways of moving items disallow it. Quite a few people are unaware of this and could probably be changed so that more methods can 'crumble" to another tile.

@Qrox
Copy link
Contributor

Qrox commented Dec 2, 2023

What was the actual rationale of the item limit though? In #20483 (comment) kevin said it was to avoid the performance issue that stemed from using std::list to store map items, but we now use cata::colony instead which has O(log(n)) random access time, and the charge removal had a pass even if it actually caused performance issues compared to some hypothetical performance regression caused by removing the item limit, so I don't find that argument convincing.

In #35411 (comment) kevin instead said it was to avoid hitting bugs in the code when the item count grows too large, but I don't see the difference between items in a container triggering the bugs and items in a map tile triggering the bugs. so if we really want to avoid bugs at all cost we should perhaps add the limit to containers as well (and get more bug reports from players because then they will not be able to store more than 4096 grains of salt in a condiment bottle...). But to be honest, if there is code that bugs out when there are to many items, the code should be fixed instead of putting a bandaid on it and pretending the bug doesn't exist. If the bug refers to the integer overflow problem stated in an earlier comment in that issue, the only difference I see that would trigger an integer overflow in a map tile is when too many items with small volume but large weight are added, which can be solved by adding a weight limit per tile instead, to be consistent with containers.

@ZhilkinSerg
Copy link
Contributor

You can easily change one line and compile with increased item limit:

constexpr int MAX_ITEM_IN_SQUARE = 4096;

@CoroNaut
Copy link

CoroNaut commented Dec 2, 2023

Time to make it int_max I guess. It could also be placed in options for the player to decide if they are playing on slower machines. Performance doesn't seem as big a concern as it has been previously, and newer methods of representing items make it better anyways.

@Zireael07
Copy link
Contributor

It could also be placed in options for the player to decide if they are playing on slower machines

IIRC it was already proposed and soundly rejected

Copy link
Contributor

github-actions bot commented Jan 1, 2024

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 Jan 1, 2024
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Closed for lack of activity, but still valid. <Suggestion / Discussion> Talk it out before implementing
Projects
None yet
Development

No branches or pull requests

5 participants