-
Notifications
You must be signed in to change notification settings - Fork 150
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
Better idling for metatileentities and multiblocks #1554
base: master
Are you sure you want to change the base?
Better idling for metatileentities and multiblocks #1554
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your hard work. From functional point it looks solid. I have some questions regarding implementation and other comments to places which needs adjustments.
Also there is problem with code formatting. As we are providing formatting in .editorconfig
I would suggest you to apply it or I may ignore additional PRs without proper formatting (and yes I am that type of person).
src/main/java/gregtech/api/capability/impl/AbstractRecipeLogic.java
Outdated
Show resolved
Hide resolved
src/main/java/gregtech/api/capability/impl/AbstractRecipeLogic.java
Outdated
Show resolved
Hide resolved
src/main/java/gregtech/api/capability/impl/NotifiableFilteredFluidHandler.java
Outdated
Show resolved
Hide resolved
.../java/gregtech/common/metatileentities/electric/multiblockpart/MetaTileEntityFluidHatch.java
Outdated
Show resolved
Hide resolved
e471d92
to
cd3e063
Compare
fb9ae22
to
3c9479e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spent a brief time looking this over, and left a small number of comments.
In addition to the comments left on the PR, I noticed some weird behavior while testing in game.
When testing on multiblocks, the front face was not lighting up green as should when the multiblock becomes active. This occurred for both the Multismelter and the EBF, so I expect somewhere that isActive
is not being properly updated.
src/main/java/gregtech/api/capability/impl/AbstractRecipeLogic.java
Outdated
Show resolved
Hide resolved
src/main/java/gregtech/api/capability/impl/AbstractRecipeLogic.java
Outdated
Show resolved
Hide resolved
src/main/java/gregtech/api/capability/impl/AbstractRecipeLogic.java
Outdated
Show resolved
Hide resolved
src/main/java/gregtech/common/metatileentities/multi/electric/MetaTileEntityMultiFurnace.java
Outdated
Show resolved
Hide resolved
a32225b
to
15feb38
Compare
Thanks for updating this. Can you please fix formatting of all edited files as it does not conform our style. IntelliJ IDEA should be able to do it perfectly as we are providing |
This looks quite solid to me. Anyone from Addons do you see any glaring problems that this could cause to you? |
1038313
to
e2f654b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more things brought up in looking this over.
src/main/java/gregtech/common/metatileentities/multi/electric/MetaTileEntityMultiFurnace.java
Outdated
Show resolved
Hide resolved
src/main/java/gregtech/common/metatileentities/electric/MetaTileEntityMacerator.java
Outdated
Show resolved
Hide resolved
@@ -198,9 +205,12 @@ protected Recipe findRecipe(long maxVoltage, | |||
} | |||
|
|||
// If there were no accepted ingredients, then there is no recipe to process. | |||
if(recipeInputs.isEmpty()) { | |||
// the output may be filled up | |||
if (recipeInputs.isEmpty() && !invalidInputsForRecipes) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only case where !invalidInputsForRecipes
would be true is when a matching recipe exists, and so the recipe inputs would not be empty, so this logic doesn't quite make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the output is full ( so canFit is false ) for that recipe, we exit the loop before adding to recipeInputs.
in that case, an empty recipeInputs does not mean that there are no valid input, it means we could not add anything because the output is full.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, we could have iterated through the for loop once first, which would set the state of invalidInputsForRecipes
. It might be a good idea to reset the value of invalidInputsForRecipes
if the check for canFitOutputs
passes, but before the break out of the loop.
For addons, this might cause some issues with @DStrand1 's Distinct Input bus implementation found in Gregicality (Initial PR done here GregTechCEu/gregicality-legacy#451), although I will leave it up to him to review this and see if he can rework the distinct bus implementation based on the changes in this PR, and if he has any other concerns. It may be interesting to implement distinct buses directly in GTCE, as although GTCE multiblocks could not make much use of it, especially since there are no Large Machine Multiblocks as there are in Gregicality, having a unified implementation may make it easier for addons to implement it, and integrate it with the changes done to GTCE logic. |
takes in consideration the result of the last iteration when: 1. no recipe found for current items in inputInventory. 2. if the recipe found cant push its result due to lack of space in the outputinventory.
avoids copying itemstacks that are at max size, and cant be merged into anyway
can notify controllers/metatileentities of inventory changes
recipe logic can wait for changes to the output if it cant process recipes if the output is full. can also wait for changes in the input inventory in case a recipe was searched for but didnt result in a valid recipe. implemented tests. one simulates a chemical reactor, the other one an electric blast furnace
restored cover field as final deprecated old arrays moved method to protected add java doc for deprecated checkRecipeInputsDirty renamed variables from single to plural for more clarity added instance checks before casting
match formatting to the project.
reformatted javadoc re-based
…ks copy" This reverts commit f05c193.
80781dc
to
c847713
Compare
Sorry to be late to the party on this. I have a few comments, the first is a longish diff.
Here is the diff:
|
@@ -257,6 +259,30 @@ public final String getMetaFullName() { | |||
return getMetaName() + ".name"; | |||
} | |||
|
|||
public <T> void addNotifiedInput(T input) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not understanding the logic here?
Why do items use the top level class IItemHandlerModifiable
while the fluids use FluidTank and NotifiableFluidTank.
Why don't the fluids use the IFluidHandler top level class?
} | ||
} | ||
|
||
public <T> void addNotifiedOutput(T output) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameterized type on this and the addNotifiedInput() does nothing?
It could just be replaced with Object.
You are using instanceof and cast on the parameter anyway.
/** | ||
* @param metaTileEntity MetaTileEntity to be notified | ||
*/ | ||
default void setNotifiableMetaTileEntity(MetaTileEntity metaTileEntity) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this really be a default method?
Is there a case where an implementing class doesn't implement this method?
Unfortunately, I don't know much about the AbstractRecipeLogic class which is the important change in this PR. |
What:
This PR is my attempt on the issue of gregtech machines being "laggy" when actually idling with full inputs and outputs.
I've iterated this concept a some times and i think its good so here it is.
How solved:
implemented checks in the Abstract recipe logic for inputs (if a valid recipe can be made out of them) and outputs (if its full, and we cant accept a recipe because of it, wait for the output to make some space before looping unsuccessfully)
implemented fluid and item handlers that can change those settings by notifying the metatileentity that its inventory changed.
Outcome:
GTCE single and multiblock machines will idle with a input of partial ingredients and when its outputs are full.
Additional info:
By avoiding recipe lookups when the inputs dont make a valid recipe, and by avoiding simulating merging items that we cant push into the output, we can save some tick time for other stuff
Possible compatibility issue:
Included tests that match the expected behaviour of a chemical reactor ( made by @exa ) and a blast furnace.
the outputfull is set on the
addItemsToItemHandler
method, and checked for in the new methodshouldSearchForRecipes
, so will work immediatelly.the inputsinvalidation flag should be set o the method that actually returns the Recipe object, and may need implementation on their part, but should not be too much work.