Conversation
WalkthroughTrade ship logic was refactored: gold calculation now depends only on distance; spawn rate now depends on a rejection counter and total trade ships rather than player port/trade-ship counts. Method signatures and call sites in configuration and execution layers were updated accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/core/execution/PortExecution.ts (1)
12-12: Small resilience thought:tradeShipSpawnRejectionsgrows without bound between successful spawns.In practice this is capped by the pity mechanism (spawn becomes near-guaranteed well before the counter gets very high). But if
PseudoRandom.chance()does not handle aspawnRateof 0 gracefully, aMath.max(1, ...)clamp inDefaultConfig.tradeShipSpawnRate(already noted there) would fully protect this path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/execution/PortExecution.ts` at line 12, tradeShipSpawnRejections can grow unbounded between successes; clamp the spawn rate or guard against zero so PseudoRandom.chance() cannot be fed 0. Fix by ensuring DefaultConfig.tradeShipSpawnRate is normalized (e.g. Math.max(1, DefaultConfig.tradeShipSpawnRate) when used) or add a protective check in the spawn logic before calling PseudoRandom.chance(spawnRate) to use at least 1; update references in the spawn code that read DefaultConfig.tradeShipSpawnRate and the call to PseudoRandom.chance(...) so the rate is never zero while preserving the existing pity behavior tied to tradeShipSpawnRejections.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/configuration/DefaultConfig.ts`:
- Around line 300-302: tradeShipGold currently returns toInt(10000 + 150 *
Math.pow(dist, 1.1)) but does not apply the instance gold multiplier
(inconsistent with trainGold and goldAdditionRate); update tradeShipGold(dist:
number) to multiply the computed base gold by this.goldMultiplier() before
converting to int (use toInt on the final value) so trade ship rewards respect
custom goldMultiplier settings.
- Around line 304-318: The tradeShipSpawnRate function can return 0 which makes
this.random.chance(0) always succeed; clamp the returned spawn-rate to a minimum
of 1 to avoid a guaranteed spawn. Update the
tradeShipSpawnRate(tradeShipSpawnRejections, numTradeShips) implementation
(referencing the baseSpawnRate, rejectionModifier, and the return value) so
after computing Math.floor((100 * rejectionModifier) / baseSpawnRate) you ensure
the result is at least 1 (e.g., use Math.max(..., 1)) before returning. Ensure
callers like this.random.chance continue to receive an integer >= 1.
---
Nitpick comments:
In `@src/core/execution/PortExecution.ts`:
- Line 12: tradeShipSpawnRejections can grow unbounded between successes; clamp
the spawn rate or guard against zero so PseudoRandom.chance() cannot be fed 0.
Fix by ensuring DefaultConfig.tradeShipSpawnRate is normalized (e.g. Math.max(1,
DefaultConfig.tradeShipSpawnRate) when used) or add a protective check in the
spawn logic before calling PseudoRandom.chance(spawnRate) to use at least 1;
update references in the spawn code that read DefaultConfig.tradeShipSpawnRate
and the call to PseudoRandom.chance(...) so the rate is never zero while
preserving the existing pity behavior tied to tradeShipSpawnRejections.
9d5b682 to
e3f3e44
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/execution/PortExecution.ts (1)
71-84: Optional: pity accumulates per roll, not per call
tradeShipSpawnRejectionsis incremented inside the loop once per failed roll, so a level-N port accrues N rejections per check period rather than 1. The spawn rate is also captured once before the loop, so the improved odds from those mid-loop increments only kick in on the next call, not the remaining rolls in the same call.If the intent is "one pity step per failed check period regardless of port level," move the increment and reset outside the loop:
♻️ Suggested restructure
shouldSpawnTradeShip(): boolean { const numTradeShips = this.mg.unitCount(UnitType.TradeShip); const spawnRate = this.mg .config() .tradeShipSpawnRate(this.tradeShipSpawnRejections, numTradeShips); for (let i = 0; i < this.port!.level(); i++) { if (this.random.chance(spawnRate)) { this.tradeShipSpawnRejections = 0; return true; } - this.tradeShipSpawnRejections++; } + this.tradeShipSpawnRejections++; return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/execution/PortExecution.ts` around lines 71 - 84, The loop in shouldSpawnTradeShip currently increments tradeShipSpawnRejections per failed roll and captures spawnRate before the loop, causing N increments for a level-N port; change the logic so spawnRate is computed once from the current tradeShipSpawnRejections, then run the level-based chance rolls (using random.chance) without changing tradeShipSpawnRejections during the loop; if any roll succeeds, set tradeShipSpawnRejections = 0 and return true, otherwise after the loop increment tradeShipSpawnRejections by 1 and return false (references: shouldSpawnTradeShip(), this.tradeShipSpawnRejections, mg.config().tradeShipSpawnRate(), random.chance(), this.port!.level()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/core/configuration/DefaultConfig.ts`:
- Around line 309-323: The returned inverse-odds from tradeShipSpawnRate can be
0 (causing PseudoRandom.chance(0) to behave as always-true); fix by ensuring the
function never returns 0—clamp to a minimum of 1. In tradeShipSpawnRate (use the
local symbols tradeShipSpawnRate, baseSpawnRate, rejectionModifier), check for
baseSpawnRate <= 0 and return 1, or compute the value then return Math.max(1,
Math.floor((100 * rejectionModifier) / baseSpawnRate)); this preserves the
intended guaranteed-pity behavior while avoiding the semantically confusing 0.
---
Nitpick comments:
In `@src/core/execution/PortExecution.ts`:
- Around line 71-84: The loop in shouldSpawnTradeShip currently increments
tradeShipSpawnRejections per failed roll and captures spawnRate before the loop,
causing N increments for a level-N port; change the logic so spawnRate is
computed once from the current tradeShipSpawnRejections, then run the
level-based chance rolls (using random.chance) without changing
tradeShipSpawnRejections during the loop; if any roll succeeds, set
tradeShipSpawnRejections = 0 and return true, otherwise after the loop increment
tradeShipSpawnRejections by 1 and return false (references:
shouldSpawnTradeShip(), this.tradeShipSpawnRejections,
mg.config().tradeShipSpawnRate(), random.chance(), this.port!.level()).
Description:
Now that pathfinding is much more efficient with hpa*, we can add more trade ships.
This PR does the following:
It's pretty difficult to balance on singleplayer so I'm sure the values will be adjusted after playtests.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
evan