Extend SAM Range to cover Hydros when stacked #2351
Conversation
|
|
WalkthroughConfig adds level-aware SAM range methods; SAM execution and radius rendering now use per-SAM-level ranges; ghost/build preview range visualization computes and updates ranges based on resolved SAM level. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as User / UI
participant SIL as StructureIconsLayer
participant Config as Config
participant Draw as SpriteFactory/Renderer
participant SAM as SAMLauncher
rect rgb(243,248,255)
UI->>SIL: show ghost / select buildable
SIL->>SIL: resolveGhostRangeLevel(unit)
SIL->>Config: samRange(level)
Config-->>SIL: computed range
SIL->>Draw: updateGhostRange(range)
Draw-->>UI: ghost range rendered
end
rect rgb(255,248,240)
UI->>SAM: simulation tick / engage
SAM->>Config: samRange(sam.level())
Config-->>SAM: range
SAM->>SAM: isInRange(target) / getSingleTarget()
SAM-->>UI: fire or no-fire
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks✅ Passed checks (2 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/client/graphics/layers/SAMRadiusLayer.ts(1 hunks)src/client/graphics/layers/StructureDrawingUtils.ts(1 hunks)src/client/graphics/layers/StructureIconsLayer.ts(4 hunks)src/core/configuration/Config.ts(1 hunks)src/core/configuration/DefaultConfig.ts(1 hunks)src/core/execution/SAMLauncherExecution.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: DevelopingTom
Repo: openfrontio/OpenFrontIO PR: 1900
File: src/core/execution/SAMLauncherExecution.ts:103-111
Timestamp: 2025-08-24T11:13:08.495Z
Learning: In SAMLauncherExecution.ts, the cached target bug can only occur if: 1) SAM is not on cooldown when nuke is in range, 2) SAM goes on cooldown right after computing trajectory, 3) SAM becomes available again before nuke explodes. This is not possible with current cooldown values but the fix is still valuable for robustness.
📚 Learning: 2025-08-24T11:13:08.495Z
Learnt from: DevelopingTom
Repo: openfrontio/OpenFrontIO PR: 1900
File: src/core/execution/SAMLauncherExecution.ts:103-111
Timestamp: 2025-08-24T11:13:08.495Z
Learning: In SAMLauncherExecution.ts, the cached target bug can only occur if: 1) SAM is not on cooldown when nuke is in range, 2) SAM goes on cooldown right after computing trajectory, 3) SAM becomes available again before nuke explodes. This is not possible with current cooldown values but the fix is still valuable for robustness.
Applied to files:
src/core/configuration/DefaultConfig.tssrc/core/configuration/Config.tssrc/client/graphics/layers/SAMRadiusLayer.tssrc/core/execution/SAMLauncherExecution.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/execution/SAMLauncherExecution.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/execution/SAMLauncherExecution.ts
📚 Learning: 2025-06-22T21:51:14.990Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1248
File: src/client/graphics/layers/TerritoryInfoLayer.ts:20-20
Timestamp: 2025-06-22T21:51:14.990Z
Learning: In TerritoryInfoLayer.ts, the highlightedTerritory field uses both null and undefined intentionally: undefined represents initial state or inactive layer (Ctrl released), while null represents active layer with no territory being highlighted at cursor position. This distinction is important for proper state change detection.
Applied to files:
src/client/graphics/layers/StructureIconsLayer.ts
🧬 Code graph analysis (4)
src/core/configuration/DefaultConfig.ts (2)
src/core/game/UnitImpl.ts (1)
level(406-408)src/core/game/GameView.ts (1)
level(167-169)
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/client/graphics/layers/StructureDrawingUtils.ts (2)
src/core/game/UnitImpl.ts (2)
level(406-408)type(145-147)src/core/game/GameView.ts (3)
level(167-169)type(94-96)type(340-342)
src/client/graphics/layers/StructureIconsLayer.ts (2)
src/core/game/GameView.ts (2)
unit(658-660)level(167-169)src/core/game/Game.ts (1)
BuildableUnit(758-764)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/core/configuration/DefaultConfig.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-24T11:13:08.495Z
Learnt from: DevelopingTom
Repo: openfrontio/OpenFrontIO PR: 1900
File: src/core/execution/SAMLauncherExecution.ts:103-111
Timestamp: 2025-08-24T11:13:08.495Z
Learning: In SAMLauncherExecution.ts, the cached target bug can only occur if: 1) SAM is not on cooldown when nuke is in range, 2) SAM goes on cooldown right after computing trajectory, 3) SAM becomes available again before nuke explodes. This is not possible with current cooldown values but the fix is still valuable for robustness.
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-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
🧬 Code graph analysis (1)
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 (2)
915-930: Level 1 now correctly stays at base range.The implementation properly addresses the previous concern about level 1. The formula
baseRange + step * (numericLevel - 1)ensures level 1 returns exactly 70 (base SAM range), and levels 2–5 interpolate smoothly up to 110 (hydrogen outer range + 10). The step divisor(maxRangeLevel - 1)correctly spreads the progression across five levels.
918-918: Level 0 behavior is already handled safely by the clamping logic.The Math.max(1, level) clamp at line 918 is intentional and correct. SAM units in the codebase are always created at level 1 (confirmed in tests), and the clamp ensures that if level 0 were somehow passed, it would map to the same range as level 1. The samRange() calculation at line 915–926 uses this clamped value to compute: baseRange + step × (numericLevel − 1). For both level 0 and level 1, this produces baseRange (the minimum range). This is defensive programming—level 1 is the true minimum level, and the code safely prevents invalid states. No action needed.
NewYearNewPhil
force-pushed
the
increate-stacked-sam-range
branch
from
3ec8bb1 to
2997b03
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/client/graphics/layers/StructureIconsLayer.ts (1)
401-420: Clarify the fallback to level 2.The level resolution logic is generally sound, but the fallback to level 2 at line 416 is unclear. If we're upgrading a unit but can't find it (neither via
game.unit()norpotentialUpgrade), why default to level 2 instead of level 1?This could happen in edge cases (e.g., unit just destroyed, network lag, race condition). Consider:
- Returning
undefinedif the unit can't be found and letting the caller handle it- Falling back to level 1 for consistency with new builds
- Adding a comment explaining why level 2 is the right choice
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/client/graphics/layers/SAMRadiusLayer.ts(1 hunks)src/client/graphics/layers/StructureDrawingUtils.ts(1 hunks)src/client/graphics/layers/StructureIconsLayer.ts(4 hunks)src/core/configuration/Config.ts(1 hunks)src/core/configuration/DefaultConfig.ts(1 hunks)src/core/execution/SAMLauncherExecution.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/client/graphics/layers/SAMRadiusLayer.ts
- src/core/configuration/DefaultConfig.ts
- src/core/configuration/Config.ts
- src/client/graphics/layers/StructureDrawingUtils.ts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-24T11:13:08.495Z
Learnt from: DevelopingTom
Repo: openfrontio/OpenFrontIO PR: 1900
File: src/core/execution/SAMLauncherExecution.ts:103-111
Timestamp: 2025-08-24T11:13:08.495Z
Learning: In SAMLauncherExecution.ts, the cached target bug can only occur if: 1) SAM is not on cooldown when nuke is in range, 2) SAM goes on cooldown right after computing trajectory, 3) SAM becomes available again before nuke explodes. This is not possible with current cooldown values but the fix is still valuable for robustness.
Applied to files:
src/core/execution/SAMLauncherExecution.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/execution/SAMLauncherExecution.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/execution/SAMLauncherExecution.ts
🧬 Code graph analysis (1)
src/client/graphics/layers/StructureIconsLayer.ts (2)
src/core/game/GameView.ts (2)
unit(658-660)level(167-169)src/core/game/Game.ts (1)
BuildableUnit(758-764)
🔇 Additional comments (6)
src/core/execution/SAMLauncherExecution.ts (2)
53-53: LGTM! Level-aware range calculation implemented correctly.The range now correctly uses the SAM's current level to determine its effective range. This aligns with the PR objective of extending SAM range for stacked (higher-level) SAMs.
85-85: LGTM! Pre-shooting detection uses maximum range appropriately.Using
"MAX"for detection range allows the SAM to detect and compute trajectories for nukes that will eventually enter range, enabling the pre-shot targeting system to work correctly at all SAM levels.src/client/graphics/layers/StructureIconsLayer.ts (4)
61-61: LGTM! Tracking range level enables optimization.The
rangeLevelfield allowsupdateGhostRangeto skip unnecessary range recreation when the level hasn't changed.
376-380: LGTM! Ghost range initialization properly deferred.Setting range to
nullinitially and then computing the base level separately is a clean approach that allows the range to be created with the correct level from the start.
422-445: LGTM! Range update logic is efficient and correct.The method properly:
- Guards against missing ghost
- Optimizes by skipping recreation when level unchanged
- Cleans up old range before creating new one
- Reuses container position and delegates to factory
281-282: LGTM! Ghost range updates based on buildable unit state.Resolving and applying the range level during each render cycle ensures the ghost preview reflects whether the player is upgrading (showing next level's range) or building new (showing level 1 range).
| // i.e. when a SAM has level >= 5, it outranges hydros | ||
| const maxRangeLevel = 5; | ||
| const numericLevel = level === "MAX" ? maxRangeLevel : Math.max(1, level); | ||
|
|
||
| const safetyMargin = 10; | ||
|
|
||
| const baseRange = this.defaultSamRange(); | ||
| const hydroRange = this.nukeMagnitudes(UnitType.HydrogenBomb).outer; | ||
|
|
||
| const step = Math.ceil( | ||
| (hydroRange + safetyMargin - baseRange) / Math.max(1, maxRangeLevel - 1), | ||
| ); | ||
|
|
||
| return baseRange + step * (Math.min(numericLevel, maxRangeLevel) - 1); |
There was a problem hiding this comment.
I think basing it on the nuke magnitudes is a bit too complex. i'm thinking we use a rational function instead, something like:
return 250 * level/(level + 5)
this will have it asymptote ate 250 pixel range.
There was a problem hiding this comment.
alright, changed it to something simpler now:
150 - 480 / (level + 5)
results in level 1 = 70 (defaultSamRange), level 5 = 102 (just above hydro), approaches 150
i think a higher cap would be extreme, 250 is 2.5 hydro ranges, basically unkillable at that point. new screenshot for visualization:
| if (this.potentialUpgrade) { | ||
| return this.potentialUpgrade.unit.level() + 1; | ||
| } | ||
| return 2; |
There was a problem hiding this comment.
I was coding a bit too defensive here I suppose, because just based on types, the unit could still be undefined at that point, so 2 was sort of a sane guess given this is in the canUpgrade branch.
I've now changed it to simply error if such a state occured, which should never happen.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/client/graphics/layers/StructureIconsLayer.ts (1)
401-417: Handle missing upgrade unit more explicitly.When
canUpgradeis set but the existing unit cannot be found (line 408-413), the code logs an error and falls through to return1. This is misleading since we're upgrading an unknown-level unit, not building a new one.Consider returning
undefinedto indicate the level cannot be determined, or explicitly return a fallback value:if (buildableUnit.canUpgrade !== false) { const existing = this.game.unit(buildableUnit.canUpgrade); if (existing) { return existing.level() + 1; } else { console.error("Failed to find existing SAMLauncher for upgrade"); + return undefined; // or return 2 as a reasonable fallback } } return 1;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/client/graphics/layers/StructureIconsLayer.ts(4 hunks)src/core/configuration/Config.ts(1 hunks)src/core/configuration/DefaultConfig.ts(1 hunks)src/core/execution/SAMLauncherExecution.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/execution/SAMLauncherExecution.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-24T11:13:08.495Z
Learnt from: DevelopingTom
Repo: openfrontio/OpenFrontIO PR: 1900
File: src/core/execution/SAMLauncherExecution.ts:103-111
Timestamp: 2025-08-24T11:13:08.495Z
Learning: In SAMLauncherExecution.ts, the cached target bug can only occur if: 1) SAM is not on cooldown when nuke is in range, 2) SAM goes on cooldown right after computing trajectory, 3) SAM becomes available again before nuke explodes. This is not possible with current cooldown values but the fix is still valuable for robustness.
Applied to files:
src/core/configuration/Config.tssrc/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
🧬 Code graph analysis (3)
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/client/graphics/layers/StructureIconsLayer.ts (3)
src/core/game/GameView.ts (2)
unit(658-660)level(167-169)src/core/game/Game.ts (1)
BuildableUnit(758-764)src/core/game/UnitImpl.ts (1)
level(406-408)
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 (6)
src/core/configuration/DefaultConfig.ts (1)
916-919: Verify the formula choice with maintainer.The implementation uses
150 - 480 / (level + 5), but the past review comment suggested250 * level/(level + 5)with asymptote at 250. Your formula achieves the stated goals (level 1 = 70, level 5 just above hydro range), but differs from the maintainer's suggestion. Is this intentional, or should the formula match the suggestion more closely?src/core/configuration/Config.ts (1)
173-173: Clean interface extension.The parameterized
samRange(level)signature is correctly added alongside the existingdefaultSamRange()method, maintaining backward compatibility.src/client/graphics/layers/StructureIconsLayer.ts (4)
61-61: Good optimization with level tracking.The optional
rangeLevelproperty enables efficient early returns inupdateGhostRangeby detecting when the level hasn't changed.
281-282: Range update properly integrated into render flow.The level resolution and range update are correctly placed after the buildable unit is determined, ensuring the range visualization reflects the current upgrade state.
376-380: Deferred range creation improves clarity.Initializing
rangetonulland creating it after resolving the base level is cleaner than creating it immediately with potentially incorrect parameters.
419-442: Solid implementation with optimization.The early return at line 424 when
rangeLevel === levelprevents unnecessary range destruction and recreation. The cleanup and state management flow is correct.
|
@evanpelle the |
|
I feel like I would never stack 50 SAM sites in one place (nor get enough money with the new gold trade income!) Would it be possible to make a max upgrade of ~10 SAM sites? It would be better to do 5x10 stacks of SAMs than one 50 stack. - And it would be unfair to allow people to upgrade more than 10 levels IMO. |
NewYearNewPhil
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/core/configuration/DefaultConfig.ts (1)
916-919: Fix missing input validation to prevent edge case issues.The method lacks validation for the
levelparameter, creating critical edge cases:
- Level 0: returns 54 (below base range of 70)
- Level -5: division by zero crash
- Negative levels: unpredictable behavior
Apply this diff to clamp the level to safe values:
samRange(level: number): number { // rational growth function (level 1 = 70, level 5 just above hydro range, asymptotically approaches 150) - return this.maxSamRange() - 480 / (level + 5); + const clampedLevel = Math.max(1, level); + return this.maxSamRange() - 480 / (clampedLevel + 5); }
🧹 Nitpick comments (2)
src/core/configuration/DefaultConfig.ts (1)
916-923: Consider adding unit tests for this critical range calculation.Since you requested input on testing in the PR description, and this method affects SAM targeting, rendering, and ghost previews across the codebase, unit tests would help verify:
- Level 1 returns base range (70)
- Levels 2-4 interpolate correctly
- Level 5+ returns expected values
- Edge cases (level 0, negative values) are handled
Do you want me to generate a test suite for the
samRangeandmaxSamRangemethods?src/core/configuration/Config.ts (1)
172-174: Remove or consolidatedefaultSamRange()for API consistency.Analysis shows that
samRange(1)returns exactly 70—the same value asdefaultSamRange(). SincedefaultSamRange()is called only once (in `structureSpawnTileValue.ts:115), you have two options:
Migrate to
samRange(1): Replace the single call withsamRange(1)and removedefaultSamRange()from the interface and implementation. This eliminates redundancy.Keep
defaultSamRange()documented: If you prefer the convenience API for clarity, add a comment explaining that it's an alias forsamRange(1)(level 1 SAM range).The current design has
samRange(level)used actively (3 call sites) whiledefaultSamRange()sits unused except once. Consolidating simplifies the API surface.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/core/configuration/Config.ts(1 hunks)src/core/configuration/DefaultConfig.ts(1 hunks)src/core/execution/SAMLauncherExecution.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-24T11:13:08.495Z
Learnt from: DevelopingTom
Repo: openfrontio/OpenFrontIO PR: 1900
File: src/core/execution/SAMLauncherExecution.ts:103-111
Timestamp: 2025-08-24T11:13:08.495Z
Learning: In SAMLauncherExecution.ts, the cached target bug can only occur if: 1) SAM is not on cooldown when nuke is in range, 2) SAM goes on cooldown right after computing trajectory, 3) SAM becomes available again before nuke explodes. This is not possible with current cooldown values but the fix is still valuable for robustness.
Applied to files:
src/core/execution/SAMLauncherExecution.tssrc/core/configuration/DefaultConfig.tssrc/core/configuration/Config.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/execution/SAMLauncherExecution.tssrc/core/configuration/DefaultConfig.tssrc/core/configuration/Config.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/execution/SAMLauncherExecution.ts
🧬 Code graph analysis (2)
src/core/configuration/DefaultConfig.ts (2)
src/core/game/UnitImpl.ts (1)
level(406-408)src/core/game/GameView.ts (1)
level(167-169)
src/core/configuration/Config.ts (2)
src/core/game/UnitImpl.ts (1)
level(406-408)src/core/game/GameView.ts (1)
level(167-169)
🪛 GitHub Actions: 🧪 CI
src/core/execution/SAMLauncherExecution.ts
[error] 228-228: Test assertion failed: expected 'sam1.isInCooldown()' to be falsy, but received true.
[warning] 108-108: Console warning: 'cannot build Nuke' during execution.
🔇 Additional comments (3)
src/core/execution/SAMLauncherExecution.ts (3)
53-53: LGTM!Correctly uses the level-based SAM range for this specific SAM's range check.
85-85: LGTM!Using
maxSamRange()for detection range allows the SAM to preshot nukes beyond its current level's range, which aligns with the targeting system's design.
228-228: Confirm whether SAM range changes caused test failure and whether behavior change is intentional.The test "SAMs should target only nukes aimed at nearby targets if not close to launch site" verifies that
sam1(at position 50,1) should not intercept a nuke targeted 149 tiles away at (199,1), whilesam2(at 199,1) should intercept it. The test currently fails becausesam1.isInCooldown()is true when it should be false.This indicates
sam1fired when it shouldn't have. Since the test directly validates range-based targeting logic, verify whether your SAM range changes extended the intercept range beyond 149 tiles. If so, either update the test expectations if this is intentional, or adjust the range configuration.
NewYearNewPhil
force-pushed
the
increate-stacked-sam-range
branch
from
8e45ad5 to
cd60ea3
Compare
|
Looks like a SAM test is failing |
NewYearNewPhil
dismissed stale reviews from evanpelle and coderabbitai[bot]
via
ec6328b
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/util/TestConfig.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/util/TestConfig.ts (2)
src/core/game/UnitImpl.ts (1)
level(406-408)src/core/game/GameView.ts (1)
level(167-169)
fixed in ec6328b |
4 participants
Description:
Implements SAM range extension for stacked SAMs to cover hydros as requested in #2347 and many times from users in discord.
This implementation is as simple as possible: At level 5 and higher, SAMs extend their range to the range of a hydrogen bomb + 10 for a small safety margin. Levels 2-4 are interpolated between.
Screenshot to show the sizes compared to a hydro:
Everything works together with the new range UI, although I might need to unify with / rebase on #2350. Not yet tested with #2348, but shouldn't be an issue.
Input needed:
I'm happy to implement any of these paths, or just roll with the simple way it's set up now, just let me know.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
newyearnewphil