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

Remove limit of item count per tile #35411

Closed
wants to merge 1 commit into from
Closed

Conversation

BevapDin
Copy link
Contributor

@BevapDin BevapDin commented Nov 8, 2019

SUMMARY: Infrastructure "Remove item count per tile limit"

The crafting inventory already considers several dozen tiles around the character. It does not matter whether the games checks 1 tile with 10,000 items, or 10 tiles with 1,000, it has to iterate over all of them anyway.

And the item volume will add a natural limit anyway.

Removes checks involving them (replaced with a fixed `true` and simplified the resulting code).

Removes the constants.

Removes `item_stack::count_limit` as it represents this feature.

Uses `MAX_INT - something` as "special" indices in "game.cpp" - those values are the largest possible indices and it's very unlikely to ever appear as actual indices.
@KorGgenT
Copy link
Member

KorGgenT commented Nov 9, 2019

wasn't the max item limit per tile added due to integer overflow via mods? and i know we don't strictly support mods, but not handling the error that could appear doesn't seem that great

@BevapDin
Copy link
Contributor Author

BevapDin commented Nov 9, 2019

wasn't the max item limit per tile added due to integer overflow via mods? and i know we don't strictly support mods, but not handling the error that could appear doesn't seem that great

What integer overflow? Under what circumstances? What has it to do with mods? Can you b´please be a bit more specific or link to the respective PRs?

@mlangsdorf
Copy link
Contributor

Blazemod and aftershock both have vehicle parts with extremely high volume; please check that there aren't any problems with a mostly loaded "cargo dimension" (id: space_anomaly) from blazemod.

@mlangsdorf mlangsdorf added <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Crafting / Construction / Recipes Includes: Uncrafting / Disassembling Items / Item Actions / Item Qualities Items and how they work and interact Vehicles Vehicles, parts, mechanics & interactions labels Nov 9, 2019
@kevingranade
Copy link
Member

kevingranade commented Nov 10, 2019 via email

@kevingranade
Copy link
Member

Also I've been considering having crafting enumeration NOT consider the entire huge pile of items, instead only searching the "uppermost" X volume worth of items.

@BevapDin
Copy link
Contributor Author

Sanity checks not being 100% effective is no reason to remove them
entirely, the existing number is an utterly insane large number.

That is not a sanity check. That is an arbitrary limit. If we allow 4096 item on a square, why not 4097?

Put another way, what's the use case for having more than 4,000 items in a
single square?

#35398

I have encountered this problem on my own a few times.


And I have another argument: it's not realistic.

And the limit is actually not an item limit, but an item stack limit. One can put 10000 arrows (one stack, counts as 1) and 4095 other items on a square. But one can not put 10000 rocks (count as 10000) on a square.

@BevapDin
Copy link
Contributor Author

Checked by dropping about 40,000 lighters on a trunk. 11607 of them got into the trunk and 28393 landed on the grass below. Examining the tiles (cargo/terrain) works fine, as does picking them up.

@kevingranade
Copy link
Member

That is not a sanity check. That is an arbitrary limit.

That is not a contradiction, we don't know what number is going to cause an actual problem, so we limit it to an arbitrary number we think is safe. If you think that number is sometimes unsafe we can look into dropping it, if you think a higher number is necessary and will always be safe, we can look into raising it, but no limit is simply asking for trouble. The problem is, 4K is already a ridiculously high number that there is no sane rationale for reaching.

I'm not sure why you mention #35398, it doesn't have a rationale, just a bug report. The limit is involved in the bug, but if people are in the habit of trying to store everything in their base in a single tile, they will hit whatever limit exists, and I'd rather they hit a synthetic limit that we know the parameters of instead of him going off and discovering entirely new limits scattered all over the code base.
This includes future issues and performance issues.

@Maddremor
Copy link
Contributor

It could be worthwhile to look for candidates for making item stacks instead of individual items. Rocks and rags stand out as reasonable off the top of my head.

@DMFan79
Copy link

DMFan79 commented Nov 15, 2019

That is not a sanity check. That is an arbitrary limit.

That is not a contradiction, we don't know what number is going to cause an actual problem, so we limit it to an arbitrary number we think is safe. If you think that number is sometimes unsafe we can look into dropping it, if you think a higher number is necessary and will always be safe, we can look into raising it, but no limit is simply asking for trouble. The problem is, 4K is already a ridiculously high number that there is no sane rationale for reaching.

I'm not sure why you mention #35398, it doesn't have a rationale, just a bug report. The limit is involved in the bug, but if people are in the habit of trying to store everything in their base in a single tile, they will hit whatever limit exists, and I'd rather they hit a synthetic limit that we know the parameters of instead of him going off and discovering entirely new limits scattered all over the code base.
This includes future issues and performance issues.

I've opened the thread (#35398) to suggest changing several items into stacks, I'm ok with having a limit per tile. The main problem is that after a while, even without hoarding like I do, the base storage gets messy. Stacks help alleviate the problem.

I'd like to hear what people think about my suggestion.

@ZhilkinSerg
Copy link
Contributor

Let's postpone this post 0.E stable.

@kevingranade
Copy link
Member

I'm fine with a different limit, but no limit is not an option.

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` Crafting / Construction / Recipes Includes: Uncrafting / Disassembling Items / Item Actions / Item Qualities Items and how they work and interact Vehicles Vehicles, parts, mechanics & interactions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants