feat: chance recipe EV as exact rational pending stock (#89) - #90
Conversation
Determinize fixed-chance Create outputs via whole+pending Core ledger (#89), keep extract/Bus/craft on whole units, and dogfood milling EV. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces exact rational arithmetic via a new ExactRational class to support chance-based expected-value credits and pending stock in the storage ledger, integrating this with Create's milling, crushing, and cutting recipes. It updates the persistence schema to version 2 to store pending fractions, adapts the terminal UI to display pending stock in tooltips, and adds comprehensive self-tests and integration game tests. Feedback points out a critical issue in StorageCoreBlockEntity.java where a negative updated value during transaction simulation can trigger an uncaught IllegalArgumentException from ExactRational.whole(), potentially causing a crash.
| boolean stillOccupied; | ||
| try { | ||
| ExactRational total = ExactRational.whole(updated).add(pending).add(credit); | ||
| stillOccupied = total.floor() > 0 || !total.fractionalPart().isZero(); | ||
| } catch (ArithmeticException exception) { | ||
| return base; | ||
| } |
There was a problem hiding this comment.
If updated is negative (which can happen during simulation of transactions that consume more resources than are currently available), ExactRational.whole(updated) will throw an IllegalArgumentException because whole exact rationals cannot be negative. Since this block only catches ArithmeticException, the IllegalArgumentException will propagate and cause an unhandled crash during transaction simulation.
We should guard against negative updated values and treat them as not occupied to prevent this crash.
| boolean stillOccupied; | |
| try { | |
| ExactRational total = ExactRational.whole(updated).add(pending).add(credit); | |
| stillOccupied = total.floor() > 0 || !total.fractionalPart().isZero(); | |
| } catch (ArithmeticException exception) { | |
| return base; | |
| } | |
| boolean stillOccupied; | |
| if (updated < 0) { | |
| stillOccupied = false; | |
| } else { | |
| try { | |
| ExactRational total = ExactRational.whole(updated).add(pending).add(credit); | |
| stillOccupied = total.floor() > 0 || !total.fractionalPart().isZero(); | |
| } catch (ArithmeticException exception) { | |
| return base; | |
| } | |
| } |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the ExactRational class to support chance expected-value credits and pending stock for recipe outputs, particularly for Create Milling, Crushing, and Cutting recipes. It upgrades the storage ledger to schema version 2 to persist pending fractional amounts, updates the crafting terminal to track and commit expected credits, and enhances the UI tooltips to display pending quantities. A critical issue was identified in StorageCoreBlockEntity.java where a negative updated amount could cause an unhandled IllegalArgumentException inside ExactRational.whole(), potentially crashing the server instead of failing closed gracefully. A check should be added to return the base capacity immediately if updated is negative.
| long updated; | ||
| try { | ||
| updated = Math.addExact(current, entry.getValue()); | ||
| } catch (ArithmeticException exception) { | ||
| return base; | ||
| } |
There was a problem hiding this comment.
If entry.getValue() is negative and its absolute value exceeds current, updated will be negative. Calling ExactRational.whole(updated) with a negative value will throw an IllegalArgumentException and crash the operation (or the server) instead of failing closed gracefully. We should check if updated < 0 and return base immediately, as the transaction is guaranteed to fail anyway.
| long updated; | |
| try { | |
| updated = Math.addExact(current, entry.getValue()); | |
| } catch (ArithmeticException exception) { | |
| return base; | |
| } | |
| long updated; | |
| try { | |
| updated = Math.addExact(current, entry.getValue()); | |
| } catch (ArithmeticException exception) { | |
| return base; | |
| } | |
| if (updated < 0) { | |
| return base; | |
| } |
Mirror ledger underflow handling so capacity simulation cannot crash on overdraft. Co-authored-by: Cursor <cursoragent@cursor.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the ExactRational class to support chance-based recipe outputs (such as Create's Milling, Crushing, and Cutting) by crediting exact rational expected values into a new pending stock category. The StorageResourceLedger is updated to schema version 2 to persist these fractional values, and the UI is enhanced to display pending amounts in tooltips. The review feedback correctly identifies critical issues in StorageCoreBlockEntity.java where change events (fireChanged and fireResourceChanged) are fired with incorrect delta values. Specifically, when expected-value credits consolidate into whole units or expected debits consume them, the hardcoded or explicit transaction deltas fail to reflect the actual change in stored whole amounts, leading to potential desynchronization with external storage networks. Actionable code suggestions are provided to track pre-transaction amounts and compute the true deltas.
| if (!resourceLedger.apply( | ||
| deltas, expectedCredits, ledgerCapacity(deltas, expectedCredits), action)) { | ||
| return false; | ||
| } | ||
| if (action == Action.EXECUTE) { | ||
| if (invalidatesCraftableCache) craftableRevision++; | ||
| if (capacityTypesChanged) refreshTypeCount(); | ||
| if (deltas.keySet().stream().anyMatch( | ||
| key -> StorageResourceBridge.descriptorId(key).isPresent()) | ||
| || expectedCredits.keySet().stream().anyMatch( | ||
| key -> StorageResourceBridge.descriptorId(key).isPresent())) { | ||
| machineRevision++; | ||
| } | ||
| markStorageChanged(); | ||
| for (Map.Entry<StorageResourceKey, ItemKey> entry : itemKeys.entrySet()) { | ||
| updateItemIndex( | ||
| entry.getValue(), | ||
| resourceLedger.amount(entry.getKey()), | ||
| resourceLedger.occupies(entry.getKey())); | ||
| long delta = deltas.getOrDefault(entry.getKey(), 0L); | ||
| fireChanged( | ||
| entry.getValue(), | ||
| delta, | ||
| resourceLedger.amount(entry.getKey()), | ||
| actor); | ||
| } | ||
| for (StorageResourceKey key : deltas.keySet()) { | ||
| if (itemKeys.containsKey(key)) continue; | ||
| fireResourceChanged( | ||
| key, | ||
| deltas.get(key), | ||
| resourceLedger.amount(key), | ||
| actor); | ||
| StorageResourceBridge.energyType(key).ifPresent( | ||
| type -> fireEnergyChanged(type, resourceLedger.amount(key))); | ||
| StorageResourceBridge.stationWorkDescriptorId(key).ifPresent( | ||
| descriptorId -> fireStationWorkChanged( | ||
| descriptorId, | ||
| deltas.get(key), | ||
| resourceLedger.amount(key))); | ||
| } | ||
| for (StorageResourceKey key : expectedCredits.keySet()) { | ||
| if (itemKeys.containsKey(key) || deltas.containsKey(key)) continue; | ||
| fireResourceChanged( | ||
| key, | ||
| 0L, | ||
| resourceLedger.amount(key), | ||
| actor); | ||
| } | ||
| } |
There was a problem hiding this comment.
When applying a resource transaction, the deltas map only contains the explicit changes requested by the recipe. However, expected-value credits (expectedCredits) can also cause the whole amount of a resource to change (e.g., when fractional pending amounts consolidate into a whole unit). Using deltas.getOrDefault(key, 0L) to determine the delta for fireChanged and fireResourceChanged will result in a delta of 0L even when the actual stored whole amount increases, causing critical desynchronization with external systems (like AE2/Refined Storage networks) that rely on these events.
To fix this, we should capture the old whole amounts of all affected resources before applying the transaction, and then compute the actual delta as newAmount - oldAmount.
Map<StorageResourceKey, Long> oldAmounts = new HashMap<>();
if (action == Action.EXECUTE) {
for (StorageResourceKey key : deltas.keySet()) {
oldAmounts.put(key, resourceLedger.amount(key));
}
for (StorageResourceKey key : expectedCredits.keySet()) {
oldAmounts.putIfAbsent(key, resourceLedger.amount(key));
}
}
if (!resourceLedger.apply(
deltas, expectedCredits, ledgerCapacity(deltas, expectedCredits), action)) {
return false;
}
if (action == Action.EXECUTE) {
if (invalidatesCraftableCache) craftableRevision++;
if (capacityTypesChanged) refreshTypeCount();
if (deltas.keySet().stream().anyMatch(
key -> StorageResourceBridge.descriptorId(key).isPresent())
|| expectedCredits.keySet().stream().anyMatch(
key -> StorageResourceBridge.descriptorId(key).isPresent())) {
machineRevision++;
}
markStorageChanged();
for (Map.Entry<StorageResourceKey, ItemKey> entry : itemKeys.entrySet()) {
long oldAmount = oldAmounts.getOrDefault(entry.getKey(), 0L);
long newAmount = resourceLedger.amount(entry.getKey());
long delta = newAmount - oldAmount;
updateItemIndex(
entry.getValue(),
newAmount,
resourceLedger.occupies(entry.getKey()));
fireChanged(
entry.getValue(),
delta,
newAmount,
actor);
}
for (StorageResourceKey key : deltas.keySet()) {
if (itemKeys.containsKey(key)) continue;
long oldAmount = oldAmounts.getOrDefault(key, 0L);
long newAmount = resourceLedger.amount(key);
long delta = newAmount - oldAmount;
fireResourceChanged(
key,
delta,
newAmount,
actor);
StorageResourceBridge.energyType(key).ifPresent(
type -> fireEnergyChanged(type, newAmount));
StorageResourceBridge.stationWorkDescriptorId(key).ifPresent(
descriptorId -> fireStationWorkChanged(
descriptorId,
delta,
newAmount));
}
for (StorageResourceKey key : expectedCredits.keySet()) {
if (itemKeys.containsKey(key) || deltas.containsKey(key)) continue;
long oldAmount = oldAmounts.getOrDefault(key, 0L);
long newAmount = resourceLedger.amount(key);
long delta = newAmount - oldAmount;
fireResourceChanged(
key,
delta,
newAmount,
actor);
}
}| if (!resourceLedger.applyExpectedDebits( | ||
| expectedDebits, ledgerCapacity(), action)) { | ||
| return false; | ||
| } | ||
| if (action == Action.EXECUTE) { | ||
| craftableRevision++; | ||
| refreshTypeCount(); | ||
| markStorageChanged(); | ||
| for (Map.Entry<StorageResourceKey, ItemKey> entry : itemKeys.entrySet()) { | ||
| updateItemIndex( | ||
| entry.getValue(), | ||
| resourceLedger.amount(entry.getKey()), | ||
| resourceLedger.occupies(entry.getKey())); | ||
| fireChanged( | ||
| entry.getValue(), | ||
| 0L, | ||
| resourceLedger.amount(entry.getKey()), | ||
| actor); | ||
| } | ||
| } |
There was a problem hiding this comment.
Similarly to applyResourceTransaction, applying expected debits can cause a whole unit to be consumed (e.g., when a rollback of expected credits reduces the total amount below a whole unit boundary). Hardcoding the delta to 0L in fireChanged will cause desynchronization with external systems.
We should capture the old whole amounts before applying the debits and compute the actual delta as newAmount - oldAmount.
Map<StorageResourceKey, Long> oldAmounts = new HashMap<>();
if (action == Action.EXECUTE) {
for (StorageResourceKey key : expectedDebits.keySet()) {
oldAmounts.put(key, resourceLedger.amount(key));
}
}
if (!resourceLedger.applyExpectedDebits(
expectedDebits, ledgerCapacity(), action)) {
return false;
}
if (action == Action.EXECUTE) {
craftableRevision++;
refreshTypeCount();
markStorageChanged();
for (Map.Entry<StorageResourceKey, ItemKey> entry : itemKeys.entrySet()) {
long oldAmount = oldAmounts.getOrDefault(entry.getKey(), 0L);
long newAmount = resourceLedger.amount(entry.getKey());
long delta = newAmount - oldAmount;
updateItemIndex(
entry.getValue(),
newAmount,
resourceLedger.occupies(entry.getKey()));
fireChanged(
entry.getValue(),
delta,
newAmount,
actor);
}
}Listener events must use apply-before/after whole amounts so EV credits and debit rollbacks stay synchronized. Co-authored-by: Cursor <cursoragent@cursor.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces ExactRational to support chance expected-value credits and pending stock in the storage ledger, updating the persistence schema to version 2 and integrating pending stock display in the terminal. The review feedback highlights two critical issues: first, ExactRational.fromUnitInterval silently rounds float chances to the nearest 1/10000 basis instead of failing closed for unrepresentable values; second, StorageCoreBlockEntity.applyExpectedDebits fails to notify listeners of non-item resource changes, potentially causing desynchronization during rollbacks.
| public static ExactRational fromUnitInterval(float chance) { | ||
| if (!Float.isFinite(chance) || chance <= 0.0F || chance > 1.0F) { | ||
| throw new IllegalArgumentException("Chance must be a finite value in (0, 1]"); | ||
| } | ||
| if (chance == 1.0F) { | ||
| return ONE; | ||
| } | ||
| long scaled = Math.round((double) chance * CHANCE_BASIS); | ||
| if (scaled <= 0 || scaled > CHANCE_BASIS) { | ||
| throw new IllegalArgumentException("Chance could not be represented on basis " + CHANCE_BASIS); | ||
| } | ||
| return of(scaled, CHANCE_BASIS); | ||
| } |
There was a problem hiding this comment.
The current implementation of fromUnitInterval silently rounds the float chance to the nearest 1/10000 basis value. This violates the fail-closed design described in the documentation (which states that any ProcessingOutput whose chance cannot be represented on the fixed 1/10000 basis should be rejected). For example, a chance of 0.12345F or 0.99999F will be silently rounded and accepted instead of being rejected. We should add a strict representation check with a small tolerance (e.g., 0.01 after scaling by 10000) to ensure the chance is exactly representable on the 1/10000 basis.
public static ExactRational fromUnitInterval(float chance) {
if (!Float.isFinite(chance) || chance <= 0.0F || chance > 1.0F) {
throw new IllegalArgumentException("Chance must be a finite value in (0, 1]");
}
if (chance == 1.0F) {
return ONE;
}
long scaled = Math.round((double) chance * CHANCE_BASIS);
if (scaled <= 0 || scaled > CHANCE_BASIS) {
throw new IllegalArgumentException("Chance could not be represented on basis " + CHANCE_BASIS);
}
if (Math.abs((double) chance * CHANCE_BASIS - scaled) > 0.01) {
throw new IllegalArgumentException("Chance could not be represented on basis " + CHANCE_BASIS);
}
return of(scaled, CHANCE_BASIS);
}References
- Do not implement silent fallbacks for configuration values; if a configuration is missing or invalid, the system should fail or throw an exception.
| for (Map.Entry<StorageResourceKey, ItemKey> entry : itemKeys.entrySet()) { | ||
| long newAmount = resourceLedger.amount(entry.getKey()); | ||
| long delta = newAmount - previousAmounts.getOrDefault(entry.getKey(), 0L); | ||
| updateItemIndex( | ||
| entry.getValue(), | ||
| newAmount, | ||
| resourceLedger.occupies(entry.getKey())); | ||
| fireChanged( | ||
| entry.getValue(), | ||
| delta, | ||
| newAmount, | ||
| actor); | ||
| } |
There was a problem hiding this comment.
In applyExpectedDebits, the method only iterates over itemKeys to notify listeners of changes via fireChanged. However, expectedDebits can also contain non-item resources (such as fluids) that are being debited (e.g., during a transaction rollback). Because there is no notification loop for non-item keys in expectedDebits, external listeners will not be notified of these changes, leading to desynchronization. We should add a loop to fire fireResourceChanged for any non-item keys present in expectedDebits.
for (Map.Entry<StorageResourceKey, ItemKey> entry : itemKeys.entrySet()) {
long newAmount = resourceLedger.amount(entry.getKey());
long delta = newAmount - previousAmounts.getOrDefault(entry.getKey(), 0L);
updateItemIndex(
entry.getValue(),
newAmount,
resourceLedger.occupies(entry.getKey()));
fireChanged(
entry.getValue(),
delta,
newAmount,
actor);
}
for (StorageResourceKey key : expectedDebits.keySet()) {
if (itemKeys.containsKey(key)) continue;
long newAmount = resourceLedger.amount(key);
long delta = newAmount - previousAmounts.getOrDefault(key, 0L);
fireResourceChanged(
key,
delta,
newAmount,
actor);
}Reject unit-interval chances more than 0.01 off the 1/10000 lattice, and notify non-item listeners on expected-debit rollback. Co-authored-by: Cursor <cursoragent@cursor.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the ExactRational class to support fractional expected-value (EV) credits and pending stock for chance-based recipe outputs, specifically integrating with Create's Milling, Crushing, and Cutting processes. The storage ledger has been updated to schema version 2 to persist pending fractional carries, and the user interface now displays pending amounts in item tooltips. Additionally, robust transaction handling, rollback mechanisms, and comprehensive unit and integration tests have been added to verify the consolidation of fractional pending items into whole units. There are no review comments to evaluate.
CI script gates must match the build.gradle expected SelfTest summary. Co-authored-by: Cursor <cursoragent@cursor.com>
|
/gemini review |
Co-authored-by: Cursor <cursoragent@cursor.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the ExactRational class to represent non-negative exact rational numbers, enabling the system to support chance-based expected-value (EV) credits and pending stock (e.g., for Create Milling, Crushing, and Cutting). The StorageResourceLedger is updated to schema version 2 to track these pending fractional amounts alongside whole amounts, and the UI components are updated to display pending stock in tooltips. Integration tests, documentation, and build scripts have been updated to reflect these changes. I have no feedback to provide as there are no review comments to evaluate.
There was a problem hiding this comment.
Code Review
This pull request introduces support for chance-based recipe outputs by calculating and tracking exact rational expected values (EV) in a new pending stock ledger. It adds the ExactRational record to handle precise fractional arithmetic, updates the storage ledger to schema version 2 to persist pending fractions, and integrates this system with the Create mod compatibility layer (milling, crushing, cutting) to credit expected values. I have no review comments or feedback to provide as no comments were submitted for evaluation.
Lock TerminalDisplayStack pending metadata and Craftable commit through applyCoreResourceMutation. Co-authored-by: Cursor <cursoragent@cursor.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new exact rational representation (ExactRational) to support chance-based expected-value credits and pending stock within the storage ledger (upgraded to schema 2). It integrates this system with Create compatibility, allowing Milling, Crushing, and Cutting recipes with fractional output chances (on a 1/10000 basis) to credit pending stock, which consolidates into whole units upon reaching integer values. The terminal display has been updated to show pending fractions in tooltips, and corresponding tests have been added to verify these mechanics. As there are no review comments provided, I have no further feedback to offer.
1 participant
Summary
ExactRationaland ledger schema 2whole + pendingso fixed-chance outputs credit deterministic expected value without float storage.whole;pending > 0still occupies type capacity; Terminal shows whole with optional pending tooltip.(0,1]chance (basis1/10000); dogfoodmilling/short_grassconsolidates 4×25% into one wheat seed.Test plan
./gradlew build./gradlew runCreateGameTestServer(SelfTest 204935, 13/13)Closes #89
Made with Cursor