Update Trade ship meta fixes #1541 #1772
Conversation
WalkthroughThe Config interface now requires two parameters for tradeShipSpawnRate. DefaultConfig implements a ports-aware spawn-rate and adjusts tradeShipGold. PortExecution passes both current trade ships and player port count. Util adds an exported sigmoid function. Changes
Sequence Diagram(s)sequenceDiagram
participant Player
participant PortExecution
participant Config
Player->>PortExecution: tick()
PortExecution->>PortExecution: determine numTradeShips, numPlayerPorts
PortExecution->>Config: tradeShipSpawnRate(numTradeShips, numPlayerPorts)
Config-->>PortExecution: spawnRate
PortExecution->>PortExecution: loop per port level, random.chance(spawnRate)
PortExecution-->>Player: spawn decision (true/false)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Suggested reviewers
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/core/configuration/DefaultConfig.ts (1)
360-367: Add method documentation for clarity.The expected spawn rate calculation looks correct and follows good composition by reusing
tradeShipSpawnRate. However, the method would benefit from JSDoc comments explaining what it calculates.Add documentation like this:
+ /** + * Calculates the expected number of trade ships spawned per tick across all player ports. + * @param numTradeShips Current number of trade ships in the game + * @param numPlayerPorts Total number of player-owned ports + * @returns Expected trade ships spawned per tick + */ expectedTradeShipSpawnRate( numTradeShips: number, numPlayerPorts: number, ): number {src/core/execution/PortExecution.ts (1)
81-85: Spawn rate now factors player ports — good. Add guardrails for probability bounds.Nice improvement. To be safe against misconfigured configs, clamp probability before using random.chance.
Proposed tweak:
- const spawnRate = this.mg - .config() - .tradeShipSpawnRate(numTradeShips, numPlayerPorts); + const spawnRateRaw = this.mg.config().tradeShipSpawnRate(numTradeShips, numPlayerPorts); + const spawnRate = Math.max(0, Math.min(1, spawnRateRaw));Please add/adjust tests covering:
- More player ports should not reduce spawn probability, all else equal.
- Higher global trade ship count reduces spawn probability (if that’s the intent).
- Edge cases: returned spawn rate <= 0 and >= 1 behave as expected.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Dockerfile(1 hunks)src/core/configuration/Config.ts(1 hunks)src/core/configuration/DefaultConfig.ts(2 hunks)src/core/execution/PortExecution.ts(1 hunks)
🔥 Files not summarized due to errors (1)
- src/core/configuration/DefaultConfig.ts: Error: Server error: no LLM provider could handle the message
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
src/core/configuration/DefaultConfig.ts (3)
329-329: LGTM! Clean formatting.The formatting improvement makes the calculation more readable without changing the logic.
344-348: LGTM! Improved readability.The variable extraction makes the trade ship gold calculation clearer and easier to understand.
350-358: All call sites updated for the new signatureAll uses of tradeShipSpawnRate now pass both numTradeShips and numPlayerPorts. No single-argument calls remain.
• DefaultConfig.ts – method implementation and internal call use two args
• PortExecution.ts – spawn logic calls with two args
• Config.ts – interface updated to two parameters
• DevConfig.ts – old one-arg stub is commented out, so it falls back to DefaultConfigNo further changes needed.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
evanpelle
force-pushed
the
fix-tradeship-freq
branch
from
24fc638 to
39005ab
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/core/configuration/DefaultConfig.ts (1)
329-329: Train spawn scaling doubled — please confirm balance intent and extract constantsChanging 20 → 40 doubles spawn rate scaling. Is this intentional for balance? If yes, extract magic numbers to named constants for clarity and easy tuning.
For example:
// near top or config constants block const TRAIN_SPAWN_SCALE = 40; const TRAIN_SPAWN_CAP = 1400; // here return Math.min(TRAIN_SPAWN_CAP, Math.round(TRAIN_SPAWN_SCALE * Math.pow(numberOfStations, 0.5)));Consider adding a small test to lock expected values for a few inputs (e.g., 1, 4, 9, 100).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
resources/maps/world/manifest.json(0 hunks)src/core/configuration/Config.ts(1 hunks)src/core/configuration/DefaultConfig.ts(2 hunks)src/core/execution/PortExecution.ts(1 hunks)src/core/game/GameImpl.ts(1 hunks)
💤 Files with no reviewable changes (1)
- resources/maps/world/manifest.json
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/configuration/Config.ts
- src/core/execution/PortExecution.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/core/game/GameImpl.ts (1)
src/core/game/GameView.ts (1)
myPlayer(501-504)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (1)
src/core/configuration/DefaultConfig.ts (1)
358-371: AlltradeShipSpawnRatecalls now use the 2-arg signature
I ran an AST search and a regex scan—there are no single-argument calls left.
Affected files:
- src/core/execution/PortExecution.ts
- src/core/configuration/DefaultConfig.ts
- src/core/configuration/Config.ts
(The old one-arg stub in DevConfig.ts is only in comments.)
evanpelle
force-pushed
the
fix-tradeship-freq
branch
from
39005ab to
604ecce
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/core/configuration/DefaultConfig.ts (1)
348-355: Gold multiplier grows unbounded with many ports — use bounded geometric sumKeeping a per-port floor of 0.1 breaks geometric decay and makes totalMultiplier grow roughly linearly for large numPorts. This can blow up the economy.
Apply this refactor using the closed-form geometric sum (bounded, O(1)):
- let totalMultiplier = 1; - for (let i = 0; i < numPorts; i++) { - totalMultiplier += Math.max( - 0.1, - basePortBonus * Math.pow(diminishingFactor, i), - ); - } + // Geometric sum: 1 + basePortBonus * (1 - diminishingFactor^numPorts) / (1 - diminishingFactor) + const totalMultiplier = + 1 + + basePortBonus * + (1 - + Math.pow(diminishingFactor, Math.max(0, Math.floor(numPorts)))) / + (1 - diminishingFactor);If you want a hard cap, wrap with within(totalMultiplier, 1, cap).
🧹 Nitpick comments (3)
src/core/Util.ts (1)
291-298: Fix misleading sigmoid comment (increasing 0 → 1, not 1 → 0)The logistic form used increases from 0 to 1 as value grows. The current comment says “Goes from 1 => 0,” which is the inverse behavior. This can confuse future readers.
Apply this diff to correct the comment:
-// Goes from 1 => 0 +// Standard logistic: monotonically increases from 0 => 1 as value increases. +// Use `1 - sigmoid(...)` if you need a decreasing curve.src/core/configuration/DefaultConfig.ts (2)
368-371: Document the shape of the base factor (decaying with more trade ships)Nit: add a short note that this returns a 0..1 factor that decreases as numTradeShips grows; clarify why decayRate = ln(2)/10 (half-life of ~10 ships relative to midpoint).
Example doc (outside current diff range):
/** * Returns a [0..1] factor that decreases as numTradeShips increases. * decayRate = ln(2)/halfLife; midpoint = ships at which factor ≈ 0.5. */
373-377: Clarify comment; function decreases with more ports to create diminishing returnsThis multiplier decreases as ports increase (1/(1 + a·ports)). Net effect: per‑port spawn probability drops, and expected spawns across all ports grow sublinearly (~O(sqrt(ports)) with the current combination).
Apply this comment tweak:
- // Higher number => faster expected number of trade ships levels off + // Diminishing returns: per-port factor decreases as ports grow. + // With the current combined formula, expected spawns across all ports grow sublinearly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
resources/maps/world/manifest.json(0 hunks)src/core/Util.ts(1 hunks)src/core/configuration/Config.ts(1 hunks)src/core/configuration/DefaultConfig.ts(3 hunks)src/core/execution/PortExecution.ts(1 hunks)src/core/game/GameImpl.ts(1 hunks)
💤 Files with no reviewable changes (1)
- resources/maps/world/manifest.json
🚧 Files skipped from review as they are similar to previous changes (3)
- src/core/execution/PortExecution.ts
- src/core/game/GameImpl.ts
- src/core/configuration/Config.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/core/configuration/DefaultConfig.ts (1)
src/core/Util.ts (1)
sigmoid(292-298)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Deploy to openfront.dev
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
src/core/configuration/DefaultConfig.ts (2)
27-27: Import of sigmoid looks goodCentralizing the logistic in Util and importing it here is a good move for reuse and consistency.
329-329: Confirm intended train spawn increase
We didn’t find any other references to the old 20 × √n formula or hard-coded tests/docs for trainSpawnRate. Please verify this 2× buff is deliberate and update any affected tests or documentation.Points to review:
- src/core/configuration/DefaultConfig.ts (line 329)
return Math.min(1400, Math.round(40 * Math.pow(numberOfStations, 0.5)));
Ensure the multiplier change from 20 → 40 is intended.- src/core/configuration/Config.ts (line 134)
Interface declaration; update comments if needed.- src/core/execution/TrainStationExecution.ts (line 52)
Caller of trainSpawnRate; adjust or add tests for new spawn rate.- Tests & docs
No matches found. Manually check for any scenarios depending on the old rate.
| // Probability of trade ship spawn = 1 / tradeShipSpawnRate | ||
| tradeShipSpawnRate(numTradeShips: number, numPlayerPorts: number): number { | ||
| const combined = Math.sqrt( | ||
| this.tradeShipBaseSpawn(numTradeShips) * | ||
| this.tradeShipPortMultiplier(numPlayerPorts), | ||
| ); | ||
|
|
||
| return Math.floor(10 / combined); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Sanitize inputs and guard against degenerate rates; keep function pure
Clamp inputs to non‑negative integers and ensure we never return 0 (future-proof against formula changes). This keeps the function deterministic and side‑effect free.
Apply this diff:
- tradeShipSpawnRate(numTradeShips: number, numPlayerPorts: number): number {
- const combined = Math.sqrt(
- this.tradeShipBaseSpawn(numTradeShips) *
- this.tradeShipPortMultiplier(numPlayerPorts),
- );
-
- return Math.floor(10 / combined);
- }
+ tradeShipSpawnRate(numTradeShips: number, numPlayerPorts: number): number {
+ // sanitize inputs
+ const ships = Math.max(0, Math.floor(numTradeShips));
+ const ports = Math.max(0, Math.floor(numPlayerPorts));
+
+ const combined = Math.sqrt(
+ this.tradeShipBaseSpawn(ships) * this.tradeShipPortMultiplier(ports),
+ );
+ // combined ∈ (0, 1], but guard to avoid 1/0 if formula changes in the future.
+ const rate = Math.max(1, Math.floor(10 / Math.max(1e-9, combined)));
+ return rate;
+ }Also verify all callers updated to the new 2‑arg signature:
🏁 Script executed:
#!/usr/bin/env bash
# 1) Find calls still passing a single argument
ast-grep --pattern $'$_ . tradeShipSpawnRate($_)' || true
# 2) Show all call sites with context
rg -n -A3 -B3 'tradeShipSpawnRate\s*\('Length of output: 2734
🏁 Script executed:
#!/usr/bin/env bash
# Show implementation of tradeShipBaseSpawn
rg -n -C3 "tradeShipBaseSpawn" src/core/configuration/DefaultConfig.ts
# Show implementation of tradeShipPortMultiplier
rg -n -C3 "tradeShipPortMultiplier" src/core/configuration/DefaultConfig.tsLength of output: 1280
Enforce non-negative integer inputs and guard against zero division in tradeShipSpawnRate
All call sites already use the two-arg signature, but the formula can overflow or flip sign if someone passes a negative or non-integer value (or if the internal formula ever returns 0). Let’s sanitize and clamp early, and never return 0 or Infinity.
• File: src/core/configuration/DefaultConfig.ts
Update tradeShipSpawnRate as follows:
tradeShipSpawnRate(numTradeShips: number, numPlayerPorts: number): number {
- const combined = Math.sqrt(
- this.tradeShipBaseSpawn(numTradeShips) *
- this.tradeShipPortMultiplier(numPlayerPorts),
- );
-
- return Math.floor(10 / combined);
+ // sanitize inputs to non-neg integers
+ const ships = Math.max(0, Math.floor(numTradeShips));
+ const ports = Math.max(0, Math.floor(numPlayerPorts));
+
+ const combined = Math.sqrt(
+ this.tradeShipBaseSpawn(ships) * this.tradeShipPortMultiplier(ports),
+ );
+ // guard against combined→0 or formula changes
+ const safeCombined = Math.max(1e-9, combined);
+ // spawnRate = floor(10 / combined) but at least 1
+ return Math.max(1, Math.floor(10 / safeCombined));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Probability of trade ship spawn = 1 / tradeShipSpawnRate | |
| tradeShipSpawnRate(numTradeShips: number, numPlayerPorts: number): number { | |
| const combined = Math.sqrt( | |
| this.tradeShipBaseSpawn(numTradeShips) * | |
| this.tradeShipPortMultiplier(numPlayerPorts), | |
| ); | |
| return Math.floor(10 / combined); | |
| } | |
| // Probability of trade ship spawn = 1 / tradeShipSpawnRate | |
| tradeShipSpawnRate(numTradeShips: number, numPlayerPorts: number): number { | |
| // sanitize inputs to non-negative integers | |
| const ships = Math.max(0, Math.floor(numTradeShips)); | |
| const ports = Math.max(0, Math.floor(numPlayerPorts)); | |
| const combined = Math.sqrt( | |
| this.tradeShipBaseSpawn(ships) * this.tradeShipPortMultiplier(ports), | |
| ); | |
| // guard against combined → 0 or future formula changes | |
| const safeCombined = Math.max(1e-9, combined); | |
| // spawnRate = floor(10 / combined) but at least 1 | |
| return Math.max(1, Math.floor(10 / safeCombined)); | |
| } |
🤖 Prompt for AI Agents
In src/core/configuration/DefaultConfig.ts around lines 358 to 366, the
tradeShipSpawnRate method must validate and sanitize its inputs and the
intermediate result to prevent negative/non-integer values and
divide-by-zero/Infinity returns; clamp numTradeShips and numPlayerPorts to
integers >= 0 (e.g. Math.max(0, Math.floor(...))) before using them, compute
combined as currently done, then guard combined to be >= a small positive value
(e.g. Math.max(combined, 1e-6)) so you never divide by zero, compute the rate
and ensure the final return is a positive integer (never 0 or Infinity) by
clamping to a minimum of 1 with Math.max(1, Math.floor(...)); update only this
method to apply these input sanitizations and final clamping.
604ecce to
2ce55d1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/core/configuration/DefaultConfig.ts (3)
345-356: Gold multiplier grows unbounded with many ports — use bounded geometric sumPer-port floor (0.05) breaks geometric decay and makes multiplier grow ~linearly with many ports. This can blow up the economy.
Use the closed-form geometric sum for additional ports, and optionally cap the total multiplier.
- const baseGold = Math.floor(25000 + 100 * dist); - const basePortBonus = 0.5; - const diminishingFactor = 0.9; - let totalMultiplier = 1; - for (let i = 1; i < numPorts; i++) { - totalMultiplier += Math.max( - 0.05, - basePortBonus * Math.pow(diminishingFactor, i - 1), - ); - } - return BigInt(Math.floor(baseGold * totalMultiplier)); + const baseGold = Math.floor(25000 + 100 * dist); + const basePortBonus = 0.5; + const diminishingFactor = 0.9; + const additionalPorts = Math.max(0, Math.floor(numPorts) - 1); + // Geometric sum of diminishing bonuses for additional ports (bounded) + const totalMultiplier = + 1 + + basePortBonus * + (1 - Math.pow(diminishingFactor, additionalPorts)) / + (1 - diminishingFactor); + // Optional hard cap to avoid runaway economy; tune as needed + const cappedMultiplier = within(totalMultiplier, 1, 3.5); + return BigInt(Math.floor(baseGold * cappedMultiplier));
358-366: Sanitize inputs and guard against tiny ‘combined’ to avoid 1/0 and non-integer returnsClamp inputs to non-negative integers and guard the denominator. Ensure we never return 0.
- tradeShipSpawnRate(numTradeShips: number, numPlayerPorts: number): number { - const combined = Math.sqrt( - this.tradeShipBaseSpawn(numTradeShips) * - this.tradeShipPortMultiplier(numPlayerPorts), - ); - - return Math.floor(10 / combined); - } + tradeShipSpawnRate(numTradeShips: number, numPlayerPorts: number): number { + // sanitize inputs + const ships = Math.max(0, Math.floor(numTradeShips)); + const ports = Math.max(0, Math.floor(numPlayerPorts)); + + const combined = Math.sqrt( + this.tradeShipBaseSpawn(ships) * this.tradeShipPortMultiplier(ports), + ); + const safeCombined = Math.max(1e-9, combined); + return Math.max(1, Math.floor(10 / safeCombined)); + }
379-386: Avoid floor-quantization bias in expected spawns; compute continuous expectationUsing
1 / floor(10 / combined)underestimates expected spawns and adds step artifacts. Use the continuous form directly with sanitized inputs.- const spawnRate = this.tradeShipSpawnRate(numTradeShips, numPlayerPorts); - const expectedSpawn = numPlayerPorts * (1 / spawnRate); - return expectedSpawn; + const ships = Math.max(0, Math.floor(numTradeShips)); + const ports = Math.max(0, Math.floor(numPlayerPorts)); + const combined = Math.sqrt( + this.tradeShipBaseSpawn(ships) * this.tradeShipPortMultiplier(ports), + ); + // Probability per port per tick (continuous) ≈ combined / 10 + return ports * (combined / 10);
🧹 Nitpick comments (2)
src/core/configuration/DefaultConfig.ts (2)
329-329: Train spawn scaling doubled — verify balance; extract magic numberDoubling to 40 changes pacing a lot and hits the 1400 cap at ~1225 stations. Please verify with telemetry/tests. Also, avoid magic numbers inline.
Apply this small refactor for clarity:
- return Math.min(1400, Math.round(40 * Math.pow(numberOfStations, 0.5))); + const SCALE = 40; + return Math.min(1400, Math.round(SCALE * Math.sqrt(numberOfStations)));
373-377: Comment is misleading; function creates diminishing returns, not “levels off”
1 / (1 + k * ports)decreases with more ports. Expected spawns (considering multiplication by ports later) grow sublinearly (~sqrt), not “level off.”- // Higher number => faster expected number of trade ships levels off + // More ports => diminishing returns; expected spawns grow sublinearly
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
resources/maps/world/manifest.json(0 hunks)src/core/Util.ts(1 hunks)src/core/configuration/Config.ts(1 hunks)src/core/configuration/DefaultConfig.ts(3 hunks)src/core/execution/PortExecution.ts(1 hunks)src/core/game/GameImpl.ts(1 hunks)
💤 Files with no reviewable changes (1)
- resources/maps/world/manifest.json
🚧 Files skipped from review as they are similar to previous changes (4)
- src/core/Util.ts
- src/core/game/GameImpl.ts
- src/core/configuration/Config.ts
- src/core/execution/PortExecution.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/core/configuration/DefaultConfig.ts (1)
src/core/Util.ts (1)
sigmoid(292-298)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (2)
src/core/configuration/DefaultConfig.ts (2)
27-27: Import of sigmoid looks good
sigmoidis used below in trade-ship spawning. No issues with the import.
368-372: Base spawn curve via sigmoid — OKMonotone, bounded in (0,1], and numerically safe with chosen parameters.
evanpelle
force-pushed
the
fix-tradeship-freq
branch
from
2ce55d1 to
51852cd
Compare
evanpelle
force-pushed
the
fix-tradeship-freq
branch
from
51852cd to
e8ca96f
Compare
evanpelle
force-pushed
the
fix-tradeship-freq
branch
from
e8ca96f to
35dca07
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/core/configuration/DefaultConfig.ts (2)
346-356: Per-port floor causes unbounded (linear) gold growth — replace loop with bounded geometric sumUsing Math.max(..., 0.05) breaks geometric decay and makes totalMultiplier grow roughly linearly with many ports. With 12+ ports, each extra port adds a fixed 0.05, which can inflate the economy.
Use the closed-form geometric sum (bounded) and optionally cap to the asymptote (3.5 for basePortBonus=0.5, diminishingFactor=0.8). Also coerce numPorts to a safe non-negative integer.
Apply this diff:
tradeShipGold(dist: number, numPorts: number): Gold { const baseGold = Math.floor(50_000 + 100 * dist); const basePortBonus = 0.5; const diminishingFactor = 0.8; - let totalMultiplier = 1; - for (let i = 1; i < numPorts; i++) { - totalMultiplier += Math.max( - 0.05, - basePortBonus * Math.pow(diminishingFactor, i - 1), - ); - } - return BigInt(Math.floor(baseGold * totalMultiplier)); + const ports = Math.max(0, Math.floor(numPorts)); + // Sum_{i=1..ports-1} basePortBonus * diminishingFactor^(i-1) + const added = + ports > 1 + ? (basePortBonus * + (1 - Math.pow(diminishingFactor, ports - 1))) / + (1 - diminishingFactor) + : 0; + // Bounded and smooth; asymptote is 1 + basePortBonus/(1 - diminishingFactor) = 3.5 + const totalMultiplier = within(1 + added, 1, 3.5); + return BigInt(Math.floor(baseGold * totalMultiplier)); }Side note: If dist can be negative in any edge case, clamp it to >= 0 before use.
359-369: Sanitize inputs and guard against combined→0 to avoid Infinity spawn ratesAs numTradeShips and/or numPlayerPorts grow large, combined can approach 0, making Math.floor(10 / combined) overflow to Infinity. Also, allow only non-negative integers for inputs to keep the function deterministic.
Apply this diff:
// Probability of trade ship spawn = 1 / tradeShipSpawnRate tradeShipSpawnRate(numTradeShips: number, numPlayerPorts: number): number { - // Geometric mean of base spawn rate and port multiplier - const combined = Math.sqrt( - this.tradeShipBaseSpawn(numTradeShips) * - this.tradeShipPortMultiplier(numPlayerPorts), - ); - - return Math.floor(10 / combined); + // Geometric mean of base spawn rate and port multiplier + const ships = Math.max(0, Math.floor(numTradeShips)); + const ports = Math.max(0, Math.floor(numPlayerPorts)); + const combined = Math.sqrt( + this.tradeShipBaseSpawn(ships) * this.tradeShipPortMultiplier(ports), + ); + // Guard against combined→0; keep output a finite positive integer + const safeCombined = Math.max(1e-9, combined); + return Math.max(1, Math.floor(10 / safeCombined)); }Verification script: ensure all call sites use the new two-arg signature (no lingering single-arg calls).
#!/usr/bin/env bash # 1) Show any single-argument calls (should be none) ast-grep --pattern $'tradeShipSpawnRate($_)' || true # 2) List all usages with context for a manual scan rg -n -A3 -B3 'tradeShipSpawnRate\s*\('
🧹 Nitpick comments (2)
src/core/configuration/DefaultConfig.ts (2)
370-374: Optional: clamp base spawn away from 0 for numeric safety1 - sigmoid(...) can underflow to 0 at extreme ship counts, which propagates to combined≈0. You already guard in the caller after the diff above; adding a local clamp keeps this helper safer and reusable.
private tradeShipBaseSpawn(numTradeShips: number): number { const decayRate = Math.log(2) / 30; - return 1 - sigmoid(numTradeShips, decayRate, 100); + return within(1 - sigmoid(numTradeShips, decayRate, 100), 1e-9, 1); }
375-381: Minor: make intent explicit and sanitize locally (or assert input contracts)Return never reaches 0 for finite inputs, but clarity helps. Consider coercing ports here too, or add a brief contract comment. Also, naming it portScale makes it distinct from the sigmoid’s decayRate.
private tradeShipPortMultiplier(numPlayerPorts: number): number { - // Higher number => faster expected number of trade ships levels off - // This decays gradually to prevent the scenario where more ports => fewer trade ships - // Expected number of trade ships is proportional to numPlayerPorts * tradeShipPortMultiplier - const decayRate = 0.1; - return 1 / (1 + decayRate * numPlayerPorts); + // Expected spawns ∝ P / (1 + kP). Monotone in P, saturates to 1/k as P→∞. + const k = 0.1; + const ports = Math.max(0, Math.floor(numPlayerPorts)); + return 1 / (1 + k * ports); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/core/Util.ts(1 hunks)src/core/configuration/Config.ts(1 hunks)src/core/configuration/DefaultConfig.ts(2 hunks)src/core/execution/PortExecution.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/core/configuration/Config.ts
- src/core/Util.ts
- src/core/execution/PortExecution.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/core/configuration/DefaultConfig.ts (1)
src/core/Util.ts (1)
sigmoid(291-297)
🔇 Additional comments (1)
src/core/configuration/DefaultConfig.ts (1)
27-27: Good addition: sigmoid import is appropriate and usedImporting sigmoid here is correct and keeps math helpers centralized in Util.ts.
evanpelle
force-pushed
the
fix-tradeship-freq
branch
from
f75c0c8 to
35dca07
Compare
evanpelle
force-pushed
the
fix-tradeship-freq
branch
from
35dca07 to
1535fd2
Compare
drillskibo
added
the
Feature - Simulation
|
@evanpelle this is tagged as a v25 release blocker, what's the status of this PR? |
|
Replaced by #1872 |
github-project-automation
bot
moved this from Development
to Complete
in OpenFront Release Management
Description:
v24 put a cap on the number of trade ships, while this helped with performance it caused a major issue where large players with many ports could starve out players with few ports, sometimes they would see almost no trade ships.
This meta has two factors:
The two factors are geometrically averaged to produce final trade ship spawn probability. By reducing the spawn rate for players with many ports, this prevents large players from starving small players.
This change also puts a floor of 5% gold increase per additional port. One problem with v24 was that players with many ports (50+) saw practically no benefit from adding ports (the # trade ships was at the cap and the port bonus asymptoted to zero). So now at least there will always be a benefit to increasing ports, even if minor.
Changes base port bonus from 25% to 50%, but decreased diminishing factor from .9=>.8. This change encourages smaller players to build more ports, but the faster dropoff hurts players with more ports, preventing runaway gold.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
evan