Fix #122 by simulating inventory transactions before execution. #127
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was happening because the output slot of a furnace is protected from all insertions. We were greedily extracting items at the beginning of the transfer and couldn't place the remainder back into the protected slot. Instead, simulate the transaction first, count how many items successfully transfer, then actually mutate the inventories by that amount. The ItemStorage wrappers already supported simulating transactions, so I just exposed the option in InventoryUtil. Also, because it's likely there's still some edge cases where things will go wrong (especially in the presence of modded blocks), I added some detailed logging in the case that we detect that items have been lost. Fixes cc-tweaked#122
This fixes a similar issue to the previous commit. It was possible for a TurtleSuckCommand to lose the remainder or put it back in a different slot from where it came.
|
is this mergable or is it stale? |
|
I... swear this was fixed?? is this still a problem?? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The problem was that we were extracting up to the limit before we knew how many items the destination inventory could actually accept, and then we would fail to replace the remaining items back into the furnace output slot since it's protected from insertions. My fix is to simulate the transaction first, see how many items were successfully transferred, and only then mutate the inventories by that amount.
I would appreciate any reviews or testing on this because I feel like there are probably edge cases I haven't considered and I don't have a good world to test this on. I added some error logging in case it detects that items were lost.
There was also a similar issue with the turtle suck command that I applied the same sort of fix to, along with the error logging.