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

Optimize loading of pockets with many items inside #68051

Merged

Conversation

Kamayana
Copy link
Contributor

@Kamayana Kamayana commented Sep 8, 2023

Summary

Performance "Optimize loading of pockets with many items inside"

Purpose of change

When pockets have a large number of items inside the game can take a very long time to load them, as the game runs a can_contain check on every individual item inside, each of which checks against all the other items inside, giving it an exponential complexity growth.

Describe the solution

Use the simplified can_contain check created in #67631 to greatly reduce the processing time of the combine in item::deserialize. This simplified check ignores other contents of the pocket and only checks if the item could fit in the pocket even if it were empty. There's not really a reason to do the more intensive can_contain check, since if it was invalid it would've been handled before getting serialized in the first place.

Describe alternatives you've considered

It's possible there are other calls of can_contain that could use the simplified version, but I'm just doing it on a case-by-case basis for whatever needs optimization.

Testing

This save has a pocket universe filled with 100,000 pills: Long Load-trimmed.tar.gz

  1. Attempt to load that save in the vanilla version. When it gets to "Character save", the part where it's loading items, it hangs for so long that I eventually have to give up and force quit.
  2. Apply changes.
  3. Load the save in the new version. It only spends a few seconds on "Character save".
  4. Check inventory to see that everything still loaded properly.

Additional context

@github-actions github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` Items: Containers Things that hold other things Code: Performance Performance boosting code (CPU, memory, etc.) json-styled JSON lint passed, label assigned by github actions labels Sep 8, 2023
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 Sep 8, 2023
@KHeket
Copy link
Contributor

KHeket commented Sep 8, 2023

Bless you saint Kamayana, goddes of optimization 🙏

@Qrox
Copy link
Contributor

Qrox commented Sep 9, 2023

if it was invalid it would've been handled before getting serialized in the first place.

The size of items could change between serialization and deserialization due to JSON changes so I think you should test what happens in that case. Ideally the items should spill out of the pocket but I suppose it's fine to keep them inside the pocket as long as it does not generate any errors.

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Sep 9, 2023
@Kamayana
Copy link
Contributor Author

Kamayana commented Sep 9, 2023

if it was invalid it would've been handled before getting serialized in the first place.

The size of items could change between serialization and deserialization due to JSON changes so I think you should test what happens in that case. Ideally the items should spill out of the pocket but I suppose it's fine to keep them inside the pocket as long as it does not generate any errors.

Tested it, the container will sit in the world undisturbed until the player picks it up, which will spill the items out. No errors. However, if the container is non-rigid and wearable, examining it will show the "volume exceeds capacity" error from relative_encumbrance, so there's something to fix there. Also, while I think it works okay for too-full containers to only spill when you pick them up, I also think it might be possible to efficiently spill them on load, so I can look into that.

@Kamayana Kamayana marked this pull request as draft September 9, 2023 08:16
@Kamayana
Copy link
Contributor Author

Kamayana commented Sep 10, 2023

So I tested having overfilled pockets spill when loaded, but it felt weird for items to spill items out all on their own. The way the game currently works, with overfilled containers only spilling when you pick them up, seemed like something that could actually make sense in-universe, so I decided to see how it'd work to replace the error message with a note in the examine menu about it. I made it so overfilled pockets spill if they get hauled as well.

Here's a comparison of what happens when loading a save with containers that are now overfilled because of json changes:

Vanilla:

  1. Shows an error during loading ("error: item apple cannot fit into pocket while loading: not enough space").
  2. The game is loaded.
  3. After a single turn, carried overfilled containers spill.
  4. Overfilled containers on the map do not spill on their own.
  5. Examining the overfilled container on the ground shows an incorrect number of items, falsely decrementing the number to the 88 items (29.92 L) the pocket can hold, even though there are actually 90 inside: image
  6. On picking up the overfilled container, its contents spill.

With my previous changes:

  1. No error during loading.
  2. The game is loaded.
  3. After a single turn, carried overfilled containers spill.
  4. Overfilled containers on the map do not spill on their own.
  5. On examining the overfilled container, an error is shown ("volume exceeds capacity (30600ml > 30000ml) ).
  6. Examining the overfilled container on the ground correctly shows there are 90 apples currently inside the pocket:
    image
  7. On picking up the overfilled container, its contents spill.

With my new changes:

  1. No error during loading.
  2. The game is loaded.
  3. After a single turn, carried overfilled containers spill.
  4. Overfilled containers on the map do not spill on their own.
  5. Examining the overfilled container on the ground correctly shows there are 90 apples currently inside the pocket, but makes a note that the container is overfilled and will spill if moved:
    image
  6. On picking up or hauling the overfilled container, its contents spill.

@Kamayana Kamayana marked this pull request as ready for review September 10, 2023 08:45
@Maleclypse Maleclypse merged commit cf9cdde into CleverRaven:master Sep 10, 2023
@Kamayana Kamayana deleted the simplify_can_contain_deserialization branch January 9, 2024 12:43
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 [C++] Changes (can be) made in C++. Previously named `Code` Code: Performance Performance boosting code (CPU, memory, etc.) Items: Containers Things that hold other things json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants