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: wrong initial position of hexception #1188

Merged
merged 5 commits into from
Aug 11, 2024
Merged

Conversation

edisontim
Copy link
Collaborator

@edisontim edisontim commented Aug 9, 2024

PR Type

Bug fix, Enhancement


Description

  • Refactored SceneManager to handle undefined initial scene and removed hexCoords parameter from switchScene method.
  • Updated setup methods in HexagonScene and its subclasses to remove hexCoords parameter and use LocationManager for coordinates.
  • Improved URL handling in WorldmapScene and various UI components by using new methods in the Position class.
  • Ensured non-nullable return values in LocationManager methods.
  • Minor formatting improvements for better readability.

Changes walkthrough 📝

Relevant files
Formatting
1 files
GameRenderer.ts
Minor formatting improvement in `handleURLChange` method 

client/src/three/GameRenderer.ts

  • Added a blank line for better readability in handleURLChange method.
  • +1/-0     
    Enhancement
    9 files
    SceneManager.ts
    Refactor SceneManager to handle undefined initial scene   

    client/src/three/SceneManager.ts

  • Changed currentScene initialization to undefined.
  • Removed hexCoords parameter from switchScene method.
  • Updated method calls to handle potential undefined values.
  • +6/-7     
    LocationManager.ts
    Ensure non-nullable return values in LocationManager methods

    client/src/three/helpers/LocationManager.ts

  • Changed return type of getCol and getRow methods to non-nullable.
  • +2/-2     
    HexagonScene.ts
    Simplify setup method in HexagonScene                                       

    client/src/three/scenes/HexagonScene.ts

    • Removed hexCoords parameter from setup method.
    +1/-1     
    Hexception.ts
    Refactor Hexception setup to use LocationManager coordinates

    client/src/three/scenes/Hexception.ts

  • Updated setup method to retrieve coordinates from LocationManager.
  • Adjusted camera and tile manager setup accordingly.
  • +11/-7   
    Worldmap.ts
    Improve URL handling and biome retrieval in WorldmapScene

    client/src/three/scenes/Worldmap.ts

  • Updated onHexagonDoubleClick to change URL and dispatch event.
  • Refactored biome retrieval to use Position class.
  • +8/-7     
    Position.ts
    Add method for hex location URLs in Position class             

    client/src/types/Position.ts

    • Added toHexLocationUrl method for generating hex location URLs.
    +6/-1     
    ArmyManagementCard.tsx
    Use toMapLocationUrl for URL generation in ArmyManagementCard

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

    • Updated URL generation to use toMapLocationUrl method.
    +2/-2     
    TopMiddleNavigation.tsx
    Use Position class methods for URL generation in TopMiddleNavigation

    client/src/ui/modules/navigation/TopMiddleNavigation.tsx

  • Updated URL generation to use toHexLocationUrl and toMapLocationUrl
    methods.
  • +3/-6     
    Steps.tsx
    Use toHexLocationUrl for URL generation in onboarding steps

    client/src/ui/modules/onboarding/Steps.tsx

    • Updated URL generation to use toHexLocationUrl method.
    +4/-8     

    💡 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:04pm

    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.

    The changes in this pull request improve the handling of positions and scene transitions in the game. The use of the LocationManager and Position class makes the code more consistent and easier to maintain. The separation of map and hex location URL generation is a good practice. Overall, these changes should lead to more reliable position handling and smoother scene transitions.

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

    client/src/three/scenes/Hexception.ts Show resolved Hide resolved
    client/src/three/scenes/Worldmap.ts Outdated Show resolved Hide resolved
    client/src/types/Position.ts Show resolved Hide resolved
    Copy link

    github-actions bot commented Aug 9, 2024

    PR Reviewer Guide 🔍

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

    Possible Bug
    The method switchScene no longer accepts hexCoords as a parameter, which changes the behavior of scene setup. Ensure that this change does not affect other parts of the application that rely on hexCoords being passed to setup.

    API Change
    The methods getCol and getRow have been changed to always return a number instead of possibly returning null. This could lead to unexpected behavior if the URL parameters are not set, as Number(null) will convert to 0, which might not be the intended default value.

    Refactoring Impact
    The setup method in HexceptionScene has been significantly refactored to use LocationManager for getting column and row values instead of receiving them directly. This change should be thoroughly tested to ensure it integrates well with the rest of the application.

    Copy link

    github-actions bot commented Aug 9, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add a check to ensure the scene object is not undefined before invoking methods on it

    Consider checking if scene is not undefined before calling changeScene to avoid
    potential runtime errors if scene is not found in the map.

    client/src/three/SceneManager.ts [27]

    -this.scenes.get(this.getCurrentScene()!)?.changeScene(sceneName);
    +const currentScene = this.scenes.get(this.getCurrentScene()!);
    +if (currentScene) {
    +  currentScene.changeScene(sceneName);
    +}
     
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential runtime error by ensuring that the scene object is not undefined before invoking methods on it, which is a crucial improvement for code stability and reliability.

    9
    Add a check to ensure biome is not undefined before accessing its method

    Ensure that biome is not undefined before calling getBiome to prevent potential
    runtime errors.

    client/src/three/scenes/Worldmap.ts [187]

    -const biome = this.biome.getBiome(biomePosition.x, biomePosition.y);
    +const biome = this.biome ? this.biome.getBiome(biomePosition.x, biomePosition.y) : undefined;
     
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential runtime error by ensuring that biome is not undefined before accessing its method, which is crucial for preventing crashes and ensuring code stability.

    9
    Enhancement
    Conditionally call setup to ensure it's only executed when necessary

    Replace the direct call to setup() in the constructor with a conditional check to
    ensure that setup is only called when necessary, or consider initializing in a
    lifecycle method if applicable.

    client/src/three/scenes/Hexception.ts [72]

    -this.setup();
    +if (this.shouldSetup) {
    +  this.setup();
    +}
     
    Suggestion importance[1-10]: 7

    Why: This suggestion improves the code by adding a conditional check before calling setup, which can prevent unnecessary executions and improve performance. However, it is not as critical as a bug fix.

    7
    Maintainability
    Use a more descriptive variable name for the THREE.Object3D instance

    Consider using a more descriptive variable name than dummy for the THREE.Object3D
    instance to improve code readability.

    client/src/three/scenes/Worldmap.ts [251]

    -const dummy = new THREE.Object3D();
    +const placeholderObject = new THREE.Object3D();
     
    Suggestion importance[1-10]: 6

    Why: This suggestion improves code readability and maintainability by using a more descriptive variable name. While beneficial, it is a minor enhancement compared to bug fixes.

    6

    @aymericdelab
    Copy link
    Collaborator

    linter

    @aymericdelab aymericdelab merged commit 7c80f13 into main Aug 11, 2024
    21 checks passed
    @aymericdelab aymericdelab deleted the fix/hexception-position branch August 11, 2024 19:05
    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.

    2 participants