Skip to content
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.

Sign up for GitHub

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

Fix crafting jobs appearing on other grids #30

Closed
wants to merge 2 commits into from
Closed

Fix crafting jobs appearing on other grids #30

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eade2a2
Prevent crafting jobs ending up on random grids when there are multip…
rualyl Aug 17, 2024
ed1728f
remove dead code
rualyl Aug 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 9 additions & 14 deletions src/main/java/com/almostreliable/merequester/requester/RequesterBlockEntity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,31 +179,26 @@ private boolean handleRequests() {
var tickRateModulation = TickRateModulation.IDLE;
for (var i = 0; i < requestStatus.length; i++) {
var state = requestStatus[i];
var result = handleRequest(i);
if (!Objects.equals(state, result)) {

// 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 :)
Comment on lines +183 to +185
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@rualyl rualyl Aug 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

var nextState = state.handle(this, i);

if (state.type().translateToClient() != nextState.type().translateToClient()) {
changed = true;
}
var resultTickRateModulation = result.getTickRateModulation();
var resultTickRateModulation = nextState.getTickRateModulation();
if (resultTickRateModulation.ordinal() > tickRateModulation.ordinal()) {
tickRateModulation = resultTickRateModulation;
}

updateRequestStatus(i, result);
updateRequestStatus(i, nextState);
}
currentTickRate = tickRateModulation;
return changed;
}

private StatusState handleRequest(int slot) {
var state = requestStatus[slot];
updateRequestStatus(slot, state.handle(this, slot));
if (requestStatus[slot].type() != RequestStatus.IDLE && !Objects.equals(requestStatus[slot], state)) {
return handleRequest(slot);
}

return requestStatus[slot];
}

private void updateRequestStatus(int slot, StatusState state) {
requestStatus[slot] = state;
if (state.type().translateToClient() != requests.get(slot).getClientStatus()) {
Expand Down
20 changes: 1 addition & 19 deletions src/main/java/com/almostreliable/merequester/requester/status/MissingState.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,23 +3,12 @@
import appeng.api.networking.ticking.TickRateModulation;
import com.almostreliable.merequester.requester.RequesterBlockEntity;

import javax.annotation.Nullable;

public class MissingState implements StatusState {

@Nullable private PlanState simulatedPlanState;

MissingState() {}

@Override
public StatusState handle(RequesterBlockEntity host, int slot) {
// re-run the crafting job plan to see if we are still missing ingredients
if (simulatedPlanState != null) {
var planSim = simulatedPlanState.handle(host, slot);
simulatedPlanState = null;
return planSim;
}

var idleSim = StatusState.IDLE.handle(host, slot);
if (idleSim == StatusState.IDLE || idleSim == StatusState.EXPORT) {
// idle simulation returning idle means a request is no
Expand All @@ -30,14 +19,7 @@ public StatusState handle(RequesterBlockEntity host, int slot) {
}

// idle sim returned that we can request
var requestSim = StatusState.REQUEST.handle(host, slot);
if (requestSim == StatusState.IDLE) {
return StatusState.IDLE;
}

// request sim returned that we can start planning
simulatedPlanState = (PlanState) requestSim;
return this;
return PlanState.BuildPlan(host, slot, true);
}

@Override
Expand Down
26 changes: 24 additions & 2 deletions src/main/java/com/almostreliable/merequester/requester/status/PlanState.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package com.almostreliable.merequester.requester.status;

import appeng.api.networking.crafting.CalculationStrategy;
import appeng.api.networking.crafting.CraftingSubmitErrorCode;
import appeng.api.networking.crafting.ICraftingPlan;
import appeng.api.networking.ticking.TickRateModulation;
Expand All @@ -11,12 +12,33 @@

public final class PlanState implements StatusState {

private final boolean wasMissing;
private final Future<? extends ICraftingPlan> future;

PlanState(Future<? extends ICraftingPlan> future) {
PlanState(boolean wasMissing, Future<? extends ICraftingPlan> future) {
this.wasMissing = wasMissing;
this.future = future;
}

static PlanState BuildPlan(RequesterBlockEntity host, int slot, boolean wasMissing)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
// Once BuildPlan is invoked, we're crafting no matter what, rather than returning a null PlanState
var amountToCraft = Math.max(1, host.getStorageManager().computeAmountToCraft(slot));
var key = host.getRequests().getKey(slot);

var future = host.getMainNodeGrid()
.getCraftingService()
.beginCraftingCalculation(
host.getLevel(),
host::getActionSource,
key,
amountToCraft,
CalculationStrategy.CRAFT_LESS
);

return new PlanState(wasMissing, future);
}

@Override
public StatusState handle(RequesterBlockEntity host, int index) {
if (!future.isDone()) return this;
Expand Down Expand Up @@ -45,7 +67,7 @@ public StatusState handle(RequesterBlockEntity host, int index) {

@Override
public RequestStatus type() {
return RequestStatus.PLAN;
return wasMissing ? RequestStatus.MISSING : RequestStatus.PLAN;
}

@Override
Expand Down
18 changes: 2 additions & 16 deletions src/main/java/com/almostreliable/merequester/requester/status/RequestState.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,22 +9,8 @@ public class RequestState implements StatusState {
RequestState() {}

@Override
public StatusState handle(RequesterBlockEntity owner, int index) {
var amountToCraft = owner.getStorageManager().computeAmountToCraft(index);
if (amountToCraft <= 0) return StatusState.IDLE;
var key = owner.getRequests().getKey(index);

var future = owner.getMainNodeGrid()
.getCraftingService()
.beginCraftingCalculation(
owner.getLevel(),
owner::getActionSource,
key,
amountToCraft,
CalculationStrategy.CRAFT_LESS
);

return new PlanState(future);
public StatusState handle(RequesterBlockEntity host, int slot) {
return PlanState.BuildPlan(host, slot, false);
}

@Override
Expand Down
Loading