[client] fixes - Githubissues

r0man1337 commented 1 week ago

User description

Fixed zoom bug #999
Added active battle label icon #998
Improved UX of hover handler #997
Own armies icon color changed to green #995
All label icons are now smaller

PR Type

Bug fix, Enhancement

Description

Added a new battle label with a pulsing red effect using Billboard and Image components.
Simplified the hover handler logic in useEventHandlers.
Adjusted the scale, position, and color of the army label.
Set a minimum distance for camera transitions with a duration greater than 0.1.
Adjusted the scale of various labels (shard, hyper) and reduced the size of particles material.

Changes walkthrough 📝

Relevant files

Enhancement

Battles.tsx `Add battle label with pulsing red effect` client/src/ui/components/models/buildings/worldmap/Battles.tsx Added new imports for `Billboard`, `Image`, `useTexture`, `useFrame`, and `useRef`. Introduced a new texture `armyLabel` for the battle model. Added a pulsing red color effect to the battle label. Integrated `Billboard` and `Image` components to display the battle label.	+32/-2
Army.tsx `Adjust army label scale, position, and color` client/src/ui/components/worldmap/armies/Army.tsx Adjusted the scale and position of the army label. Changed the render order and added conditional color for the army label.	+4/-3
Camera.tsx `Set minimum distance for camera transitions` client/src/ui/utils/Camera.tsx Set a minimum distance for camera transitions with a duration greater than 0.1.	+4/-1
ShardsMines.tsx `Adjust shard label scale` client/src/ui/components/models/buildings/worldmap/ShardsMines.tsx - Adjusted the scale of the shard label.	+1/-1
InstancedCastles.tsx `Reduce particles material size` client/src/ui/components/models/buildings/worldmap/InstancedCastles.tsx - Reduced the size of the particles material.	+1/-1
Structures.tsx `Adjust hyper label scale and position` client/src/ui/components/models/buildings/worldmap/Structures.tsx - Adjusted the scale and position of the hyper label.	+1/-1

Bug fix

useEventHandlers.tsx

Simplify hover handler logic

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

Simplified the hover handler by directly using e.instanceId and
e.object.

+2/-5

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

vercel[bot] commented 1 week 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 27, 2024 5:38am

github-actions[bot] commented 1 week ago

PR Reviewer Guide 🔍

⏱️ Estimated effort to review [1-5]	3
🧪 Relevant tests	No
🔒 Security concerns	No
⚡ Key issues to review	Possible Bug: The color pulsing logic in `Battles.tsx` uses `Math.sin(time * 4) + 2` for the red component, which might not behave as expected since the result can exceed the typical color component range of [0, 1]. Consider normalizing or clamping the value.
	Performance Concern: The `useFrame` hook in `Battles.tsx` is used to update the color every frame, which might be performance-intensive depending on the complexity and number of instances. Consider optimizing or throttling updates.
	Code Clarity: The refactoring in `useEventHandlers.tsx` simplifies the hover handler but assumes properties `instanceId` and `object` are directly on the event `e`. Ensure this assumption is correct or document why this change is safe.

github-actions[bot] commented 1 week ago

PR Code Suggestions ✨

Category	Suggestion	Score
Possible bug	Ensure detailed sub-dependency versions are specified to prevent compatibility issues ___ The version specification for `version: 8.1.0(typescript@5.4.4)` should maintain the detailed dependency versions as previously included, to ensure specific sub-dependency versions are used, which can prevent potential compatibility issues. [pnpm-lock.yaml [13]](https://github.com/BibliothecaDAO/eternum/pull/1021/files#diff-32824c984905bb02bc7ffcef96a77addd1f1602cff71a11fbbfdd7f53ee026bbR13-R13) ```diff -version: 8.1.0(typescript@5.4.4) +version: 8.1.0(@swc/core@1.5.28)(postcss@8.4.38)(typescript@5.4.4) ``` Suggestion importance[1-10]: 10 Why: The suggestion correctly identifies the need to maintain detailed sub-dependency versions to avoid potential compatibility issues, which is crucial for the stability of the project.	10
	Add back necessary dependencies to avoid runtime errors ___ The version `version: 1.0.25` for `@web3mq/client` has removed the encoding dependency which was previously specified. If this dependency is still required for proper functionality, it should be added back to avoid runtime errors. [pnpm-lock.yaml [82]](https://github.com/BibliothecaDAO/eternum/pull/1021/files#diff-32824c984905bb02bc7ffcef96a77addd1f1602cff71a11fbbfdd7f53ee026bbR82-R82) ```diff -version: 1.0.25 +version: 1.0.25(encoding@0.1.13) ``` Suggestion importance[1-10]: 10 Why: Removing the encoding dependency for `@web3mq/client` could lead to runtime errors if it is still required. Adding it back ensures proper functionality.	10
	Ensure color values are within the valid range ___ The color setting in the `useFrame` hook can be improved for clarity and correctness. The current implementation might not correctly set the red component of the color. Consider adjusting the calculation to ensure the color values are within the valid range. [client/src/ui/components/models/buildings/worldmap/Battles.tsx [65]](https://github.com/BibliothecaDAO/eternum/pull/1021/files#diff-5f342f04015b76da66b7e97f1c3491940764ce97259e444fa972a6db2667517cR65-R65) ```diff -testRef.current.material.color.set(Math.sin(time * 4) + 2, 0, 0); +testRef.current.material.color.set(Math.max(0, Math.min(1, Math.sin(time * 4) + 2)), 0, 0); ``` Suggestion importance[1-10]: 9 Why: This suggestion addresses a potential bug by ensuring that color values are clamped within the valid range, which is crucial for correct rendering.	9
Possible issue	Avoid downgrading the lockfile version to maintain compatibility with newer features ___ The version downgrade from '9.0' to '6.0' for `lockfileVersion` might cause compatibility issues with newer features or bug fixes that were present in the higher version. It is generally recommended to use the latest stable version unless there is a specific reason to downgrade. [pnpm-lock.yaml [1]](https://github.com/BibliothecaDAO/eternum/pull/1021/files#diff-32824c984905bb02bc7ffcef96a77addd1f1602cff71a11fbbfdd7f53ee026bbR1-R1) ```diff -lockfileVersion: '6.0' +lockfileVersion: '9.0' ``` Suggestion importance[1-10]: 9 Why: Downgrading the lockfile version from '9.0' to '6.0' can lead to compatibility issues with newer features or bug fixes. It is generally advisable to use the latest stable version unless there is a specific reason for the downgrade.	9
Best practice	Extract texture configuration to enhance readability and maintainability ___ To enhance code readability and maintainability, consider extracting the texture configuration logic into a separate function or using a custom hook. [client/src/ui/components/models/buildings/worldmap/Battles.tsx [44-48]](https://github.com/BibliothecaDAO/eternum/pull/1021/files#diff-5f342f04015b76da66b7e97f1c3491940764ce97259e444fa972a6db2667517cR44-R48) ```diff -const armyLabel = useTexture("/textures/army_label.png", (texture) => { +const useConfiguredTexture = (path) => { + return useTexture(path, (texture) => { texture.colorSpace = THREE.SRGBColorSpace; texture.magFilter = THREE.LinearFilter; texture.minFilter = THREE.LinearFilter; -}); + }); +}; +const armyLabel = useConfiguredTexture("/textures/army_label.png"); ``` Suggestion importance[1-10]: 8 Why: Extracting the texture configuration logic into a separate function or custom hook improves code readability and maintainability by reducing repetition and encapsulating logic.	8
Best practice	Ensure all necessary type definitions are included for type safety ___ The version `version: 1.4.1(vite@5.2.13)` for `vite-plugin-top-level-await` has removed the `@types/node` dependency which was previously included. If this type definition is required for the project, consider adding it back to ensure type safety. [pnpm-lock.yaml [163]](https://github.com/BibliothecaDAO/eternum/pull/1021/files#diff-32824c984905bb02bc7ffcef96a77addd1f1602cff71a11fbbfdd7f53ee026bbR163-R163) ```diff -version: 1.4.1(vite@5.2.13) +version: 1.4.1(vite@5.2.13)(@types/node@20.14.2) ``` Suggestion importance[1-10]: 8 Why: Including the `@types/node` dependency is important for maintaining type safety in the project. However, the impact is slightly less critical compared to runtime dependencies.	8
Maintainability	Improve variable naming for clarity ___ Consider using a more descriptive variable name for `testRef` to better reflect its purpose, especially since it is used to reference a THREE.Mesh object for a pulsing color effect. [client/src/ui/components/models/buildings/worldmap/Battles.tsx [59]](https://github.com/BibliothecaDAO/eternum/pull/1021/files#diff-5f342f04015b76da66b7e97f1c3491940764ce97259e444fa972a6db2667517cR59-R59) ```diff -const testRef = useRef(null); +const pulsingColorRef = useRef(null); ``` Suggestion importance[1-10]: 7 Why: The suggestion to rename `testRef` to `pulsingColorRef` improves code readability and maintainability by making the variable's purpose clearer.	7
Performance	Use conditional rendering to improve performance ___ It is recommended to use conditional rendering for components that may not always need to be rendered, such as `Billboard` and `Image` in this context. This can help improve performance by avoiding unnecessary rendering. [client/src/ui/components/models/buildings/worldmap/Battles.tsx [73-83]](https://github.com/BibliothecaDAO/eternum/pull/1021/files#diff-5f342f04015b76da66b7e97f1c3491940764ce97259e444fa972a6db2667517cR73-R83) ```diff - - - +{showBattleLabel && ( + + + +)} ``` Suggestion importance[1-10]: 6 Why: While conditional rendering can improve performance by avoiding unnecessary rendering, the performance gain in this specific context may be minimal. However, it is still a good practice.	6

ponderingdemocritus commented 6 days ago

can you fix build error pls

BibliothecaDAO / eternum

[client] fixes #1021