Improve tradeship spawn rates fixes #1541 #1750
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe changes update the calculation of trade ship spawn rates and gold rewards by modifying method signatures and logic to consider both the number of trade ships and player-owned ports. A new method for expected spawn rate is added, and internal usage is updated to match the new parameters. Changes
Sequence Diagram(s)sequenceDiagram
participant Player
participant PortExecution
participant Config (DefaultConfig)
Player->>PortExecution: Attempt to spawn trade ship
PortExecution->>Config (DefaultConfig): tradeShipSpawnRate(numTradeShips, numPlayerPorts)
Config (DefaultConfig)-->>PortExecution: Return calculated spawn rate
PortExecution->>PortExecution: Determine if spawn occurs (random check)
PortExecution-->>Player: Result (spawn or not)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ 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)
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 comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
evanpelle
force-pushed
the
fix-tradeship-freq
branch
from
f0b50b2 to
af523bc
Compare
evanpelle
force-pushed
the
fix-tradeship-freq
branch
from
af523bc to
92cf015
Compare
![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
📜 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)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-05-19T06:00:38.007Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#784
File: src/core/game/StatsImpl.ts:125-134
Timestamp: 2025-05-19T06:00:38.007Z
Learning: In StatsImpl.ts, unused parameters in boat/stats-related methods are intentionally kept for future use and shouldn't be removed.
Applied to files:
src/core/configuration/Config.tssrc/core/execution/PortExecution.tssrc/core/configuration/DefaultConfig.ts
📚 Learning: 2025-05-31T18:15:03.445Z
Learnt from: 1brucben
PR: openfrontio/OpenFrontIO#977
File: src/core/execution/AttackExecution.ts:123-125
Timestamp: 2025-05-31T18:15:03.445Z
Learning: The removeTroops function in PlayerImpl.ts already prevents negative troop counts by using minInt(this._troops, toInt(troops)) to ensure it never removes more troops than available.
Applied to files:
src/core/configuration/Config.tssrc/core/configuration/DefaultConfig.ts
📚 Learning: 2025-05-22T23:48:51.113Z
Learnt from: jrouillard
PR: openfrontio/OpenFrontIO#848
File: src/core/execution/TradeShipExecution.ts:125-127
Timestamp: 2025-05-22T23:48:51.113Z
Learning: In TradeShipExecution, the path length is already checked in the computeNewPath() function. If the path is empty, it returns PathFindResultType.PathNotFound instead of PathFindResultType.Completed, making additional empty path checks unnecessary when handling the Completed case.
Applied to files:
src/core/execution/PortExecution.ts
📚 Learning: 2025-06-22T05:48:19.241Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#786
File: src/client/TerritoryPatternsModal.ts:337-338
Timestamp: 2025-06-22T05:48:19.241Z
Learning: In src/client/TerritoryPatternsModal.ts, the bit shifting operators (<<) used in coordinate calculations with decoder.getScale() are intentional and should not be changed to multiplication. The user scottanderson confirmed this is functioning as intended.
Applied to files:
src/core/execution/PortExecution.ts
🧬 Code Graph Analysis (1)
src/core/configuration/DefaultConfig.ts (1)
src/core/game/Game.ts (1)
Gold(16-16)
🔇 Additional comments (5)
Dockerfile (1)
53-54: Good addition for version traceability.Adding the GIT_COMMIT environment variable to the final stage ensures version information is available at runtime, which helps with debugging and deployment tracking.
src/core/execution/PortExecution.ts (1)
81-84: Implementation correctly updated to match interface.The code properly calculates the player's port count and passes both
numTradeShipsandnumPlayerPortsto the spawn rate method, aligning with the new signature and game mechanics requirements.src/core/configuration/DefaultConfig.ts (3)
344-351: Well-designed diminishing returns helper.The
goldMultipliermethod implements sensible economics with a 0.95 diminishing factor and 0.1 minimum value per additional port. This prevents excessive scaling while still rewarding port expansion.
354-357: Balanced trade ship gold adjustment.Halving the base gold from 50000 to 25000 while applying the port multiplier maintains economic balance, ensuring the spawn rate changes don't dramatically alter gold income.
329-329: Please verify the doubled train spawn rate is intentionalIt looks like the multiplier in trainSpawnRate(numberOfStations) was increased from 20 to 40, so:
- In src/core/configuration/DefaultConfig.ts (line 329), you now return
Math.round(40 * √numberOfStations)(capped at 1400) instead of20 * √numberOfStations.- In src/core/execution/TrainStationExecution.ts, that higher value flows into
shouldSpawnTrain, boosting the per‐tick chance (for each unit level) of spawning a train.I didn’t find any tests or documentation covering this change. Please confirm:
- That doubling the multiplier is deliberate.
- There’s no risk of overwhelming the system with too many trains.
- Any additional test coverage or documentation updates are handled.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
evanpelle
force-pushed
the
fix-tradeship-freq
branch
from
92cf015 to
24fc638
Compare
github-project-automation
bot
moved this from Development
to Complete
in OpenFront Release Management
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
The previous behavior had trade ship spawn rate decrease as the number of trade ships on the map increased. This was problematic because large players with many ports could starve out smaller players.
This change has the trade ship spawn rate decrease as number of ports increases, but increases the value of trade ships to compensate. It also decreases as a function of number of tradeships, but it is less of a factor as before.
Gold per trade ship scales with goldMultiplier(), but tradeship spawn rate decreases with sqrt(goldMultiplier()), this ensures that more ports => more gold.
After roughly 10-15 ports the increase trails off, preventing large players from gaining too much gold.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
evan