BibliothecaDAO / eternum

onchain eternal game
https://alpha-eternum.realms.world
MIT License
46 stars 34 forks source link

Army sounds #966

Closed aymericdelab closed 3 months ago

aymericdelab commented 3 months ago

PR Type

Enhancement, Bug fix


Description


Changes walkthrough ๐Ÿ“

Relevant files
Enhancement
8 files
_mapStore.tsx
Add travelPaths state to map store slice.                               

client/src/hooks/store/_mapStore.tsx - Added `travelPaths` state to the map store slice.
+1/-0     
useUISound.tsx
Add new sound hooks and selectors for army actions.           

client/src/hooks/useUISound.tsx
  • Added new sound selectors for unit selection and marching.
  • Introduced repeat method for looping sounds.
  • Added hooks for playing marching and selected army sounds.
  • +60/-0   
    WarriorModel.tsx
    Enhance WarriorModel with hover effects and props.             

    client/src/ui/components/models/armies/WarriorModel.tsx
  • Added WarriorModelProps type.
  • Integrated hover state management and effects.
  • Included ArmyHitBox component.
  • +21/-4   
    Army.tsx
    Update Army component to pass props and manage animation.

    client/src/ui/components/worldmap/armies/Army.tsx
  • Updated useArmyAnimation hook call to include isMine.
  • Passed army prop to WarriorModel.
  • +3/-5     
    ArmyHitBox.tsx
    Add sound effects and refactor ArmyHitBox component.         

    client/src/ui/components/worldmap/armies/ArmyHitBox.tsx
  • Added sound effects for hover and selection.
  • Refactored to use hovered state from props.
  • +27/-11 
    useArmyAnimation.tsx
    Add marching sound effect to army animation.                         

    client/src/ui/components/worldmap/armies/useArmyAnimation.tsx
  • Added marching sound effect for army animation.
  • Adjusted animation frame logic.
  • +15/-10 
    HexLayers.tsx
    Add mouse event handlers for click and drag actions.         

    client/src/ui/components/worldmap/hexagon/HexLayers.tsx - Added mouse event handlers to manage click and drag actions.
    +32/-2   
    HoveredHexagon.tsx
    Add shovel sound effect on travel path detection.               

    client/src/ui/components/worldmap/hexagon/HoveredHexagon.tsx - Added shovel sound effect when a travel path is detected.
    +4/-0     
    Bug fix
    1 files
    useEventHandlers.tsx
    Add null check for hoveredHexRef in event handler.             

    client/src/ui/components/worldmap/hexagon/useEventHandlers.tsx - Added null check for `hoveredHexRef` in `handleArmyActionClick`.
    +1/-0     

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

    vercel[bot] commented 3 months ago

    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 Jun 20, 2024 1:57pm
    github-actions[bot] commented 3 months ago

    PR Reviewer Guide ๐Ÿ”

    โฑ๏ธ Estimated effort to review [1-5] 4
    ๐Ÿงช Relevant tests No
    ๐Ÿ”’ Security concerns No
    โšก Key issues to review Possible Bug:
    The useArmyAnimation hook in useArmyAnimation.tsx uses the position object directly in the dependency array of useEffect. This could lead to excessive re-renders or unexpected behavior if the position object is recreated on each render. Consider using destructured properties of position or memoizing the object.
    Performance Concern:
    The useArmyAnimation hook also plays a sound effect every time the army moves. This could lead to performance issues or a poor user experience if the sound effect is too frequent or too loud. Consider adding conditions to limit when the sound is played.
    Code Clarity:
    In ArmyHitBox.tsx, the onRightClick function is complex and does multiple things (setting selected entity and playing sounds). It might be beneficial to split this into smaller, more focused functions.
    github-actions[bot] commented 3 months ago

    PR Code Suggestions โœจ

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Add error handling for sound playback to enhance application robustness ___ **Consider adding error handling or a fallback mechanism for the sound playback functions to
    handle cases where the sound file might not load or play correctly. This can prevent the
    application from crashing or behaving unexpectedly due to audio issues.** [client/src/hooks/useUISound.tsx [83-87]](https://github.com/BibliothecaDAO/eternum/pull/966/files#diff-ed5fa149d5d1bf0c1b193d171a333e40fbab6f8043d4ecb30f9236761b83675fR83-R87) ```diff const repeat = useCallback(() => { if (sound) { sound.loop(true); play(); + } else { + console.error("Failed to load sound"); + // Optionally, implement a fallback sound or handle the error appropriately } }, [sound, play]); ```
    Suggestion importance[1-10]: 8 Why: Adding error handling for sound playback is a good enhancement to prevent the application from crashing or behaving unexpectedly due to audio issues. This suggestion is contextually accurate and improves the robustness of the application.
    8
    Check if animationPath is not empty before playing sounds to avoid unnecessary triggers ___ **Consider checking if animationPath is not empty before playing the marching sound to avoid
    unnecessary sound triggers when there is no movement.** [client/src/ui/components/worldmap/armies/useArmyAnimation.tsx [37]](https://github.com/BibliothecaDAO/eternum/pull/966/files#diff-ce247ef03061209e44162c6a4f2a687d3ad45f5b3843f3f16253cd738cf64335R37-R37) ```diff -isMine && playMarchingSound(); // Play marching sound when animation starts +if (isMine && animationPath && animationPath.length > 0) { + playMarchingSound(); // Play marching sound when animation starts +} ```
    Suggestion importance[1-10]: 6 Why: Checking if `animationPath` is not empty before playing the marching sound is a reasonable enhancement to avoid unnecessary sound triggers. This suggestion is contextually accurate and improves the user experience.
    6
    Best practice
    Improve type safety by specifying a more accurate type for event parameters ___ **Use a more specific type for the event parameter in event handlers to provide better type
    safety and to take advantage of TypeScript's type checking.** [client/src/ui/components/worldmap/armies/ArmyHitBox.tsx [39]](https://github.com/BibliothecaDAO/eternum/pull/966/files#diff-2dd1c84f2d9bb67d297d72c8c18345fa06b591e9f2ab0eae41c55f09407d7a35R39-R39) ```diff -const onPointerEnter = useCallback((e: any) => { +const onPointerEnter = useCallback((e: React.PointerEvent) => { e.stopPropagation(); }, []); ```
    Suggestion importance[1-10]: 7 Why: Using a more specific type for the event parameter in event handlers improves type safety and leverages TypeScript's type checking. This is a best practice that enhances code quality and maintainability.
    7
    Possible bug
    Initialize travelPaths with a default value to prevent runtime errors ___ **Initialize travelPaths with a default value to ensure that it is not undefined elsewhere
    in the code. This can prevent potential runtime errors when other parts of the application
    try to access or modify travelPaths.** [client/src/hooks/store/_mapStore.tsx [84]](https://github.com/BibliothecaDAO/eternum/pull/966/files#diff-6d3df53ed6fbfb737106e9d7b48fa7c3fa33d11521804ea10a7f5d2fad2517beR84-R84) ```diff -travelPaths: new Map(), +travelPaths: new Map() || new Map(), ```
    Suggestion importance[1-10]: 5 Why: Initializing `travelPaths` with a default value is a minor improvement to prevent potential runtime errors. However, the existing code already initializes `travelPaths` with a new Map, so the suggestion is not crucial.
    5