Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update battle simulator #2686

Merged
merged 6 commits into from
Feb 13, 2025
Merged

update battle simulator #2686

merged 6 commits into from
Feb 13, 2025

Conversation

aymericdelab
Copy link
Collaborator

@aymericdelab aymericdelab commented Feb 10, 2025

Summary by CodeRabbit

  • New Features

    • Introduced a new combat simulation panel for enhanced battle interactions.
    • Added interactive selectors for choosing biome, tier, and troop type.
  • Refactor

    • Replaced the older battle simulation with a new combat simulation experience.
    • Removed outdated pillage simulation options and associated functionalities.
  • Bug Fixes

    • Eliminated dynamic calculations for troop losses during raids, replacing them with static values.
  • Chores

    • Cleaned up and streamlined simulation utilities for improved maintainability.

Copy link

vercel bot commented Feb 10, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
eternum ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 10, 2025 4:42pm
eternum-docs ❌ Failed (Inspect) Feb 10, 2025 4:42pm
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
eternum-landing ⬜️ Ignored (Inspect) Visit Preview Feb 10, 2025 4:42pm

Copy link
Contributor

mentatbot bot commented Feb 10, 2025

Hi @aymericdelab! You need to be added as a user to interact with me. Please ask @ponderingdemocritus to add you on the settings page.

You are receiving this comment because I am set to review all PRs. That is configurable here.

Copy link
Contributor

coderabbitai bot commented Feb 10, 2025

Warning

Rate limit exceeded

@aymericdelab has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 36 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between d1aaad9 and 1cebf7b.

📒 Files selected for processing (1)
  • packages/core/src/utils/combat-simulator.ts (1 hunks)

Walkthrough

This pull request removes all references to the PillageSimulation component and its associated logic while refactoring battle simulation into a CombatSimulation workflow. In addition, new UI components for selecting biomes, tiers, and troop types have been introduced. Core utility modules have been updated by removing the legacy battle-simulator and adding a new combat-simulator with refined calculations. Modifications also include changes in type definitions, component props, and export updates to support the revised simulation functionalities.

Changes

Files / Grouping Change Summary
client/apps/game/src/ui/components/military/entities-army-table.tsx Removed <PillageSimulation /> and related import; replaced <BattleSimulation /> with <CombatSimulation />.
client/apps/game/src/ui/components/navigation/config.tsx Removed "PillageSimulation" option from OSWindows type and deleted the exported pillageSimulation constant.
client/apps/game/src/ui/components/worldmap/battles/* Deleted battle-simulation-panel.tsx and pillage-simulation-panel.tsx; added new combat-simulation-panel.tsx.
client/apps/game/src/ui/elements/* Added new components: SelectBiome, SelectTier, and SelectTroop for enhanced selection interfaces.
client/apps/game/src/ui/modules/military/battle-view/* Removed troop loss calculation functions and interfaces in battle-actions.tsx and utils.tsx, replacing dynamic loss values with static ones.
client/apps/game/src/ui/modules/simulation/* Renamed and refactored simulation module from BattleSimulation to CombatSimulation (updated title and width); removed pillage-simulation.tsx.
packages/core/src/utils/* Deleted legacy battle-simulator.ts; added combat-simulator.ts with new classes, enums, and methods; updated index exports accordingly.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant UI as CombatSimulationPanel
    participant Simulator as CombatSimulator
    participant OS as OSWindow

    User->>UI: Enter army and biome details
    UI->>Simulator: simulateBattle(attacker, defender, biome)
    Simulator-->>UI: Return damage values and results
    UI->>OS: Render Combat Simulation with updated title/width
    UI-->>User: Display simulation outcome
Loading

Possibly related PRs

Suggested reviewers

  • bob0005

Poem

I’m just a bunny in a code-filled glade,
Hopping through changes in lines finely laid.
Pillage has hopped away, no longer in sight,
Now combat battles burst forth in light.
With a twitch of my nose, I cheer every fix—
Carrots and code make the perfect mix!
🐇🌟


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

Failed to generate code suggestions for PR

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: 5

🧹 Nitpick comments (11)
packages/core/src/utils/combat-simulator.ts (7)

1-9: Consider replacing the static-only Percentage class with simple utility functions.
The Percentage class only contains static methods and fields, triggering a lint rule (noStaticOnlyClass). Converting it to a collection of standalone functions or a namespace can improve clarity and avoid confusion with this.

Example refactor:

-export class Percentage {
-  static _100() {
-    return 10_000;
-  }
-
-  static get(value: number, numerator: number) {
-    return (value * numerator) / Percentage._100();
-  }
-}
+export function get100() {
+  return 10_000;
+}
+
+export function getPercentage(value: number, numerator: number) {
+  return (value * numerator) / get100();
+}
🧰 Tools
🪛 Biome (1.9.4)

[error] 1-9: Avoid classes that contain only static members.

Prefer using simple functions instead of classes with only static members.

(lint/complexity/noStaticOnlyClass)


36-41: Consider using an enum or narrower type for tier.
Currently, tier is 1 | 2 | 3. While this is valid, an enum can enhance type safety and readability if the values become more extensive or require additional context later.


43-48: Magic numbers in constants might need documentation or config-based storage.
BASE_T1_VALUE, STAMINA_ATTACK_THRESHOLD, and BASE_DAMAGE_FACTOR are critical to your damage formula. A short comment or external config references can help future maintainers understand these baseline values and tune them without diving into code.

🧰 Tools
🪛 Biome (1.9.4)

[error] 43-147: Avoid classes that contain only static members.

Prefer using simple functions instead of classes with only static members.

(lint/complexity/noStaticOnlyClass)


49-54: Same for BETA and transition constants.
BETA_SMALL, BETA_LARGE, C0, and DELTA drive your scaling. Consider layering them in config or adding doc comments explaining their purpose.


86-95: Tier values are correct; consider table-driven or config approach.
The numeric multipliers (2.5 and 7) might deserve documentation or an external config entry for clarity and maintainability.

🧰 Tools
🪛 Biome (1.9.4)

[error] 89-89: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 91-91: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 93-93: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


97-107: Using this in a static context is valid but can be confusing.
The linter warns about this usage in static methods. Consider referencing the class by name or acknowledging that this is intentionally used here. Also, verify these stamina thresholds (30 and 0.3) are appropriate.

🧰 Tools
🪛 Biome (1.9.4)

[error] 99-99: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 101-101: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 101-101: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


109-114: Effective beta scaling is well-structured; add inline commentary.
The usage of Math.tanh for smoothly transitioning from BETA_SMALL to BETA_LARGE is elegant. However, a brief comment clarifying the formula’s rationale helps future contributors.

🧰 Tools
🪛 Biome (1.9.4)

[error] 111-111: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 112-112: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 112-112: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 112-112: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 112-112: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)

client/apps/game/src/ui/elements/select-biome.tsx (1)

56-74: Extract duplicate code into a reusable component.

The troop resources rendering logic is duplicated between the trigger and content sections.

Create a new component to handle the troop resources rendering:

const TroopResources: React.FC<{ biome: Biome; className?: string }> = ({ biome, className }) => (
  <div className="flex gap-8">
    {TROOP_RESOURCES.map(({ type, resourceId }) => {
      const bonus = CombatSimulator.getBiomeBonus(type, biome);
      return (
        <div key={type} className="flex items-center gap-2 w-16 justify-end">
          <ResourceIcon resource={resources.find((r) => r.id === resourceId)?.trait || ""} size="sm" />
          <span
            className={
              bonus > 1
                ? `text-order-brilliance ${className || 'text-xs'}`
                : bonus < 1
                  ? `text-order-giants ${className || 'text-xs'}`
                  : `text-gold/50 ${className || 'text-xs'}`
            }
          >
            {formatBonus(bonus)}
          </span>
        </div>
      );
    })}
  </div>
);

Then use it in both places:

// In SelectTrigger
<TroopResources biome={selectedBiome as Biome} />

// In SelectContent
<TroopResources biome={biome} className="text-sm font-medium" />

Also applies to: 86-104

client/apps/game/src/ui/components/worldmap/battles/combat-simulation-panel.tsx (3)

68-68: Consider using type validation instead of type assertion.

The type assertion tier as 1 | 2 | 3 could be unsafe if the tier value is not one of the expected values.

Consider validating the tier value before the type assertion:

-  onChange({ ...army, tier: tier as 1 | 2 | 3 });
+  const validTier = tier === 1 || tier === 2 || tier === 3 ? tier : 1;
+  onChange({ ...army, tier: validTier });

105-105: Consider extracting magic numbers into named constants.

The stamina cost (30) and bonus (30) values are used multiple times in the code. These should be extracted into named constants for better maintainability.

Add these constants at the top of the file:

+const STAMINA_COST = 30;
+const STAMINA_BONUS = 30;

Then use them in the code:

-const staminaCost = 30;
+const staminaCost = STAMINA_COST;

Also applies to: 106-106


208-214: Simplify the complex ternary operator.

The nested ternary operator for biome bonus display is hard to read and maintain.

Consider using a helper function or object mapping:

+const getBiomeBonusColor = (bonus: number) => {
+  if (bonus > 1) return "text-green-400";
+  if (bonus < 1) return "text-red-400";
+  return "text-gold/50";
+};

-className={`text-xl font-bold ${
-  CombatSimulator.getBiomeBonus(data.army.troopType, biome) > 1
-    ? "text-green-400"
-    : CombatSimulator.getBiomeBonus(data.army.troopType, biome) < 1
-      ? "text-red-400"
-      : "text-gold/50"
-}`}
+className={`text-xl font-bold ${getBiomeBonusColor(CombatSimulator.getBiomeBonus(data.army.troopType, biome))}`}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a53b70c and 250cf4f.

⛔ Files ignored due to path filters (3)
  • client/public/images/resources/26.png is excluded by !**/*.png
  • client/public/images/resources/27.png is excluded by !**/*.png
  • client/public/images/resources/28.png is excluded by !**/*.png
📒 Files selected for processing (15)
  • client/apps/game/src/ui/components/military/entities-army-table.tsx (2 hunks)
  • client/apps/game/src/ui/components/navigation/config.tsx (0 hunks)
  • client/apps/game/src/ui/components/worldmap/battles/battle-simulation-panel.tsx (0 hunks)
  • client/apps/game/src/ui/components/worldmap/battles/combat-simulation-panel.tsx (1 hunks)
  • client/apps/game/src/ui/components/worldmap/battles/pillage-simulation-panel.tsx (0 hunks)
  • client/apps/game/src/ui/elements/select-biome.tsx (1 hunks)
  • client/apps/game/src/ui/elements/select-tier.tsx (1 hunks)
  • client/apps/game/src/ui/elements/select-troop.tsx (1 hunks)
  • client/apps/game/src/ui/modules/military/battle-view/battle-actions.tsx (2 hunks)
  • client/apps/game/src/ui/modules/military/battle-view/utils.tsx (1 hunks)
  • client/apps/game/src/ui/modules/simulation/combat-simulation.tsx (2 hunks)
  • client/apps/game/src/ui/modules/simulation/pillage-simulation.tsx (0 hunks)
  • packages/core/src/utils/battle-simulator.ts (0 hunks)
  • packages/core/src/utils/combat-simulator.ts (1 hunks)
  • packages/core/src/utils/index.ts (1 hunks)
💤 Files with no reviewable changes (5)
  • client/apps/game/src/ui/modules/simulation/pillage-simulation.tsx
  • client/apps/game/src/ui/components/worldmap/battles/battle-simulation-panel.tsx
  • client/apps/game/src/ui/components/navigation/config.tsx
  • packages/core/src/utils/battle-simulator.ts
  • client/apps/game/src/ui/components/worldmap/battles/pillage-simulation-panel.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
packages/core/src/utils/combat-simulator.ts

[error] 1-9: Avoid classes that contain only static members.

Prefer using simple functions instead of classes with only static members.

(lint/complexity/noStaticOnlyClass)


[error] 43-147: Avoid classes that contain only static members.

Prefer using simple functions instead of classes with only static members.

(lint/complexity/noStaticOnlyClass)


[error] 89-89: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 91-91: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 93-93: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 99-99: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 101-101: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 101-101: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 111-111: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 112-112: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 112-112: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 112-112: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 112-112: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 122-122: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 126-126: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 128-128: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 128-128: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 129-129: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 130-130: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 135-135: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 137-137: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 137-137: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 138-138: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 139-139: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)

🔇 Additional comments (13)
client/apps/game/src/ui/modules/military/battle-view/utils.tsx (5)

21-40: LGTM! Probability calculation remains valid.

The function correctly calculates battle success probability based on army strengths and health ratios.


42-61: LGTM! Core battle calculation functions remain valid.

These utility functions continue to provide essential calculations for the new combat simulation system.


63-71: LGTM! Resource calculation aligns with new system.

The function correctly calculates maximum stolen resources using troop counts and success probability, maintaining consistency with the new rounding down approach.


73-91: LGTM! Consistent use of roundDownToPrecision.

The function maintains proper precision in troop calculations while consistently applying the new rounding down behavior.


2-2: Verify the impact of changing from roundUpToPrecision to roundDownToPrecision.

The change in rounding behavior could affect resource calculations. Please ensure this change is intentional and aligns with the new combat simulation system.

✅ Verification successful

🏁 Script executed:

#!/bin/bash
# Search for other usages of roundUpToPrecision to ensure consistency
rg "roundUpToPrecision" -A 2

Length of output: 637


Rounding Behavior Change Verification

The search shows that while roundUpToPrecision remains in use elsewhere (in both game and landing utilities), the military battle view now deliberately imports and uses roundDownToPrecision. This indicates an intentional change in rounding behavior for this module. Please verify that the new downward rounding aligns correctly with the updated combat simulation design.

packages/core/src/utils/combat-simulator.ts (3)

11-15: Enums look good.
No immediate concerns on TroopType. Implementation is concise and correct for enumerating troop categories.


17-34: Long enumeration for Biome is fine, but consider fallback handling.
It might be beneficial to handle unrecognized biome types or to confirm that the input domain is strictly validated elsewhere.


55-84: Gracefully handle missing biome or troop type in getBiomeBonus.
You already provide a default of 0 for missing biome/troop combos using ?. and ?? 0. This approach looks safe, but ensure all call sites pass valid biome/troop values.

packages/core/src/utils/index.ts (1)

2-2: Swapping from battle-simulator to combat-simulator is aligned with the new architecture.
Looks correct. No further concerns.

client/apps/game/src/ui/modules/simulation/combat-simulation.tsx (1)

5-20: Transition to CombatSimulation and updated UI references looks cohesive.

  1. Line 5: Importing CombatSimulationPanel is consistent with your new approach.
  2. Line 7: Rename from BattleSimulation to CombatSimulation is straightforward and logical.
  3. Line 16 & 18: Updating the window title and width are appropriate UI adjustments.
  4. Line 20: Rendering <CombatSimulationPanel /> is clear and consistent.
client/apps/game/src/ui/components/military/entities-army-table.tsx (1)

20-25: LGTM! Clean transition from BattleSimulation to CombatSimulation.

The changes correctly implement the removal of PillageSimulation and the transition to CombatSimulation.

client/apps/game/src/ui/components/worldmap/battles/combat-simulation-panel.tsx (1)

22-24: Consider handling the case when troop type is not found.

The function returns ResourcesIds.Knight as a default value when the troop type is not found. This could mask potential errors if an invalid troop type is provided.

Consider throwing an error or logging a warning when the troop type is not found:

-  return TROOP_RESOURCES.find((t) => t.type === troopType)?.resourceId || ResourcesIds.Knight;
+  const resource = TROOP_RESOURCES.find((t) => t.type === troopType);
+  if (!resource) {
+    console.warn(`Invalid troop type: ${troopType}`);
+    return ResourcesIds.Knight;
+  }
+  return resource.resourceId;
client/apps/game/src/ui/modules/military/battle-view/battle-actions.tsx (1)

238-238: LGTM! Changes align with PR objectives.

The replacement of dynamic troop loss calculations with static values of 0 aligns with the PR objective of removing the old battle simulator. The new combat simulation workflow will handle these calculations.

Also applies to: 242-242

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: 0

🧹 Nitpick comments (1)
client/apps/game/src/ui/utils/utils.ts (1)

51-58: Remove commented out code.

The commented out functions roundDownToPrecision and roundUpToPrecision are no longer used after removing the old battle simulator. While the comment suggests keeping them for later, it's better to rely on version control history for this purpose.

Apply this diff to remove the commented out code:

-// keep this for later
-// export function roundDownToPrecision(value: bigint, precision: number) {
-//   return BigInt(Number(value) - (Number(value) % Number(precision)));
-// }
-
-// export function roundUpToPrecision(value: bigint, precision: number) {
-//   return BigInt(Number(value) + (Number(precision) - (Number(value) % Number(precision))));
-// }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 250cf4f and c46e324.

📒 Files selected for processing (3)
  • client/apps/game/src/ui/components/worldmap/battles/troops.tsx (0 hunks)
  • client/apps/game/src/ui/modules/military/battle-view/utils.tsx (1 hunks)
  • client/apps/game/src/ui/utils/utils.ts (1 hunks)
💤 Files with no reviewable changes (1)
  • client/apps/game/src/ui/components/worldmap/battles/troops.tsx
🔇 Additional comments (1)
client/apps/game/src/ui/modules/military/battle-view/utils.tsx (1)

2-2: LGTM!

The import statement has been updated to include necessary dependencies from the @bibliothecadao/eternum package.

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: 0

♻️ Duplicate comments (1)
packages/core/src/utils/combat-simulator.ts (1)

131-141: Handle zero total troops case.

The battle simulation could fail when total troops is zero, leading to NaN results due to division by zero in Math.pow(totalTroops, betaEff).

Apply this diff to handle the edge case:

 const totalTroops = attacker.troopCount + defender.troopCount;
+if (totalTroops === 0) {
+  return { attackerDamage: 0, defenderDamage: 0 };
+}
 const betaEff = this.calculateEffectiveBeta(totalTroops);
🧰 Tools
🪛 Biome (1.9.4)

[error] 132-132: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 136-136: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 138-138: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 138-138: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 139-139: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 140-140: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)

🧹 Nitpick comments (10)
packages/core/src/utils/combat-simulator.ts (5)

1-9: Refactor static-only class to utility functions.

Consider refactoring the Percentage class to utility functions for better maintainability and to follow TypeScript best practices.

-export class Percentage {
-  static _100() {
-    return 10_000;
-  }
-
-  static get(value: number, numerator: number) {
-    return (value * numerator) / Percentage._100();
-  }
-}
+const PERCENTAGE_BASE = 10_000;
+
+export const getPercentage = (value: number, numerator: number) => {
+  return (value * numerator) / PERCENTAGE_BASE;
+};
🧰 Tools
🪛 Biome (1.9.4)

[error] 1-9: Avoid classes that contain only static members.

Prefer using simple functions instead of classes with only static members.

(lint/complexity/noStaticOnlyClass)


53-206: Refactor static-only class to utility functions.

Consider refactoring the CombatSimulator class to utility functions for better maintainability and to follow TypeScript best practices.

-export class CombatSimulator {
+const BASE_T1_VALUE = 100;
+const STAMINA_ATTACK_THRESHOLD = 30;
+const BASE_DAMAGE_FACTOR = 3.5;
+const BETA_SMALL = 0.25;
+const BETA_LARGE = 0.12;
+const C0 = 100_000;
+const DELTA = 50_000;
+
+export const getBiomeBonus = (troopType: TroopType, biome: Biome): number => {
  // ... existing implementation
+};
+
+export const simulateBattle = (
+  attacker: Army,
+  defender: Army,
+  biome: Biome,
+): { attackerDamage: number; defenderDamage: number } => {
  // ... existing implementation
+};
+
+export const getDefaultParameters = (): CombatParameters => {
  // ... existing implementation
+};
+
+export const simulateBattleWithParams = (
+  attacker: Army,
+  defender: Army,
+  biome: Biome,
+  params: CombatParameters,
+): { attackerDamage: number; defenderDamage: number } => {
  // ... existing implementation
+};
🧰 Tools
🪛 Biome (1.9.4)

[error] 53-206: Avoid classes that contain only static members.

Prefer using simple functions instead of classes with only static members.

(lint/complexity/noStaticOnlyClass)


[error] 99-99: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 101-101: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 103-103: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 109-109: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 111-111: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 111-111: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 121-121: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 122-122: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 122-122: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 122-122: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 122-122: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 132-132: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 136-136: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 138-138: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 138-138: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 139-139: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 140-140: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 145-145: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 147-147: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 147-147: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 148-148: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 149-149: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 161-161: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 162-162: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 163-163: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 164-164: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 165-165: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 166-166: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 167-167: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 187-187: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 187-187: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 188-188: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 189-189: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 196-196: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 196-196: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 197-197: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 198-198: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


1-9: Refactor static-only class into standalone functions.

Consider refactoring the Percentage class into standalone functions for better maintainability and clarity:

-export class Percentage {
-  static _100() {
-    return 10_000;
-  }
-
-  static get(value: number, numerator: number) {
-    return (value * numerator) / Percentage._100();
-  }
-}
+const PERCENTAGE_BASE = 10_000;
+
+export const getPercentageValue = (value: number, numerator: number) => {
+  return (value * numerator) / PERCENTAGE_BASE;
+};
🧰 Tools
🪛 Biome (1.9.4)

[error] 1-9: Avoid classes that contain only static members.

Prefer using simple functions instead of classes with only static members.

(lint/complexity/noStaticOnlyClass)


53-205: Refactor static-only class into a module.

Consider refactoring the CombatSimulator class into a module with standalone functions for better maintainability and to avoid static method confusion.

-export class CombatSimulator {
+// Constants
+const BASE_T1_VALUE = 100;
+const STAMINA_ATTACK_THRESHOLD = 30;
+const BASE_DAMAGE_FACTOR = 3.5;
+
+// Combat scaling parameters
+const BETA_SMALL = 0.25;
+const BETA_LARGE = 0.12;
+const C0 = 100_000;
+const DELTA = 50_000;
+
+export const getBiomeBonus = (troopType: TroopType, biome: Biome): number => {
  // ... existing getBiomeBonus implementation
};

+export const simulateBattle = (
  attacker: Army,
  defender: Army,
  biome: Biome,
): { attackerDamage: number; defenderDamage: number } => {
  // ... existing simulateBattle implementation
};

+// ... other functions
🧰 Tools
🪛 Biome (1.9.4)

[error] 53-206: Avoid classes that contain only static members.

Prefer using simple functions instead of classes with only static members.

(lint/complexity/noStaticOnlyClass)


[error] 99-99: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 101-101: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 103-103: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 109-109: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 111-111: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 111-111: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 121-121: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 122-122: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 122-122: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 122-122: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 122-122: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 132-132: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 136-136: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 138-138: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 138-138: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 139-139: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 140-140: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 145-145: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 147-147: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 147-147: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 148-148: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 149-149: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 161-161: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 162-162: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 163-163: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 164-164: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 165-165: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 166-166: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 167-167: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 187-187: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 187-187: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 188-188: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 189-189: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 196-196: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 196-196: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 197-197: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 198-198: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


171-205: Reduce code duplication in battle simulation methods.

The simulateBattleWithParams method duplicates most of the logic from simulateBattle. Consider extracting the common logic into a shared helper function.

+private static calculateDamage(
+  attacker: Army,
+  defender: Army,
+  biome: Biome,
+  totalTroops: number,
+  betaEff: number,
+  baseDamageFactor: number,
+): number {
+  return (
+    (baseDamageFactor *
+      attacker.troopCount *
+      (this.getTierValue(attacker.tier) / this.getTierValue(defender.tier)) *
+      this.calculateStaminaModifier(attacker.stamina, true) *
+      this.getBiomeBonus(attacker.troopType, biome)) /
+    Math.pow(totalTroops, betaEff)
+  );
+}

 public static simulateBattleWithParams(
   attacker: Army,
   defender: Army,
   biome: Biome,
   params: CombatParameters,
 ): { attackerDamage: number; defenderDamage: number } {
   const totalTroops = attacker.troopCount + defender.troopCount;
+  if (totalTroops === 0) {
+    return { attackerDamage: 0, defenderDamage: 0 };
+  }
   const betaEff =
     params.betaSmall -
     (params.betaSmall - params.betaLarge) * ((Math.tanh((totalTroops - params.c0) / params.delta) + 1) / 2);

-  // Calculate attacker damage
-  const attackerDamage =
-    (params.baseDamageFactor *
-      attacker.troopCount *
-      (this.getTierValue(attacker.tier) / this.getTierValue(defender.tier)) *
-      this.calculateStaminaModifier(attacker.stamina, true) *
-      this.getBiomeBonus(attacker.troopType, biome)) /
-    Math.pow(totalTroops, betaEff);
+  const attackerDamage = this.calculateDamage(
+    attacker,
+    defender,
+    biome,
+    totalTroops,
+    betaEff,
+    params.baseDamageFactor,
+  );

-  // Calculate defender damage
-  const defenderDamage =
-    (params.baseDamageFactor *
-      defender.troopCount *
-      (this.getTierValue(defender.tier) / this.getTierValue(attacker.tier)) *
-      this.calculateStaminaModifier(defender.stamina, false) *
-      this.getBiomeBonus(defender.troopType, biome)) /
-    Math.pow(totalTroops, betaEff);
+  const defenderDamage = this.calculateDamage(
+    defender,
+    attacker,
+    biome,
+    totalTroops,
+    betaEff,
+    params.baseDamageFactor,
+  );

   return {
     attackerDamage,
     defenderDamage,
   };
 }
🧰 Tools
🪛 Biome (1.9.4)

[error] 187-187: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 187-187: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 188-188: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 189-189: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 196-196: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 196-196: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 197-197: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 198-198: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)

client/apps/game/src/ui/components/worldmap/battles/combat-simulation-panel.tsx (5)

55-61: Add validation feedback for troop type selection.

Consider providing visual feedback when an invalid troop type is selected.

 <SelectTroop
   onSelect={(troopType) => {
     if (troopType) {
       onChange({ ...army, troopType });
+    } else {
+      // Show validation error
+      console.error('Invalid troop type selected');
     }
   }}
   defaultValue={army.troopType}
+  error={!army.troopType}
 />

142-151: Improve keyboard shortcut handling.

Consider using a keyboard shortcut library for better cross-browser compatibility and to avoid potential conflicts.

+import { useHotkeys } from 'react-hotkeys-hook';
+
-useEffect(() => {
-  const handleKeyDown = (e: KeyboardEvent) => {
-    if (e.ctrlKey && e.shiftKey && e.key.toLowerCase() === "s") {
-      setShowParameters((prev) => !prev);
-    }
-  };
-
-  window.addEventListener("keydown", handleKeyDown);
-  return () => window.removeEventListener("keydown", handleKeyDown);
-}, []);
+useHotkeys('ctrl+shift+s', () => setShowParameters(prev => !prev), {
+  preventDefault: true,
+  enableOnFormTags: true,
+});

163-165: Extract stamina cost constant.

Consider extracting the hardcoded stamina cost value to a constant or configuration.

+const STAMINA_COST = 30;
+
-const staminaCost = 30;
-let newAttackerStamina = attacker.stamina - staminaCost;
-let newDefenderStamina = defender.stamina - Math.min(staminaCost, defender.stamina);
+let newAttackerStamina = attacker.stamina - STAMINA_COST;
+let newDefenderStamina = defender.stamina - Math.min(STAMINA_COST, defender.stamina);

86-123: Add input validation and tooltips for combat parameters.

Consider the following improvements:

  1. Add input validation to prevent invalid numeric values.
  2. Add tooltips to explain the purpose and impact of each parameter.
 <input
   type="number"
   value={value}
   onChange={(e) => {
     const newValue = parseFloat(e.target.value);
-    if (!isNaN(newValue)) {
+    if (!isNaN(newValue) && newValue >= 0 && 
+        (key === "c0" || key === "delta" ? newValue <= 1000000 : newValue <= 1000)) {
       onParametersChange({ ...parameters, [key]: newValue });
     }
   }}
   min={0}
   max={key === "c0" || key === "delta" ? 1000000 : 1000}
   step={key === "c0" || key === "delta" ? 1000 : 0.01}
+  title={getParameterDescription(key)} // Add a helper function for parameter descriptions
   className="px-2 py-1 bg-dark-brown border border-gold/20 rounded text-gold focus:outline-none focus:border-gold/40"
 />

167-172: Extract victory condition logic to reduce duplication.

The victory condition logic is duplicated between stamina bonus calculation and victory display. Consider extracting it into a helper function.

+const determineWinner = (attackerTroopsLeft: number, defenderTroopsLeft: number) => {
+  if (attackerTroopsLeft <= 0 && defenderTroopsLeft > 0) {
+    return 'defender';
+  } else if (defenderTroopsLeft <= 0 && attackerTroopsLeft > 0) {
+    return 'attacker';
+  }
+  return null;
+};

-if (attackerTroopsLeft <= 0 && defenderTroopsLeft > 0) {
-  newDefenderStamina += 30;
-} else if (defenderTroopsLeft <= 0 && attackerTroopsLeft > 0) {
-  newAttackerStamina += 30;
-}
+const winner = determineWinner(attackerTroopsLeft, defenderTroopsLeft);
+if (winner === 'defender') {
+  newDefenderStamina += 30;
+} else if (winner === 'attacker') {
+  newAttackerStamina += 30;
+}

-{(attackerTroopsLeft <= 0 || defenderTroopsLeft <= 0) && (
+{winner && (
   <div className="mt-6 p-4 bg-green-900/30 border border-green-400/30 rounded-lg">
     <p className="text-green-400 font-bold text-center text-lg">
-      {attackerTroopsLeft <= 0 ? "Defender" : "Attacker"} Wins!
+      {winner === 'defender' ? "Defender" : "Attacker"} Wins!
       <span className="text-green-400/80 text-base ml-2">(+30 stamina bonus)</span>
     </p>
   </div>
 )}

Also applies to: 285-292

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c46e324 and d1aaad9.

📒 Files selected for processing (2)
  • client/apps/game/src/ui/components/worldmap/battles/combat-simulation-panel.tsx (1 hunks)
  • packages/core/src/utils/combat-simulator.ts (1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/core/src/utils/combat-simulator.ts

[error] 1-9: Avoid classes that contain only static members.

Prefer using simple functions instead of classes with only static members.

(lint/complexity/noStaticOnlyClass)


[error] 53-206: Avoid classes that contain only static members.

Prefer using simple functions instead of classes with only static members.

(lint/complexity/noStaticOnlyClass)


[error] 99-99: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 101-101: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 103-103: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 109-109: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 111-111: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 111-111: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 121-121: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 122-122: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 122-122: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 122-122: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 122-122: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 132-132: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 136-136: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 138-138: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 138-138: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 139-139: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 140-140: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 145-145: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 147-147: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 147-147: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 148-148: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 149-149: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 161-161: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 162-162: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 163-163: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 164-164: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 165-165: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 166-166: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 167-167: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 187-187: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 187-187: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 188-188: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 189-189: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 196-196: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 196-196: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 197-197: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 198-198: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)

🔇 Additional comments (2)
packages/core/src/utils/combat-simulator.ts (1)

126-156: Handle the case when total troops is zero.

Currently, Math.pow(totalTroops, betaEff) would give NaN if totalTroops is 0.

🧰 Tools
🪛 Biome (1.9.4)

[error] 132-132: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 136-136: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 138-138: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 138-138: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 139-139: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 140-140: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 145-145: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 147-147: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 147-147: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 148-148: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 149-149: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)

client/apps/game/src/ui/components/worldmap/battles/combat-simulation-panel.tsx (1)

26-78: LGTM!

The ArmyInput component is well-structured with proper input validation and follows React best practices.

@aymericdelab aymericdelab merged commit f65119a into next Feb 13, 2025
8 of 27 checks passed
@aymericdelab aymericdelab deleted the feat/combat-simulator branch February 13, 2025 09:38
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.

2 participants