Fix out of order bug #21
Merged
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.
|
Looks great and tests were much needed |
Caedis
approved these changes
Dec 9, 2024
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.
I am submitting this here because Towdium says he doesn't have time to maintain the upstream branch.
This is a fix for Towdium#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 Towdium#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 Towdium#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 2.6.1, btw.