Fix crafting jobs appearing on other grids #30
Conversation
…le requests missing ingredients. Show missing ingredients state more consistently.
There was a problem hiding this comment.
I am not a huge fan of the new approach. I would rather fix the singleton problem by making the MissingState instance-specific and the simulatedPlanState final and recreate the object each time.
I do appreciate the pull request though! Making the state a singleton was an oversight, that's a good catch. I may supersede this pull request in a commit in a few days when working on the 1.21 update and then pulling the change to the other branches. This still is incredibly helpful. Thanks a lot!
| // if we change multiple states per tick then the client will never see us in "missing ingredients" | ||
| // we can either special case that state, or only transition once per tick, and once-per-tick is | ||
| // significantly less code :) |
There was a problem hiding this comment.
While I agree that this change would result in less code, it's not very efficient to schedule new operations to the next tick especially when exporting is involved. This makes the requester noticeably slower.
It's incorrect that the client will never see the "missing ingredient" state with the current behavior. You can just remove the necessary ingredients for a recipe that is configured in the requester and it will show you the missing state in the GUI. This is specified here: https://github.com/AlmostReliable/merequester/blob/1.19-forge/src/main/java/com/almostreliable/merequester/requester/status/PlanState.java#L33
There was a problem hiding this comment.
In the same setup to reproduce the issues caused by the singleton MissingState I was experiencing an issue where constantly missing an ingredient would result in the client feedback only showing Idle. I believe the reason for this is because right after transitioning from PlanState to MissingState, handle is then invoked again and we were immediately returning to PlanState. As I was writing this comment, I investigated more and now realize that this is actually a 2nd artifact of MissingState being a singleton and addressing that root issue would eliminate this behavior as well.
| this.future = future; | ||
| } | ||
|
|
||
| static PlanState BuildPlan(RequesterBlockEntity host, int slot, boolean wasMissing) |
There was a problem hiding this comment.
This is completely different code style from the rest of the project. Methods should always be lowercase even if they are static. You also should not place the opening bracket in the next line.
There was a problem hiding this comment.
Sorry, C# bleeding through and I apparently hate myself because I setup vscode for Java right as java22 broke a bunch of extensions. I should have double checked formatting though, that's on me.
|
Looking forward to your solution! I felt that I would be potentially introducing too many edge cases by instantiating new MissingState instances and would need more familiarity with the codebase to make sure none slipped through. |
Prevent crafting jobs ending up on the wrong grid when there are requests with missing ingredients on multiple grids. This was happening because MissingState was being used as a singleton but had request-specific state.
Show missing ingredients state more consistently.
Issue Reference
#21
Proposed Changes
Move state out of MissingState and add a field to PlanState so that "Missing Ingredients" won't disappear until we successfully transition to LinkState.