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

Prevent cleanup of MOLLE pockets on contents update #70163

Merged
merged 2 commits into from
Dec 12, 2023

Conversation

GoLoT
Copy link
Contributor

@GoLoT GoLoT commented Dec 12, 2023

Summary

Bugfixes "Prevent cleanup of MOLLE pockets on contents update"

Purpose of change

Fixes #70160
Fixes #70126
Fixes #70121

Describe the solution

See #70160 for more details.

When items are removed from a pocket the item pocket contents are updated and any unexpected pockets are removed from the item. Only pockets defined in the item base type are kept. At some point additional pocket support was added through modular containers that can be integrated in a parent item but the cleanup code wasn't updated to not remove those new pockets during cleanups.

This PR makes it so the cleanup function doesn't remove "additional pockets" coming from integrated containers.

The update_modified_pockets() function is called only from one place at the moment. This is only a mitigation though and more problems related to the way additional pockets are handled are to be expected. See alternatives for details.

Describe alternatives you've considered

A rework of the additional pockets system since it seems to be a very hacky and unmaintainable solution. Currently the integrated items are added to a list of additional storage items while also adding extra pockets to the pocket list itself. The only way to tie the new pockets to the integrated items is through calculating indices based on the amount of pockets added by each item in the list.

This is prone to cause issues if the integrated items are modified down the road (i.e. the number of pockets added is changed) since the indices won't match any longer and their removal will cause issues. The added pockets should be tied uniquely to the item that added them (though a reference to the stored item in additional_pockets?) but it's a bigger refactor that I don't want to tackle right now.

Testing

Can't reproduce the problem after the changes.

Additional context

@github-actions github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` <Bugfix> This is a fix for a bug (or closes open issue) labels Dec 12, 2023
src/item.cpp Outdated Show resolved Hide resolved
@github-actions github-actions bot added the json-styled JSON lint passed, label assigned by github actions label Dec 12, 2023
Astyle

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@github-actions github-actions bot added the astyled astyled PR, label is assigned by github actions label Dec 12, 2023
@akrieger
Copy link
Member

@mqrause for #70044

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Dec 12, 2023
@mqrause
Copy link
Contributor

mqrause commented Dec 12, 2023

Thanks for this! I couldn't decide on which approach to take because I see the same issues. But this should be stable for now.

As a sidenote it'd be nice to have tests for the molle system, but that's not a requirement for this PR.

@akrieger akrieger merged commit 5fef522 into CleverRaven:master Dec 12, 2023
24 of 27 checks passed
@bombasticSlacks
Copy link
Contributor

This is prone to cause issues if the integrated items are modified down the road (i.e. the number of pockets added is changed) since the indices won't match any longer and their removal will cause issues. The added pockets should be tied uniquely to the item that added them (though a reference to the stored item in additional_pockets?) but it's a bigger refactor that I don't want to tackle right now.

Just perusing other PRs and saw this one. This code could for sure use a rewrite. It is a bit of a mess, but unless something changed since I last worked on it IIRC: The added pockets aren't serialized, just the added items. On game load the game appends the added pockets to the pocket data list, so if an added storage item is changed on game load the modified pocket array is added and if there are items that no longer fit, contents just spill. This added pocket cleanup code obviously didn't mesh well with that, but we've had MOLLE pockets for a few years prior with, as far as I know, no breakage on item changes, of which there have been plenty.

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` json-styled JSON lint passed, label assigned by github actions
Projects
None yet
4 participants