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/models #1191

Merged
merged 5 commits into from
Aug 11, 2024
Merged

Fix/models #1191

merged 5 commits into from
Aug 11, 2024

Conversation

bob0005
Copy link
Collaborator

@bob0005 bob0005 commented Aug 9, 2024

User description

Closes #1179
Closes #1166
Closes #1161


PR Type

Enhancement, Bug fix


Description

  • Added capacityModel and weightModel components to ArmyMovementManager and integrated capacity and weight checks in exploration validation.
  • Implemented _getArmyRemainingCapacity method in ArmyMovementManager.
  • Introduced StructureModelPaths and StructureLabelPaths in StructureManager for dynamic loading of structure models.
  • Updated StructureManager to handle multiple structure types and manage them dynamically.
  • Integrated totalStructures tracking in WorldmapScene and updated structure handling to use StructureManager.
  • Added cache invalidation for structure updates in WorldmapScene.

Changes walkthrough 📝

Relevant files
Enhancement
ArmyMovementManager.ts
Add capacity and weight checks for army movement                 

client/src/dojo/modelManager/ArmyMovementManager.ts

  • Added capacityModel and weightModel components.
  • Integrated capacity and weight checks in exploration validation.
  • Implemented _getArmyRemainingCapacity method.
  • +26/-0   
    StructureManager.ts
    Dynamic loading and management of structure models             

    client/src/three/components/StructureManager.ts

  • Introduced StructureModelPaths and StructureLabelPaths.
  • Implemented dynamic loading of structure models.
  • Updated structure management to handle multiple structure types.
  • +95/-43 
    Worldmap.ts
    Integrate structure management and cache invalidation       

    client/src/three/scenes/Worldmap.ts

  • Integrated totalStructures tracking.
  • Updated structure handling to use StructureManager.
  • Added cache invalidation for structure updates.
  • +21/-4   

    💡 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 Aug 9, 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 9, 2024 7:07pm

    Copy link
    Contributor

    mentatbot bot commented Aug 9, 2024

    You are out of MentatBot reviews. Your usage will refresh August 12 at 07:00 AM.

    Copy link

    github-actions bot commented Aug 9, 2024

    PR Reviewer Guide 🔍

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

    Possible Bug
    The method _getArmyRemainingCapacity calculates the remaining capacity using BigInt division, which might lead to incorrect results due to rounding. Consider revising the calculation logic to ensure accuracy.

    Code Smell
    The loadModels method uses a loop with async operations inside but does not handle potential simultaneous async operations well. Consider refactoring to use Promise.all for better efficiency and error handling.

    Redundant Code
    There are commented out lines for StructureType.Hyperstructure model paths which could be confusing and should be cleaned up if not needed.

    Copy link

    github-actions bot commented Aug 9, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add a check to handle undefined armyEntity before accessing its properties

    Consider handling the case where armyEntity is undefined before accessing its
    properties to prevent runtime errors.

    client/src/dojo/modelManager/ArmyMovementManager.ts [414-416]

    -const knights = armyEntity?.troops.knight_count || 0n;
    -const crossbowmen = armyEntity?.troops.crossbowman_count || 0n;
    -const paladins = armyEntity?.troops.paladin_count || 0n;
    +if (!armyEntity) return 0n;
    +const knights = armyEntity.troops.knight_count || 0n;
    +const crossbowmen = armyEntity.troops.crossbowman_count || 0n;
    +const paladins = armyEntity.troops.paladin_count || 0n;
     
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential runtime error by ensuring armyEntity is defined before accessing its properties, which is crucial for preventing crashes.

    9
    Ensure BigInt is used for all operations in troopQty calculation

    Use BigInt for troopQty calculation to ensure correct type handling and avoid
    potential bugs due to type mismatch.

    client/src/dojo/modelManager/ArmyMovementManager.ts [417]

    -const troopQty = (knights + crossbowmen + paladins) / BigInt(EternumGlobalConfig.resources.resourcePrecision);
    +const troopQty = BigInt(knights + crossbowmen + paladins) / BigInt(EternumGlobalConfig.resources.resourcePrecision);
     
    Suggestion importance[1-10]: 8

    Why: This suggestion ensures type consistency in calculations involving BigInt, which is important for preventing potential bugs due to type mismatches.

    8
    Enhancement
    Add error handling for model loading promises to manage failures gracefully

    Add error handling for the loadModels method to manage failed promises and prevent
    the application from hanging due to unhandled promise rejections.

    client/src/three/components/StructureManager.ts [81]

    -this.modelLoadPromises.push(loadPromise);
    +this.modelLoadPromises.push(loadPromise.catch(error => {
    +    console.error("Failed to load model:", error);
    +    return Promise.resolve(); // Resolve to prevent blocking other models
    +}));
     
    Suggestion importance[1-10]: 8

    Why: Adding error handling for model loading promises improves the robustness of the application by preventing it from hanging due to unhandled promise rejections.

    8
    Replace the placeholder model path with the correct model path

    Replace the placeholder model for StructureType.Hyperstructure with the intended
    model path to ensure the correct assets are loaded.

    client/src/three/components/StructureManager.ts [15]

    -[StructureType.Hyperstructure]: "models/buildings/farm.glb", // USING PLACEHOLDER MODEL
    +[StructureType.Hyperstructure]: "models/buildings/hyperstructure.glb",
     
    Suggestion importance[1-10]: 7

    Why: Replacing the placeholder model with the correct one improves the accuracy and visual representation of the application, but it is not critical for functionality.

    7

    @aymericdelab aymericdelab merged commit 45539a8 into main Aug 11, 2024
    21 checks passed
    @aymericdelab aymericdelab deleted the fix/models branch August 11, 2024 19:12
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    2 participants