Fix out of order bug #167
Open
Fix out of order bug #167
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 is a pure refactoring commit (zero behavior changes). CostList.Calculator was difficult to test because it was entangled with the logic of ILabel and Recipe. This commit moves most of the logic from CostList into AbstractCostListService, which is generic over arbitrary Labels, Recipes, and CostLists. By adding this extra layer of indirection (which should mostly be optimized away by the JVM), we can write tests that are unconcerned about things like locales and ore dictionaries, and which don't manipulate the global recipe list and so can't collide with one another. While I have rearranged things, I have avoided changing any of the fundamental logic that previously existed in CostList. The new calculation code follows the old calculation code line by line, differing only in its use of the indirection layer. MainCostListService is the shim that connects AbstractCostListService to the concrete classes ILabel, Recipe, and CostList, so most code external to the `structure` package should now go through `MainCostListService.INSTANCE`. There are examples of this in `GuiCraft` and `JecaOverlayHandler`.
…lculator). All of the tests are in CostListServiceTest.
Tweaked the post-processing of calculate() to order steps in a way that keeps the user's inventory from growing unnecessarily large, which is particularly helpful in GTNH.
|
Thanks! Feel free to fork the 1.7.10 branch to gtnh. I really don't have the time to actively maintain the 1.7.10 branch anymore. And there are some issues that I'm having trouble fixing at the moment. To publish to curseforge, contact @Towdium on discord. If you have any questions, feel free to contact me via issue or email. Or you could remind me to look at discord (I can't passively get discord messages pushed to me🙃). |
|
Thank you for the fast response! I will submit to gtnh. Should I close the PR here, or leave it as is? |
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.
This is a fix for #110
I didn't have a prayer of fixing this without a good suite of unit tests, so the first thing I had to do was refactor it, adding a level of indirection so that the step calculation logic could be tested without concern for all of the rest of the code. That refactor is in the commit "Isolated CostList into CostListService." It is a pure refactor - no behavior changes. Details in the comment on the commit.
The next commit "Added tests for AbstractCostListService" adds unit tests that test the existing implementation. I think you'll find them quite pleasant (see CostListServiceTest). Again, no behavior changes.
The real changes are the next two commits:
"Fixed out of order bug" contains a fairly minimal fix for issue #110 . It also adds two unit tests that fail on the old code, but pass on the new code.
"Optimize steps for small inventories" goes a bit beyond issue #110, and adds a feature I've wanted for a long time: it chooses an order of steps that is not only correct, but also tries to minimize the amount of space taken up in the user's inventory as they execute the steps. For instance, if there are two steps, with step A turning 64 planks into 128 sticks, and step B turning 66 planks into 44 stairs, it will place step B before step A, because step B shrinks the user's inventory, while step A grows it. This can be helpful if the user is always close to capacity in their inventory.
It's a bit more complicated and arguably more than just a bugfix. I can split it out into its own pull request if you prefer. I played with it some and noticed some behavior that might be perplexing to the user:
The user might expect that the steps screen is the same for 2 and 3.
However, there is the potential that the order of steps has been rearranged, possibly quite a bit. In particular, the step they got the 2 items for, and possibly its near prerequisites, which were previously near the end of the steps, are now closer to the beginning. This is because, previously, they had 0 of them, so crafting 5 would have used up one of their inventory slots. Now that they already have 2, crafting 3 more isn't going to use up any more slots, so it gets priority over other steps that will use up inventory slots.
Open to discussion. I did most of my testing on GTNH, btw.
If this repo (or even the 1.7.10 branch) is no longer actively maintained, let me know, and I can submit this PR to the GTNH fork instead.