Skip to content

Conversation

@Restart2008
Copy link

@Restart2008 Restart2008 commented Oct 26, 2025

…e Posts

Description:

Added a new hyperbolic formula for defensePostDefenseBuff with diminishing returns and upgradability for defense posts.

Variables
Y = total defense bonus (constant)
X = level of defense post (constant)
B = base constant of level 1 defense post (constant)
M = asymptote of the function
K = changes steepness of the curve

Formula: Y = B + M * ((X - 1) / (X - 1 + K))

Please complete the following:

  • I have added screenshots for all UI updates
  • I process any text displayed to the user through translateText() and I've added it to the en.json file
  • I have added relevant tests to the test directory
  • I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced

Please put your Discord username so you can be contacted if a bug or regression is found:

restart

@Restart2008 Restart2008 requested a review from a team as a code owner October 26, 2025 02:33
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 26, 2025

Walkthrough

The Config API's defensePostDefenseBonus gains a required level parameter. DefaultConfig implements a level-based bonus formula and call sites now pass the defense post unit level. The DefensePost unit entry is marked upgradable: true.

Changes

Cohort / File(s) Summary
Config interface
src/core/configuration/Config.ts
Method signature changed to defensePostDefenseBonus(level: number): number (was parameterless).
DefaultConfig implementation & unit info
src/core/configuration/DefaultConfig.ts
defensePostDefenseBonus(level) now validates level >= 1 and computes a level-scaled bonus (replacing a fixed value); callers updated to pass dp.unit.level(); unitInfo.DefensePost now has upgradable: true.

Sequence Diagram(s)

sequenceDiagram
    participant Attacker as Attack logic
    participant Config as DefaultConfig
    participant Unit as DefensePost unit

    Attacker->>Unit: find defense post (dp)
    Note over Unit: dp.unit.level() → level
    Attacker->>Config: defensePostDefenseBonus(level)
    Config-->>Attacker: numeric bonus (level-scaled)
    Attacker->>Attacker: apply bonus in damage calculation
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Verify all call sites updated to pass level.
  • Inspect the level-scaling formula for correctness and edge-case handling (level < 1).
  • Confirm upgradable: true works with upgrade/construction logic.

Suggested labels

Balance Tweak, Feature - AI

A post that once gave five with ease,
Now grows with every level's breeze,
Callers pass the level in line,
Upgrades flagged and numbers climb. 🛡️

Pre-merge checks

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The pull request title "feat: Implement hyperbolic defense bonus and upgradability for Defens…" directly corresponds to the main changes in the changeset. The code modifications update the defensePostDefenseBonus method signature to accept a level parameter and implement a hyperbolic formula with diminishing returns, while also adding upgradable: true to the DefensePost unit configuration. The title is specific enough for a teammate reviewing the repository history to understand that this PR introduces both a new calculation formula and the ability to upgrade defense posts.
Description Check ✅ Passed The pull request description clearly explains the changes being implemented, providing the mathematical formula (Y = B + M * ((X - 1) / (X - 1 + K))), defining all relevant variables, and explaining that the changes add a hyperbolic formula for defense bonuses with diminishing returns and upgradability for defense posts. This matches the code changes shown in the raw summary, where defensePostDefenseBonus now accepts a level parameter and implements the described calculation logic. The description is specific and directly related to the changeset content.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9aa95e0 and 3e03c28.

📒 Files selected for processing (1)
  • src/core/configuration/DefaultConfig.ts (3 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: ElMelchizedek
Repo: openfrontio/OpenFrontIO PR: 1876
File: src/core/execution/FakeHumanExecution.ts:551-568
Timestamp: 2025-08-20T10:06:20.469Z
Learning: Defense posts in OpenFrontIO provide area-of-effect defensive bonuses (5x defense, 3x speed) to any tile being attacked within 30 tiles of the post. They should be positioned with strategic depth - close enough to cover borders but far enough back (around 10-20 tiles) to avoid immediate capture by attackers.
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: src/core/execution/PlayerExecution.ts:40-52
Timestamp: 2025-08-29T16:16:11.309Z
Learning: In OpenFrontIO PlayerExecution.ts, when Defense Posts are captured due to tile ownership changes, the intended behavior is to first call u.decreaseLevel() to downgrade them, then still transfer them to the capturing player via captureUnit(). This is not a bug - Defense Posts should be both downgraded and captured, not destroyed outright.
📚 Learning: 2025-08-29T16:16:11.309Z
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: src/core/execution/PlayerExecution.ts:40-52
Timestamp: 2025-08-29T16:16:11.309Z
Learning: In OpenFrontIO PlayerExecution.ts, when Defense Posts are captured due to tile ownership changes, the intended behavior is to first call u.decreaseLevel() to downgrade them, then still transfer them to the capturing player via captureUnit(). This is not a bug - Defense Posts should be both downgraded and captured, not destroyed outright.

Applied to files:

  • src/core/configuration/DefaultConfig.ts
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.

Applied to files:

  • src/core/configuration/DefaultConfig.ts
📚 Learning: 2025-08-23T07:48:19.060Z
Learnt from: ElMelchizedek
Repo: openfrontio/OpenFrontIO PR: 1876
File: src/core/execution/FakeHumanExecution.ts:470-473
Timestamp: 2025-08-23T07:48:19.060Z
Learning: In FakeHumanExecution.ts DefensePost placement logic, returning -Infinity from structureSpawnTileValue when no sampled border tiles neighbor enemies is intentional. The logic samples up to 50 border tiles as a heuristic - if none are adjacent to enemies, it assumes DefensePost placement is unnecessary and aborts the entire placement attempt rather than continuing to evaluate individual tiles.

Applied to files:

  • src/core/configuration/DefaultConfig.ts
📚 Learning: 2025-05-31T18:15:03.445Z
Learnt from: 1brucben
Repo: openfrontio/OpenFrontIO PR: 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/DefaultConfig.ts
📚 Learning: 2025-10-27T09:47:26.395Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:770-795
Timestamp: 2025-10-27T09:47:26.395Z
Learning: In src/core/execution/FakeHumanExecution.ts, the selectSteamrollStopTarget() method intentionally allows MIRV targeting when secondHighest city count is 0 (e.g., nuclear endgame scenarios where structures are destroyed). This is valid game design—"if you can afford it, you're good to go"—and should not be flagged as requiring a guard condition.

Applied to files:

  • src/core/configuration/DefaultConfig.ts
📚 Learning: 2025-06-05T02:34:45.899Z
Learnt from: Egraveline
Repo: openfrontio/OpenFrontIO PR: 1012
File: src/core/execution/UpgradeStructureExecution.ts:49-54
Timestamp: 2025-06-05T02:34:45.899Z
Learning: In the upgrade system, gold deduction for structure upgrades is handled internally by the `upgradeUnit` method in PlayerImpl, not in the UpgradeStructureExecution class. The UpgradeStructureExecution only needs to check if the player has sufficient gold before calling `upgradeUnit`.

Applied to files:

  • src/core/configuration/DefaultConfig.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.

Applied to files:

  • src/core/configuration/DefaultConfig.ts
📚 Learning: 2025-08-20T10:06:20.469Z
Learnt from: ElMelchizedek
Repo: openfrontio/OpenFrontIO PR: 1876
File: src/core/execution/FakeHumanExecution.ts:551-568
Timestamp: 2025-08-20T10:06:20.469Z
Learning: Defense posts in OpenFrontIO provide area-of-effect defensive bonuses (5x defense, 3x speed) to any tile being attacked within 30 tiles of the post. They should be positioned with strategic depth - close enough to cover borders but far enough back (around 10-20 tiles) to avoid immediate capture by attackers.

Applied to files:

  • src/core/configuration/DefaultConfig.ts
🧬 Code graph analysis (1)
src/core/configuration/DefaultConfig.ts (1)
src/core/game/UnitImpl.ts (1)
  • level (406-408)
⏰ 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: Deploy to openfront.dev
🔇 Additional comments (3)
src/core/configuration/DefaultConfig.ts (3)

321-324: LGTM!

The comment clarifies that speed bonus remains fixed while defense bonus scales with level.


672-682: LGTM!

The call site correctly passes the defense post's level to compute the defense bonus. The level-based multiplier integrates cleanly with the existing defense logic.


309-319: Verify the constant against the PR description and game design documentation.

The code shows maxIncrease = 1.5 (confirmed at line 316). The review comment claims the PR description specifies M = 10, but this documentation does not appear in the repository, so I cannot confirm that claim.

To resolve this:

  1. Check the actual GitHub PR description for the intended formula parameters
  2. Check any game design document or specification for defense bonus scaling
  3. If maxIncrease = 10 is correct, apply the suggested change
  4. If maxIncrease = 1.5 is correct, update any external documentation to match

The implementation itself is structurally sound—input validation is in place, the formula is clean, and the call site correctly passes the unit level.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (1)
src/core/configuration/DefaultConfig.ts (1)

310-312: Consider adding explanatory comments for the formula constants.

The magic numbers reduce readability. Add a brief comment explaining the hyperbolic curve behavior and why these specific values were chosen.

Example:

 defensePostDefenseBonus(level: number): number {
+  // Hyperbolic formula providing diminishing returns:
+  // Level 1: 5x defense, Level 2: ~8.3x, Level 3: 10x, Max (∞): 15x
   const baseValue = 5;
   const maxIncrease = 10;
   const k = 2;
   return baseValue + maxIncrease * ((level - 1) / (level - 1 + k));
 }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3478b3a and 668a995.

📒 Files selected for processing (2)
  • src/core/configuration/Config.ts (1 hunks)
  • src/core/configuration/DefaultConfig.ts (3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/core/configuration/Config.ts (2)
src/core/game/UnitImpl.ts (1)
  • level (406-408)
src/core/game/GameView.ts (1)
  • level (167-169)
src/core/configuration/DefaultConfig.ts (2)
src/core/game/UnitImpl.ts (1)
  • level (406-408)
src/core/game/GameView.ts (1)
  • level (167-169)
🔇 Additional comments (2)
src/core/configuration/DefaultConfig.ts (1)

672-673: LGTM!

The call site correctly passes the unit level to compute the dynamic defense bonus. The bonus is properly applied as a multiplier to the magnitude.

src/core/configuration/Config.ts (1)

154-154: All call sites verified and properly updated.

The signature change has been correctly implemented. The only call site in the codebase (DefaultConfig.ts:672) already passes the level parameter via dp.unit.level(), matching the new interface requirement.

coderabbitai[bot]
coderabbitai bot previously approved these changes Oct 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants