Fix/double tap full send#4398
Conversation
… any client, and merges their troop consumption instead of adding them. Required to convey separately troopRatio and troopCount for these Intents
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAttack and boat intents now send ratio and troop-count fields separately. Client code emits the new shape, transport serializes it, and execution logic computes troop counts from the split values. Tests were updated for the new path. ChangesAttack ratio payload and execution flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/execution/ExecutionManager.ts (1)
95-111: 🩺 Stability & Availability | 🟡 MinorDefault
troopRatioFactorto1instead of force-unwrapping.The parameter is optional but force-unwrapped at lines 108 and 129 without checking. If
createExecis called directly with an attack or boat intent and notroopRatioFactor, the multiplication producesNaN. Defaulting to1(no scaling) is safer and clearer:Suggested fix
- createExec(intent: StampedIntent, troopRatioFactor?: number): Execution { + createExec(intent: StampedIntent, troopRatioFactor = 1): Execution { const player = this.mg.playerByClientID(intent.clientID); if (!player) { console.warn(`player with clientID ${intent.clientID} not found`); @@ -108,7 +108,7 @@ case "attack": { return new AttackExecution( Math.floor( Math.min( - troopRatioFactor! * intent.troopRatio * intent.troopCount, + troopRatioFactor * intent.troopRatio * intent.troopCount, intent.maxTroopSent ?? intent.troopCount, ), ), @@ -129,7 +129,7 @@ case "boat": return new TransportShipExecution( player, intent.dst, - Math.floor(troopRatioFactor! * intent.troopRatio * intent.troopCount), + Math.floor(troopRatioFactor * intent.troopRatio * intent.troopCount), );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/execution/ExecutionManager.ts` around lines 95 - 111, The parameter troopRatioFactor is optional but is being force-unwrapped with the ! operator in the multiplication operation at line 108 (and line 129 based on the comment). When createExec is called without providing troopRatioFactor, it will be undefined and the multiplication will result in NaN. Change the function signature of createExec to default troopRatioFactor to 1 instead of making it optional with force-unwrapping, either by using a default parameter value in the function declaration or by assigning a default value early in the function body. This ensures safe behavior when the parameter is not provided while maintaining the same calculation logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/execution/ExecutionManager.ts`:
- Around line 45-50: The computeRatio method in ExecutionManager is performing
floating-point division which violates deterministic simulation requirements for
src/core. Replace the floating-point arithmetic in the computeRatio method with
fixed-point integer arithmetic using basis points (where 10000 represents 100
percent). Additionally, audit and update the troopRatio aggregation and
troop-send computation logic at lines 106-111 and 126-130 to also use
fixed-point integer arithmetic instead of floating-point operations to ensure
deterministic behavior across all combat resolution calculations.
- Around line 45-73: The computeRatio method calculates a scaling factor based
on raw troopRatio values without considering the maxTroopSent cap that is
applied later to each intent. This causes incorrect under-sending when mixing
capped and uncapped attacks from the same client in one turn. Fix this by
modifying the calculation of totalRatioUsage_perClientID in the switch case for
"boat" and "attack" intent types to account for the effective troops that will
actually be sent after the maxTroopSent cap is applied, rather than using only
the raw intent.troopRatio value. This ensures the computed ratio properly
reflects the actual constraints that will limit troop allocation.
In `@src/core/Schemas.ts`:
- Around line 364-365: The maxTroopSent cap is being applied too late in the
process, after ratio aggregation, which allows capped retaliations to
incorrectly reduce other same-tick attacks. Apply the retaliation cap earlier by
computing the effective capped ratio using min(troopRatio, maxTroopSent /
troopCount) during the ratio aggregation phase when building per-client merge
inputs, rather than after the downstream same-tick ratio factor is computed.
Additionally, add a core test case that verifies the behavior when a capped
retaliation is mixed with another same-tick attack to ensure the cap is applied
correctly without affecting other attacks. Remember to follow the coding
guideline that all src/core/ changes must include tests.
- Line 363: The troopRatio field in Schemas.ts currently uses a floating-point
validation with z.number().gt(0).max(1), which causes precision issues when
multiplied in the core execution logic. Replace this with a fixed-point integer
validation using either basis points (0-10000) or ppm (0-1000000)
representation. Update the troopRatio validation to use
z.number().int().min(1).max(10000) for basis points or
z.number().int().min(1).max(1000000) for ppm, then ensure the core execution
code (referenced in the comment at lines 66-67, 71-72, 108-109, 129) treats
troopRatio as an integer and performs division only at the client edge when
converting back to decimal form for display purposes.
- Around line 362-365: The troopCount and maxTroopSent fields in this schema
currently accept decimal numbers (like 12.5), but troop counts must be integers.
Add the .int() constraint to the troopCount field (which has .nonnegative()) and
to the maxTroopSent field (which has .nonnegative().optional()) to tighten the
wire contract. Additionally, apply the same .int() constraint to the troops
field in DonateTroopIntentSchema at line 426 for consistency.
In `@tests/core/execution/ExecutionManager.test.ts`:
- Around line 17-28: The test has nested beforeEach hooks which causes the outer
hook to execute first (before game is initialized) and the inner hook to execute
later. This means the Executor constructor is called with an undefined game
parameter. Refactor this by consolidating into a single async beforeEach hook
where setup() is called first to initialize the game variable, then the Executor
is created with that initialized game. Additionally, remove the manual mocking
of executor.mg (the stub assignment in the test method itself) and instead rely
on the real game simulation by passing the actual initialized game object to the
Executor constructor, following the pattern used in other test files that call
setup() and then use the returned game instance directly.
---
Outside diff comments:
In `@src/core/execution/ExecutionManager.ts`:
- Around line 95-111: The parameter troopRatioFactor is optional but is being
force-unwrapped with the ! operator in the multiplication operation at line 108
(and line 129 based on the comment). When createExec is called without providing
troopRatioFactor, it will be undefined and the multiplication will result in
NaN. Change the function signature of createExec to default troopRatioFactor to
1 instead of making it optional with force-unwrapping, either by using a default
parameter value in the function declaration or by assigning a default value
early in the function body. This ensures safe behavior when the parameter is not
provided while maintaining the same calculation logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 691359b3-993e-47af-910c-31f7b98670e2
📒 Files selected for processing (8)
src/client/ClientGameRunner.tssrc/client/Transport.tssrc/client/hud/layers/AttacksDisplay.tssrc/client/hud/layers/PlayerActionHandler.tssrc/core/Schemas.tssrc/core/execution/ExecutionManager.tstests/Attack.test.tstests/core/execution/ExecutionManager.test.ts
| troopCount: z.number().nonnegative(), | ||
| troopRatio: z.number().gt(0).max(1), | ||
| // maxTroopSent present only for retaliation. | ||
| maxTroopSent: z.number().nonnegative().optional(), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
# First, let's read the file and understand the schema structure
head -n 380 src/core/Schemas.ts | tail -n 30Repository: openfrontio/OpenFrontIO
Length of output: 910
🏁 Script executed:
sed -n '370,380p' src/core/Schemas.tsRepository: openfrontio/OpenFrontIO
Length of output: 419
🏁 Script executed:
# Search for all troopCount and similar troop-related integer fields in the file
rg "troopCount|maxTroopSent|troop" src/core/Schemas.ts -nRepository: openfrontio/OpenFrontIO
Length of output: 535
🏁 Script executed:
# Let's see the context around line 426 to understand the donate_troops schema
sed -n '422,430p' src/core/Schemas.tsRepository: openfrontio/OpenFrontIO
Length of output: 398
🏁 Script executed:
# Check if .int() is used elsewhere in Zod schemas in this file
rg "\.int\(" src/core/Schemas.tsRepository: openfrontio/OpenFrontIO
Length of output: 668
🏁 Script executed:
# Let's verify the broader context - check the actual structure of these schemas and ensure the .int() placement is correct
sed -n '360,380p' src/core/Schemas.tsRepository: openfrontio/OpenFrontIO
Length of output: 732
🏁 Script executed:
# Search for tests related to these schemas to see if any tests validate decimal troop counts
find . -name "*.test.ts" -o -name "*.spec.ts" | head -20Repository: openfrontio/OpenFrontIO
Length of output: 1168
🏁 Script executed:
# Search for tests of schemas, especially AttackIntentSchema or BoatAttackIntentSchema
find . -path "*/tests/*" -name "*[Ss]chema*.test.ts" -exec grep -l "AttackIntent\|BoatAttackIntent\|troopCount" {} \;Repository: openfrontio/OpenFrontIO
Length of output: 161
🏁 Script executed:
# Search for tests that validate intent schemas
rg "AttackIntentSchema|BoatAttackIntentSchema" tests/ --type tsRepository: openfrontio/OpenFrontIO
Length of output: 161
🏁 Script executed:
# Let's check if there are any usages of AttackIntentSchema or BoatAttackIntentSchema that might show how troopCount is used
rg "AttackIntentSchema|BoatAttackIntentSchema|donate_troops" src/ --type ts -A 2 -B 2 | head -50Repository: openfrontio/OpenFrontIO
Length of output: 2568
🏁 Script executed:
# Check where these intents are created to see if they're ever given decimal troopCount values
rg "troopCount|donate_troops|troops:" src/core/ --type ts -B 2 -A 2 | head -80Repository: openfrontio/OpenFrontIO
Length of output: 3916
🏁 Script executed:
# Let's look at the broader context in ExecutionManager to see how troopCount is used
sed -n '1,50p' src/core/execution/ExecutionManager.ts | head -30Repository: openfrontio/OpenFrontIO
Length of output: 1976
🏁 Script executed:
# Look at the exact calculation with Math.floor to understand the intent
rg "Math.floor.*troopRatio.*troopCount" src/core/execution/ExecutionManager.ts -B 5 -A 5Repository: openfrontio/OpenFrontIO
Length of output: 631
🏁 Script executed:
# Let's check the DonateTroopIntentSchema and see if troops should also be an integer
sed -n '422,428p' src/core/Schemas.tsRepository: openfrontio/OpenFrontIO
Length of output: 317
Validate troop counts as integers.
troopCount and maxTroopSent represent troop counts but the schema accepts decimals like 12.5. Use .int() to tighten the wire contract.
Suggested schema changes
- troopCount: z.number().nonnegative(),
+ troopCount: z.number().int().nonnegative(),
troopRatio: z.number().gt(0).max(1),
// maxTroopSent present only for retaliation.
- maxTroopSent: z.number().nonnegative().optional(),
+ maxTroopSent: z.number().int().nonnegative().optional(),- troopCount: z.number().nonnegative(),
+ troopCount: z.number().int().nonnegative(),
troopRatio: z.number().gt(0).max(1),Consider also applying the same fix to troops in DonateTroopIntentSchema (line 426).
📝 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.
| troopCount: z.number().nonnegative(), | |
| troopRatio: z.number().gt(0).max(1), | |
| // maxTroopSent present only for retaliation. | |
| maxTroopSent: z.number().nonnegative().optional(), | |
| troopCount: z.number().int().nonnegative(), | |
| troopRatio: z.number().gt(0).max(1), | |
| // maxTroopSent present only for retaliation. | |
| maxTroopSent: z.number().int().nonnegative().optional(), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/Schemas.ts` around lines 362 - 365, The troopCount and maxTroopSent
fields in this schema currently accept decimal numbers (like 12.5), but troop
counts must be integers. Add the .int() constraint to the troopCount field
(which has .nonnegative()) and to the maxTroopSent field (which has
.nonnegative().optional()) to tighten the wire contract. Additionally, apply the
same .int() constraint to the troops field in DonateTroopIntentSchema at line
426 for consistency.
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
…mputation (for deterministic behavior) and removing the maxTroop value for retalition, because capping the troopRatio is more versatile during aggregation
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/ClientGameRunner.ts (1)
1240-1243: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSend boat ratio as an integer percent.
Line 1242 still sends the raw UI ratio, like
0.6, while the new boat schema and execution math expect1..100. This can make boat intents invalid or send the wrong troop count.Proposed fix
new SendBoatAttackIntentEvent( tile, - this.renderer.uiState.attackRatio, + Math.floor(100 * this.renderer.uiState.attackRatio), this.myPlayer.troops(), ),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/ClientGameRunner.ts` around lines 1240 - 1243, The boat attack intent is still passing the raw UI ratio from ClientGameRunner instead of an integer percent expected by the new schema and execution math. Update the SendBoatAttackIntentEvent call in ClientGameRunner to convert this.renderer.uiState.attackRatio into a 1..100 integer before sending it, and make sure any related boat intent construction or handling uses the same percent-based value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/client/ClientGameRunner.ts`:
- Around line 1240-1243: The boat attack intent is still passing the raw UI
ratio from ClientGameRunner instead of an integer percent expected by the new
schema and execution math. Update the SendBoatAttackIntentEvent call in
ClientGameRunner to convert this.renderer.uiState.attackRatio into a 1..100
integer before sending it, and make sure any related boat intent construction or
handling uses the same percent-based value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 73b8f087-814a-4353-827c-1618d87226a3
📒 Files selected for processing (7)
src/client/ClientGameRunner.tssrc/client/Transport.tssrc/client/hud/layers/AttacksDisplay.tssrc/client/hud/layers/PlayerActionHandler.tssrc/core/Schemas.tssrc/core/execution/ExecutionManager.tstests/core/execution/ExecutionManager.test.ts
💤 Files with no reviewable changes (1)
- src/client/Transport.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/client/hud/layers/PlayerActionHandler.ts
- tests/core/execution/ExecutionManager.test.ts
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/core/execution/ExecutionManager.test.ts (1)
17-26: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not stub
executor.mgorcomputeRatioin this core test.After
setup()creates a real game, Lines 24-26 and 98 replace the core objects you want to validate. That makes this test pass on a mocked path instead of exercising the real simulation and execution wiring. Keep the realGame/player fromsetup()and assert on observable troop counts from the produced executions. Based on learnings, tests undertests/**/*.test.tsshould “use thesetup()helper fromtests/util/Setup.tsto create test game instances and exercise core simulation directly.”Also applies to: 97-99
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/core/execution/ExecutionManager.test.ts` around lines 17 - 26, This core test is stubbing executor.mg and computeRatio, which bypasses the real simulation path and makes the assertions validate mocked behavior instead of ExecutionManager/Executor wiring. Remove those stubs in createExecs merges attack-ratio-based intents from same client ID (and the similar block around the later lines), keep the real Game/player returned by setup(), and assert against observable troop counts or execution results produced by the actual createExecs flow.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/hud/layers/AttacksDisplay.ts`:
- Around line 203-218: The retaliation payload is being rounded to an integer
percentage in AttacksDisplay before it reaches core, which can turn a valid
1-troop reply into 0. Update the SendAttackIntentEvent path so retaliation flows
preserve the exact capped attack amount/ratio instead of flooring it into a
percent, and apply the same fix in ClientGameRunner where the attack intent is
constructed. Keep the full precision needed by core and only convert at the
final execution point if required.
---
Duplicate comments:
In `@tests/core/execution/ExecutionManager.test.ts`:
- Around line 17-26: This core test is stubbing executor.mg and computeRatio,
which bypasses the real simulation path and makes the assertions validate mocked
behavior instead of ExecutionManager/Executor wiring. Remove those stubs in
createExecs merges attack-ratio-based intents from same client ID (and the
similar block around the later lines), keep the real Game/player returned by
setup(), and assert against observable troop counts or execution results
produced by the actual createExecs flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0e197039-354f-46d5-a797-12527c193dda
📒 Files selected for processing (8)
src/client/ClientGameRunner.tssrc/client/Transport.tssrc/client/hud/layers/AttacksDisplay.tssrc/client/hud/layers/PlayerActionHandler.tssrc/core/Schemas.tssrc/core/execution/ExecutionManager.tstests/Attack.test.tstests/core/execution/ExecutionManager.test.ts
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/core/execution/ExecutionManager.test.ts (1)
23-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExercise the real game path in this core test.
setup()creates a real game, but Line 24 replacesexecutor.mgwith a stub and later assertions read private fields before execution init/tick. This can pass while real troop clamping/removal behaves differently. Prefer real players/client IDs fromsetup()and assert troop deltas after executing the created executions. As per coding guidelines, tests should usesetup()and exercise core simulation directly.Also applies to: 107-131
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/core/execution/ExecutionManager.test.ts` around lines 23 - 26, The test is stubbing executor.mg and reading private state too early, which bypasses the real simulation path. Update ExecutionManager.test.ts to use the real game/player/client IDs returned by setup() instead of mocking playerByClientID, then run the created executions through the normal init/tick flow and assert the resulting troop deltas after execution. Keep the checks focused on observable outcomes from the core execution path rather than private fields.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/execution/ExecutionManager.ts`:
- Around line 45-53: The ratio merging in ExecutionManager.computeRatio can
overflow into Infinity/NaN when many same-tick intents are combined, so update
this path to use integer-safe arithmetic or enforce a hard cap on per-client
ratio intents before merging. Keep the fix localized to computeRatio and the
same-tick intent aggregation logic that feeds it, and make sure attack/boat
troop counts remain finite even at high intent counts.
---
Nitpick comments:
In `@tests/core/execution/ExecutionManager.test.ts`:
- Around line 23-26: The test is stubbing executor.mg and reading private state
too early, which bypasses the real simulation path. Update
ExecutionManager.test.ts to use the real game/player/client IDs returned by
setup() instead of mocking playerByClientID, then run the created executions
through the normal init/tick flow and assert the resulting troop deltas after
execution. Keep the checks focused on observable outcomes from the core
execution path rather than private fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 97f43854-9440-4b1a-92d1-034aa71417bc
📒 Files selected for processing (8)
src/client/ClientGameRunner.tssrc/client/Transport.tssrc/client/hud/layers/AttacksDisplay.tssrc/client/hud/layers/PlayerActionHandler.tssrc/core/Schemas.tssrc/core/execution/ExecutionManager.tstests/Attack.test.tstests/core/execution/ExecutionManager.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/ClientGameRunner.ts (1)
1182-1198: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGuard against a divide-by-zero that makes the ratio
NaN.
mostRecentAttack.troops / this.myPlayer.troops()divides by the player's troop count. Whentroops()is0, this isInfinity(whenmostRecentAttack.troops > 0,Math.minthen safely picksattackRatio), but if both are0you get0 / 0 = NaN.Math.min(attackRatio, NaN)isNaN, and it flows throughMath.max(NaN, 1) → NaNandMath.ceil(NaN) → NaN, so aNaNpercentage is sent. A short guard keeps the value clean.🛡️ Proposed guard
+ const myTroops = this.myPlayer.troops(); + const fullDefenseRatio = + myTroops > 0 ? mostRecentAttack.troops / myTroops : 1; this.eventBus.emit( new SendAttackIntentEvent( attacker.id(), Math.ceil( // Ensures the attackRatio is between 1% and the required percentage to defend fully. Math.max( 100 * Math.min( this.renderer.uiState.attackRatio, - mostRecentAttack.troops / this.myPlayer.troops(), + fullDefenseRatio, ), 1, ), ), - this.myPlayer.troops(), + myTroops, ), );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/ClientGameRunner.ts` around lines 1182 - 1198, The attack percentage calculation in ClientGameRunner’s SendAttackIntentEvent path can become NaN when myPlayer.troops() is 0 and mostRecentAttack.troops is also 0. Add a guard around the ratio computation so the division result is never used when it would be NaN, and ensure the value passed into Math.min/Math.max/Math.ceil always falls back to a valid minimum percentage; keep the fix localized near the existing attackRatio logic in ClientGameRunner.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/client/ClientGameRunner.ts`:
- Around line 1182-1198: The attack percentage calculation in ClientGameRunner’s
SendAttackIntentEvent path can become NaN when myPlayer.troops() is 0 and
mostRecentAttack.troops is also 0. Add a guard around the ratio computation so
the division result is never used when it would be NaN, and ensure the value
passed into Math.min/Math.max/Math.ceil always falls back to a valid minimum
percentage; keep the fix localized near the existing attackRatio logic in
ClientGameRunner.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bbcf581e-7a03-4e6b-973e-f1e568db7082
📒 Files selected for processing (3)
src/client/ClientGameRunner.tssrc/core/Schemas.tstests/Attack.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/Schemas.ts
- tests/Attack.test.ts
evanpelle
left a comment
evanpelle
left a comment
There was a problem hiding this comment.
Instead of the merge math, could we just send troopRatio and resolve floor(ratio * owner.troops()) inside AttackExecution.init (not createExecs) — since same-tick executions already init sequentially after each troop removal, two 50% attacks compose to 75% for free, with no per-client maps or float math in core?
|
@evanpelle Solving To be fair, I do not know if this sequencing is guaranteed in some way or not. If we can guarantee each attack-intent is executed in the same order amongst all servers/clients, then your proposal works and is more elegant code-wise (but less elegant mathematically because the sequence influences the gameplay AND could differ from the sequence the player clicked). Let's compare all versions with a simple case. player A has 100k troops and attacks player B with 50% Att-ratio and, during the same tick, retaliate player C's attack which has 25k inbound troops. Current behavior: The game sends one 50k attack to player B and one 25k attack to retaliate player C (retaliation is complete). Total troops used: 75k. PR's behavior: The game consider it as two attacks with (50%, 100k) for player B and (25%, 100k) for retaliation against C. Your proposition has the same intents attacks with (50%) for player B and (25%) for retaliation against C but sends them directly to
Note on extreme-ness of the example: Conclusion: I think the merge-math is the clear winner. I still need to include some comments of coderabbitai though. Do you have a specific problem with the merge-computation in core? I think that it is using only integers, flooring the numbers only at two specific points (computing the correction factor and for the total troops of each attack) where both are ensured to be consistent amongst servers. |
3 participants
Add approved & assigned issue number here:
Resolves #4237
Improved version of #4397 that got automatically closed because I didn't respect the issue-resolution formatting and because I could not add commits to the PR anymore.
Description:
The PR changes the Intent Events structure using the attack-ratio to compute sent troop such that it do send to the Execution Manager both the attack ratio and the total troopCount to ratio from.
Then, the Execution Manager collects all those numbers for each ClientID sending many of those intents events in the same tick, and adjust the computation such that the total sent number of troop is equal (up to roundings) to the total troops that would be sent if the attacks where computed one after the other (the order is not important because of math).
More precisely, the "Remaining troops" after attacks of ratio 0.6, 0.3 and 0.2 is the multiplication of their non-attacks ratio, 0.40.70.8 = 0.224. The actual attack ratio for each of these same-tick attacks is then adapted accordingly BUT in a way that do not depend at all from their order or size, because the reduction is applied uniformingly.
Also, this PR adds a new test that tests various situations and compares them with the mocked-up still buggy version.
Please complete the following:
I also tested that the game is yes fixed by forcing the radial-menu attack to always send two attacks (easy way to reproduce the bug) and confirmed it is fixed.
Please put your Discord username so you can be contacted if a bug or regression is found:
Katokoda