search

BibliothecaDAO / eternum

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

Fix animation #953

Closed aymericdelab closed 3 months ago

aymericdelab commented 3 months ago

PR Type

Enhancement, Bug fix

Description

Changes walkthrough 📝

Relevant files
Enhancement
15 files
useArmies.tsx
Include Position in useEntityQuery for armies.                     
client/src/hooks/helpers/useArmies.tsx - Added `Has(Position)` to the `useEntityQuery` call.
+1/-0     
useExplore.tsx
Remove animation path state management from useExplore.   
client/src/hooks/helpers/useExplore.tsx
  • Removed animation path state management.
  • Simplified exploreHex function by removing animation path logic.
    		•  +0/-6     
    useTravel.tsx
    Remove animation path state management from useTravel.     
    client/src/hooks/helpers/useTravel.tsx
  • Removed animation path state management.
  • Simplified travelToHex function by removing animation path logic.
    		•  +0/-7     
    _mapStore.tsx
    Remove animation paths from MapStore.                                       
    client/src/hooks/store/_mapStore.tsx
  • Removed animationPaths and setAnimationPaths from MapStore interface
    and its implementation.
    		•  +0/-4     
    WarriorModel.tsx
    Simplify WarriorModel props by removing rotationY.             
    client/src/ui/components/models/armies/WarriorModel.tsx - Removed `rotationY` prop from `WarriorModel`.
    		+3/-12   
    Armies.tsx
    Remove enemy animation path updates from Armies component.
    client/src/ui/components/worldmap/armies/Armies.tsx
  • Removed useUpdateAnimationPathsForEnnemies hook and related imports.
    		•  +0/-60   
    Army.tsx
    Refactor Army component to use useArmyAnimation.                 
    client/src/ui/components/worldmap/armies/Army.tsx
  • Refactored Army component to use useArmyAnimation hook.
  • Removed animation path state management.
  • Simplified component by extracting ArmyFlag and ArmyHitBox.
    		•  +19/-145
    ArmyFlag.tsx
    Add ArmyFlag component for flag rendering.                             
    client/src/ui/components/worldmap/armies/ArmyFlag.tsx - Created new `ArmyFlag` component to handle flag rendering.
    		+28/-0   
    ArmyHitBox.tsx
    Add ArmyHitBox component for hitbox interactions.               
    client/src/ui/components/worldmap/armies/ArmyHitBox.tsx - Created new `ArmyHitBox` component to handle hitbox interactions.
    		+46/-0   
    ArmyInfoLabel.tsx
    Add visibility control to ArmyInfoLabel.                                 
    client/src/ui/components/worldmap/armies/ArmyInfoLabel.tsx - Added `visible` prop to `ArmyInfoLabel` for conditional rendering.
    		+7/-2     
    useArmyAnimation.tsx
    Add useArmyAnimation hook for army animations.                     
    client/src/ui/components/worldmap/armies/useArmyAnimation.tsx - Created `useArmyAnimation` hook to manage army animations.
    		+54/-0   
    utils.tsx
    Add utility functions for army animations.                             
    client/src/ui/components/worldmap/armies/utils.tsx - Added utility functions for army animations.
    		+14/-0   
    utils.tsx
    Add BFS pathfinding utility function.                                       
    client/src/ui/components/worldmap/hexagon/utils.tsx - Added `findShortestPathBFS` function to find paths using BFS.
    		+39/-0   
    BaseThreeTooltip.tsx
    Add visibility control to BaseThreeTooltip.                           
    client/src/ui/elements/BaseThreeTooltip.tsx
  • Added visible prop to BaseThreeTooltip for conditional rendering.
    		•  +3/-1     
    DojoHtml.tsx
    Add inline style for visibility control in DojoHtml.         
    client/src/ui/elements/DojoHtml.tsx - Added inline style to control visibility via opacity.
    		+1/-1     

    💡 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 19, 2024 6:00am
    github-actions[bot] commented 3 months ago

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5] 3
    🧪 Relevant tests No
    🔒 Security concerns No
    ⚡ Key issues to review Possible Bug:
    The removal of animation path state management in useExplore.tsx and useTravel.tsx might lead to issues where the UI does not update correctly after an entity has moved. This is because the previous mechanism of managing animations through a global state (animationPaths) has been removed, and it's unclear if the new method handles all cases.
    Refactoring Concern:
    The WarriorModel component has been simplified by removing the rotationY prop. Ensure that this does not affect the intended orientation of the models in the UI, as rotation might be a necessary feature for correct visual representation.
    github-actions[bot] commented 3 months ago

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Reintegrate the rotationY prop to the WarriorModel component for model rotation ___ **The WarriorModel component should accept and use the rotationY prop for rotating the model
    based on the army's direction. This prop was previously used and should be reintegrated to
    maintain functionality.** [client/src/ui/components/models/armies/WarriorModel.tsx [7-9]](https://github.com/BibliothecaDAO/eternum/pull/953/files#diff-d06b5122c20065f999ae44007babc11861248157d05a8a0136bab495bf6aaf51R7-R9) ```diff -export function WarriorModel({ ...props }: WarriorModelProps) { +export function WarriorModel({ rotationY, ...props }: WarriorModelProps) { const groupRef = useRef(null); const gltf = useGLTF("/models/chess_piece_king.glb"); + return ( + + + + ); ```
    Suggestion importance[1-10]: 9 Why: Reintegrating the `rotationY` prop is crucial for maintaining the functionality of rotating the model based on the army's direction. This suggestion addresses a significant issue that was introduced by the PR changes.
    		9
    Restore right-click interaction functionality to the Army component ___ **The Army component should handle right-click events to allow for interaction such as
    selecting or commanding the army. This functionality was present in the previous
    implementation and should be restored.** [client/src/ui/components/worldmap/armies/Army.tsx [18-20]](https://github.com/BibliothecaDAO/eternum/pull/953/files#diff-5208824bdf96eef1612b7e20852c5ac4bd3781394921a58d0bfb0117cfd7d5dcR18-R20) ```diff export const Army = React.memo(({ army }: ArmyProps & JSX.IntrinsicElements["group"]) => { const selectedEntity = useUIStore((state) => state.selectedEntity); + const setSelectedEntity = useUIStore((state) => state.setSelectedEntity); + const onRightClick = useCallback((e: React.MouseEvent) => { + e.preventDefault(); + setSelectedEntity(army.entity_id); + }, [setSelectedEntity, army.entity_id]); ```
    Suggestion importance[1-10]: 8 Why: Restoring the right-click interaction functionality is important for user interaction and control over the army units. This suggestion addresses a key feature that was removed in the PR changes.
    		8
    Enhance the animation effect by adding a CSS transition for opacity changes ___ **Consider using CSS transitions for opacity changes to enhance the animation effect. This
    can be done by adding a transition property to the style object. This will make the
    opacity change appear smoother, which is often more visually appealing.** [client/src/ui/elements/DojoHtml.tsx [15]](https://github.com/BibliothecaDAO/eternum/pull/953/files#diff-49c8316d205568a2369273c06568569fb50640fb30c8071cbf9015537d9ca32aR15-R15) ```diff - + ```
    Suggestion importance[1-10]: 8 Why: The suggestion to add a CSS transition for opacity changes is valid and improves the user experience by making the visibility changes smoother. It is a minor enhancement but adds to the visual appeal.
    		8
    Maintainability
    Consolidate duplicate isSelected state calculations in the Army component ___ **The isSelected state calculation is duplicated in the Army component. Consider
    consolidating these calculations to improve maintainability and reduce redundancy.** [client/src/ui/components/worldmap/armies/Army.tsx [34-36]](https://github.com/BibliothecaDAO/eternum/pull/953/files#diff-5208824bdf96eef1612b7e20852c5ac4bd3781394921a58d0bfb0117cfd7d5dcR34-R36) ```diff +// Define isSelected once and use it throughout the component const isSelected = useMemo(() => { return (selectedEntity?.id || 0n) === BigInt(army.entity_id); }, [selectedEntity, army.entity_id]); ```
    Suggestion importance[1-10]: 7 Why: Consolidating the `isSelected` state calculation improves code maintainability and reduces redundancy, making the code cleaner and easier to manage.
    		7
    Possible issue
    Ensure correct positioning and orientation for ArmyFlag and CombatLabel in the Army component ___ **The Army component should ensure that the ArmyFlag and CombatLabel components are
    correctly positioned and oriented based on the army's position and rotation. This requires
    passing the rotationY and position props to these components, which seems to be overlooked
    in the current implementation.** [client/src/ui/components/worldmap/armies/Army.tsx [40-43]](https://github.com/BibliothecaDAO/eternum/pull/953/files#diff-5208824bdf96eef1612b7e20852c5ac4bd3781394921a58d0bfb0117cfd7d5dcR40-R43) ```diff - + ```
    Suggestion importance[1-10]: 6 Why: Ensuring the correct positioning and orientation of `ArmyFlag` and `CombatLabel` components is important for visual accuracy. However, the impact is less critical compared to the other suggestions.
    		6