Chemistry Set: look up the recipe once per tick - #149
Conversation
updateEntity walked the whole recipe list several times per tick, since isRecipeValid, getAmountNeeded and the output merging branch each ran their own lookup, and every comparison allocated a padded copy of the recipe array. The lookup now happens once and the result is reused. The matching rules are untouched, so which recipe wins for a given set of slots does not change.
There was a problem hiding this comment.
What the heck is this set of tests? Why not just replace all of this with
if (checkedItemStack.getItem() == recipeItemStack.getItem() &&
(recipeItemStack.getItemDamage() == OreDictionary.WILDCARD_VALUE ||
recipeItemStack.getItemDamage() == checkedItemStack.getItemDamage()) {
checkList[j] = true;
break;
} else {
return false;
}There was a problem hiding this comment.
This would also eliminate the need for the test variable.
There was a problem hiding this comment.
The ItemBlock check is dead code. Both branches pass only when the two stacks share one Item instance, which the final condition requires anyway, so I dropped it with the import.
The else branch I would not take. The inner loop looks for any free slot holding the ingredient, and returning false there gives up on the first slot that does not match. A one ingredient recipe would then fail whenever the ingredient sits in slot two, and the simple catalyst recipe fails on any slot order but the registered one. That is what test records: whether a match turned up before the candidates ran out.
Both branches of the check can only pass when the recipe stack and the slot stack share one Item instance, which the condition right below it already requires.
koolkrafter5
left a comment
koolkrafter5
left a comment
There was a problem hiding this comment.
The actual functionality changes look good, but we should probably keep the public API in tact.
| } | ||
|
|
||
| public static ItemStack getResult(ItemStack[] recipe, ItemStack bloodOrb) { | ||
| public static AlchemyRecipe findRecipe(ItemStack[] recipe, ItemStack bloodOrb) { |
There was a problem hiding this comment.
I'm not sure if we should be changing the public API this much. I'd add some methods with the old names/signatures that wrap your changed versions, like this for each of the methods that got changed:
public static ItemStack getResult(ItemStack[] recipe, ItemStack bloodOrb) {
findRecipe(recipe, bloodOrb).getResult();
}There was a problem hiding this comment.
getResult and getAmountNeeded keep their names and signatures and delegate to findRecipe, which is the wrapper you sketched. I removed nothing from the api package.
I did remove one method, TEWritingTable.getRecipeForItems, which sits in common rather than api. Want it back as a wrapper over findRecipe?
3 participants
Summary
updateEntity walked the whole recipe list several times per tick, since isRecipeValid, getAmountNeeded and the output merging branch each ran their own lookup, and every comparison allocated a padded copy of the recipe array. The lookup now happens once and the result is reused.
The matching rules are untouched, so which recipe wins for a given set of slots does not change. This is the part of #148 that does not depend on the changes discussed there.
Checklist