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

fix army weight when exploring, add weigh + capacity in UI #1117

Merged
merged 11 commits into from
Aug 2, 2024

Conversation

bob0005
Copy link
Collaborator

@bob0005 bob0005 commented Jul 15, 2024

User description

Closes #1078


PR Type

Enhancement, Bug fix


Description

  • Added capacity and weight properties to ArmyInfo type and included Weight component in various hooks and functions.
  • Created ArmyCapacity component to display army capacity and weight.
  • Updated setCapacityConfig to remove precision modification for army weight.
  • Added ArmyCapacity component to display army capacity in multiple UI components including Entity, ArmyChip, ArmyList, and ArmyInfoLabel.
  • Added reward display for exploration in ActionInfo component.
  • Added capacity check for exploration in useTravelPath hook and canExplore function.

Changes walkthrough 📝

Relevant files
Enhancement
9 files
useArmies.tsx
Add weight and capacity properties to ArmyInfo type           

client/src/hooks/helpers/useArmies.tsx

  • Added capacity and weight properties to ArmyInfo type.
  • Included Weight component in various hooks and functions.
  • Calculated and set capacity and weight values in formatArmies
    function.
  • +26/-2   
    Entity.tsx
    Display army capacity in Entity component                               

    client/src/ui/components/entities/Entity.tsx

    • Added ArmyCapacity component to display army capacity in the UI.
    +2/-0     
    ArmyChip.tsx
    Display army capacity in ArmyChip component                           

    client/src/ui/components/military/ArmyChip.tsx

  • Added ArmyCapacity component to display army capacity in the ArmyChip
    component.
  • +2/-0     
    ArmyList.tsx
    Display army capacity in ArmyList component                           

    client/src/ui/components/military/ArmyList.tsx

  • Added ArmyCapacity component to display army capacity in the ArmyList
    component.
  • +2/-0     
    ArmyInfoLabel.tsx
    Display army capacity in ArmyInfoLabel component                 

    client/src/ui/components/worldmap/armies/ArmyInfoLabel.tsx

  • Added ArmyCapacity component to display army capacity in the
    ArmyInfoLabel component.
  • Updated army data using useArmyByArmyEntityId hook.
  • +7/-5     
    ActionInfo.tsx
    Display exploration reward in ActionInfo component             

    client/src/ui/components/worldmap/hexagon/ActionInfo.tsx

    • Added reward display for exploration in ActionInfo component.
    +16/-0   
    useTravelPath.tsx
    Add capacity check for exploration in useTravelPath hook 

    client/src/ui/components/worldmap/hexagon/useTravelPath.tsx

    • Added capacity check for exploration in useTravelPath hook.
    +8/-1     
    utils.tsx
    Add capacity check in canExplore function                               

    client/src/ui/components/worldmap/hexagon/utils.tsx

    • Added capacity check in canExplore function.
    +4/-1     
    ArmyCapacity.tsx
    Create ArmyCapacity component to display capacity and weight

    client/src/ui/elements/ArmyCapacity.tsx

  • Created ArmyCapacity component to display army capacity and weight.
  • +16/-0   
    Bug fix
    1 files
    index.ts
    Update setCapacityConfig to remove precision modification

    sdk/packages/eternum/src/config/index.ts

  • Updated setCapacityConfig to remove precision modification for army
    weight.
  • +2/-1     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    vercel bot commented Jul 15, 2024

    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 Aug 1, 2024 3:10pm

    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Possible Bug
    The calculation for capacity in line 107 might be incorrect or need clarification. It uses weight_gram and value from capacity and quantity respectively, which seems to be a mix-up or typo. It should be verified if weight_gram is the correct property to use here.

    Logic Change
    The canExplore function now includes a new parameter hasCapacity and checks if the army has enough capacity to carry the reward. This logic change should be thoroughly tested to ensure it behaves as expected under various scenarios.

    Copy link
    Contributor

    @mentatbot mentatbot bot left a comment

    Choose a reason for hiding this comment

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

    This pull request addresses the issue of army weight when exploring and adds weight and capacity information to the UI. The changes look good overall, with improvements in accuracy for army capacity calculations and better user feedback. The addition of capacity checks before exploration is a good safeguard. The code is well-structured and the comments are helpful. Consider adding more context about the weight check in the contract for future maintainers.

    Thanks for using MentatBot. Give comments a 👍 or 👎 to help me improve!

    client/src/hooks/helpers/useArmies.tsx Outdated Show resolved Hide resolved
    client/src/ui/components/entities/Entity.tsx Show resolved Hide resolved
    sdk/packages/eternum/src/config/index.ts Show resolved Hide resolved
    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add checks for existence of weight and capacity before calculations

    Consider checking for the existence of weight and capacity properties before
    performing calculations to avoid potential runtime errors if these properties are
    undefined.

    client/src/hooks/helpers/useArmies.tsx [107-110]

    -capacity:
    -  (Number(capacity?.weight_gram) * Number(quantity?.value || 0)) /
    -  EternumGlobalConfig.resources.resourceMultiplier,
    -weight: Number(weight?.value || 0) / EternumGlobalConfig.resources.resourceMultiplier,
    +capacity: capacity && quantity ? 
    +  (Number(capacity.weight_gram) * Number(quantity.value || 0)) /
    +  EternumGlobalConfig.resources.resourceMultiplier : 0,
    +weight: weight ? Number(weight.value || 0) / EternumGlobalConfig.resources.resourceMultiplier : 0,
     
    Suggestion importance[1-10]: 9

    Why: This suggestion prevents potential runtime errors by ensuring that weight and capacity are defined before performing calculations. This is crucial for maintaining the stability of the application.

    9
    Best practice
    Ensure non-negative values for weight and capacity by using Math.max

    Ensure that the weight and capacity values are always non-negative by applying a
    maximum function with zero. This prevents potential issues with negative values in
    the UI or calculations.

    client/src/hooks/helpers/useArmies.tsx [107-110]

    -capacity:
    +capacity: Math.max(0,
       (Number(capacity?.weight_gram) * Number(quantity?.value || 0)) /
    -  EternumGlobalConfig.resources.resourceMultiplier,
    -weight: Number(weight?.value || 0) / EternumGlobalConfig.resources.resourceMultiplier,
    +  EternumGlobalConfig.resources.resourceMultiplier
    +),
    +weight: Math.max(0, Number(weight?.value || 0) / EternumGlobalConfig.resources.resourceMultiplier),
     
    Suggestion importance[1-10]: 8

    Why: Ensuring non-negative values for weight and capacity is a good practice to prevent potential issues in the UI or calculations, enhancing the robustness of the code.

    8
    Maintainability
    Use a utility function for resource unit conversions to improve maintainability

    Replace the direct multiplication and division by
    EternumGlobalConfig.resources.resourceMultiplier with a utility function to handle
    resource conversions, improving code reusability and maintainability.

    client/src/hooks/helpers/useArmies.tsx [107-110]

    -capacity:
    -  (Number(capacity?.weight_gram) * Number(quantity?.value || 0)) /
    -  EternumGlobalConfig.resources.resourceMultiplier,
    -weight: Number(weight?.value || 0) / EternumGlobalConfig.resources.resourceMultiplier,
    +capacity: convertResource(
    +  Number(capacity?.weight_gram) * Number(quantity?.value || 0),
    +  'toBaseUnit'
    +),
    +weight: convertResource(Number(weight?.value || 0), 'fromBaseUnit'),
     
    Suggestion importance[1-10]: 7

    Why: Using a utility function for resource conversions improves code maintainability and reusability. However, it is less critical than preventing runtime errors.

    7

    Copy link
    Collaborator

    @edisontim edisontim left a comment

    Choose a reason for hiding this comment

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

    lgtm

    @edisontim edisontim merged commit d8fa2e2 into main Aug 2, 2024
    21 checks passed
    @edisontim edisontim deleted the fix/army-weight branch August 2, 2024 14:06
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    [client] review capacity limits for armies
    2 participants