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

Try to fix grave/sarcophagus etc. not working #69

Merged
merged 6 commits into from
Dec 28, 2024

Conversation

Foxtr0t1337
Copy link
Contributor

@Foxtr0t1337 Foxtr0t1337 commented Dec 27, 2024

  1. Added new prepatch TryFindBestBetterStorageForPatch
  2. Rebuilt for release so that changes take effect

TryFindBestBetterStoreCellForPatch had been deciding which item should be removed from listerHaulables, which improves performance. But it had been checking SlotGroups only.
Patching TryFindBestBetterStorageForPatch and checking for NonCell could solve the issue of grave/armor rack etc. not working. #60

Tried several things. Nothing broke, and it solved said problem.

1. Added new prepatch `TryFindBestBetterStorageForPatch`
2. Moved `TryRemoveFromListerHaulables` from `TryFindBestBetterStoreCellFor` to the new prepatch
3. Rebuilt for realse so that changes take effect

`TryFindBestBetterStoreCellForPatch` had been deciding which item should be removed from `listerHaulables`, which improves performance.
But it had been checking `SlotGroup`s only.
Patching `TryFindBestBetterStorageForPatch` and checking both Cell and NonCell achieves the same thing, and could solve the issue of grave/armor rack etc. not working.

Tried several things. Nothing broke, and it solved said problem.
1. Added new prepatch `TryFindBestBetterStorageForPatch`
2. Moved `TryRemoveFromListerHaulables` from `TryFindBestBetterStoreCellFor` to the new prepatch
3. Rebuilt for release so that changes take effect

`TryFindBestBetterStoreCellForPatch` had been deciding which item should be removed from `listerHaulables`, which improves performance.
But it had been checking `SlotGroup`s only.
Patching `TryFindBestBetterStorageForPatch` and checking both Cell and NonCell achieves the same thing, and could solve the issue of grave/armor rack etc. not working.

Tried several things. Nothing broke, and it solved said problem.
…"flag", which is when `TryFindBestBetterStoreCellForPatch` is enabled.
@bbradson
Copy link
Owner

So, first of, thanks for contributing again.
Your implementation for now does have a number of issues:

  • It's compiled in debug mode, meaning there's gonna be log spam again. Release is the correct configuration for public releases.
  • The method returning 0 differs from vanilla behaviour. I believe that should've been thing.PositionHeld to have the thing at the best cell's current position returned instead
  • TryFindBestBetterStoreCellFor is often called directly, like always by the animal hauling jobgiver for example. All those times it wouldn't remove from ListerHaulables if the removal only happens in TryFindBestBetterStorageFor. That'd lead to a potentially significant performance degradation from having items not needing hauling continue to get checked for too long. Not sure how to cleanly fix that without overhead tbh, but fish's ThingsQueuedForRemoval is already a hashset so it could be viable to just have the current method continue adding, but let TryFindBestBetterStorageFor then remove from the queue again if there is better storage and then do another TryFindBestBetterNonSlotGroupStorageFor check when actually resolving the queue and removing
  • The replacement for TryFindBestBetterStorageFor looks, as far as I can tell, more intrusive than it needs to be. The result being IHaulDestination container storage can be identified by the method returning true with an IntVec3.Invalid foundCell. This can be handled by a more compatible postfix too

1. Added new prepatch `TryFindBestBetterStorageForPatch`
2. Rebuilt for realse so that changes take effect

`TryFindBestBetterStoreCellForPatch` had been deciding which item should be removed from `listerHaulables`, which improves performance.
But it had been checking `SlotGroup`s only.
Patching `TryFindBestBetterStorageForPatch` to cancel the removal if the best storage is non-cell could solve the issue of grave/armor rack etc. not working.

Tried several things. Nothing broke, and it solved said problem.
@bbradson bbradson merged commit 4b44be8 into bbradson:main Dec 28, 2024
@bbradson
Copy link
Owner

Thanks

@Foxtr0t1337 Foxtr0t1337 deleted the patch1 branch December 29, 2024 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pawns can't haul to Armor Racks if item is in another stockpile with Tick prepatch option enabled
2 participants