Closed r0man1337 closed 1 month 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 12, 2024 3:22pm |
โฑ๏ธ Estimated effort to review [1-5] | 3 |
๐งช Relevant tests | No |
๐ Security concerns | No |
โก Key issues to review |
Possible Bug: The `depthWrite` property is set to `false` for various materials, which could lead to rendering issues where some objects might not appear as intended in certain scenes or under specific lighting conditions. This change should be thoroughly tested across different environments to ensure it does not negatively impact the visual quality or performance. |
Consistency Issue: The `renderOrder` property is set uniformly across various components. It's crucial to verify if this uniform setting does not conflict with the specific rendering needs of different objects, potentially causing some objects to render out of order. |
Category | Suggestion | Score |
Possible bug |
Add a conditional rendering to check if the
___
**Consider adding a condition to check if the | 9 |
Add a check to ensure that the object being pushed to the array is defined___ **Consider checking ifgroup[ind] is defined before pushing to borders to prevent potential errors when group[ind] is undefined.**
[client/src/ui/components/worldmap/hexagon/HexLayers.tsx [193-198]](https://github.com/BibliothecaDAO/eternum/pull/911/files#diff-32d953460a4689eeda29120db48be52828dc4e94c6e6dab11277ba52ce4db0feR193-R198)
```diff
+if (
+ group[ind] &&
+ !revealed.some((hex) => hex.col === tmpCol && hex.row === tmpRow) &&
+ !borders.some((hex) => hex.col === tmpCol && hex.row === tmpRow)
+) {
+ borders.push(group[ind]);
+}
-
```
Suggestion importance[1-10]: 9Why: This suggestion addresses a potential bug by adding a check to ensure that `group[ind]` is defined before pushing it to the `borders` array. This prevents possible runtime errors and improves code robustness. | 9 | |
Maintainability |
Use a loop to set properties for all elements in an array to improve code maintainability___ **Instead of settingdepthWrite individually for each material, use a loop to apply the property to all materials in the array. This reduces redundancy and makes the code easier to maintain.** [client/src/ui/components/models/biomes/TundraBiome.tsx [60-62]](https://github.com/BibliothecaDAO/eternum/pull/911/files#diff-7f5f1860eeecda9710975e81cfcb8e26d82bdc281b88bec4c37d45404da827fcR60-R62) ```diff -materials[0].depthWrite = false; -materials[1].depthWrite = false; -materials[2].depthWrite = false; +materials.forEach(material => material.depthWrite = false); ``` Suggestion importance[1-10]: 8Why: This suggestion improves code maintainability by reducing redundancy. Using a loop to set the `depthWrite` property for all materials makes the code cleaner and easier to update in the future. | 8 |
Verify and document the intention behind setting
___
**Ensure that setting | 7 | |
Remove commented-out code to clean up and clarify the codebase___ **Remove commented-out code for settingdepthWrite on materials unless there's a clear reason to keep it for future reference. Keeping commented code can lead to confusion and clutter.** [client/src/ui/components/models/biomes/TropicalSeasonalForestBiome.tsx [56-58]](https://github.com/BibliothecaDAO/eternum/pull/911/files#diff-70bffbd81abf0cc0ac28f290fb8909c67c1ba9f19753a3c716cf569cbbc0b3d2R56-R58) ```diff -// materials[2].depthWrite = false; -// materials[3].depthWrite = false; -// materials[4].depthWrite = false; +// Removed unused commented-out code ``` Suggestion importance[1-10]: 6Why: Removing commented-out code helps to keep the codebase clean and reduces potential confusion, improving overall code readability. | 6 | |
Correct the comment to accurately describe the opacity setting___ **Theopacity comment for hexMaterial is misleading as it states "Start fully transparent" but the opacity is set to 0.4. Correct the comment to reflect the actual opacity setting.** [client/src/ui/components/worldmap/hexagon/HexLayers.tsx [149]](https://github.com/BibliothecaDAO/eternum/pull/911/files#diff-32d953460a4689eeda29120db48be52828dc4e94c6e6dab11277ba52ce4db0feR149-R149) ```diff -opacity: 0.4, // Start fully transparent +opacity: 0.4, // Start with partial transparency ``` Suggestion importance[1-10]: 6Why: Correcting the comment improves code readability and maintainability by ensuring that comments accurately describe the code, preventing potential confusion for future developers. | 6 | |
Enhancement |
Refactor setting material properties to reduce redundancy and improve maintainability___ **Consider consolidating thedepthWrite property setting for materials into a single configuration function or object to improve code maintainability and reduce redundancy across different biomes.** [client/src/ui/components/models/biomes/ScorchedBiome.tsx [31-32]](https://github.com/BibliothecaDAO/eternum/pull/911/files#diff-f0a34877e7e779540a31c35d0b192c038e496afc52c904692b68acdce9a0aeabR31-R32) ```diff -materials["Scorched Rock"].depthWrite = false; -materials.Lava.depthWrite = false; +setMaterialProperties(["Scorched Rock", "Lava"], { depthWrite: false }); ``` Suggestion importance[1-10]: 8Why: Consolidating the `depthWrite` property setting into a single function or object reduces redundancy and enhances maintainability, making the code easier to manage and update. | 8 |
Possible issue |
Adjust the opacity to ensure the material is visible if it's not intended to be fully transparent___ **The opacity value forhexTransparentMaterial is set to 0, which makes it fully transparent and thus invisible. If this is not intentional, consider setting a higher opacity value.** [client/src/ui/components/worldmap/hexagon/HexLayers.tsx [141]](https://github.com/BibliothecaDAO/eternum/pull/911/files#diff-32d953460a4689eeda29120db48be52828dc4e94c6e6dab11277ba52ce4db0feR141-R141) ```diff -opacity: 0, // Start fully transparent +opacity: 0.5, // Adjusted to make the material slightly visible ``` Suggestion importance[1-10]: 7Why: This suggestion addresses a potential issue where the material might be invisible due to full transparency. Adjusting the opacity to a higher value ensures that the material is visible if that is the intended behavior. | 7 |
User description
Fix for #861 Refactor of materials depth sorting
PR Type
Bug fix, Enhancement
Description
renderOrder
property toprimitive
objects in multiple components to control rendering order.depthTest
property fromhexMaterial
in HighlightedHexes component.Changes walkthrough ๐
21 files
WarriorModel.tsx
Set render order for WarriorModel component.
client/src/ui/components/models/armies/WarriorModel.tsx - Added `renderOrder` property to `primitive` object.
BeachBiome.tsx
Adjust depth write and render order for BeachBiome.
client/src/ui/components/models/biomes/BeachBiome.tsx
renderOrder
property toprimitive
object.DeciduousForestBiome.tsx
Adjust depth write and render order for DeciduousForestBiome.
client/src/ui/components/models/biomes/DeciduousForestBiome.tsx
renderOrder
property toprimitive
object.DeepOceanBiome.tsx
Adjust depth write and render order for DeepOceanBiome.
client/src/ui/components/models/biomes/DeepOceanBiome.tsx
renderOrder
property toprimitive
object.DesertBiome.tsx
Adjust depth write and render order for DesertBiome.
client/src/ui/components/models/biomes/DesertBiome.tsx
renderOrder
property toprimitive
object.GrasslandBiome.tsx
Adjust depth write and render order for GrasslandBiome.
client/src/ui/components/models/biomes/GrasslandBiome.tsx
renderOrder
property toprimitive
object.OceanBiome.tsx
Adjust depth write and render order for OceanBiome.
client/src/ui/components/models/biomes/OceanBiome.tsx
renderOrder
property toprimitive
object.ScorchedBiome.tsx
Adjust depth write and render order for ScorchedBiome.
client/src/ui/components/models/biomes/ScorchedBiome.tsx
renderOrder
property toprimitive
objects.ShrublandBiome.tsx
Adjust depth write and render order for ShrublandBiome.
client/src/ui/components/models/biomes/ShrublandBiome.tsx
renderOrder
property toprimitive
objects.SnowBiome.tsx
Adjust depth write and render order for SnowBiome.
client/src/ui/components/models/biomes/SnowBiome.tsx
renderOrder
property toprimitive
objects.SubtropicalDesertBiome.tsx
Adjust depth write and render order for SubtropicalDesertBiome.
client/src/ui/components/models/biomes/SubtropicalDesertBiome.tsx
renderOrder
property toprimitive
objects.TaigaBiome.tsx
Adjust depth write and render order for TaigaBiome.
client/src/ui/components/models/biomes/TaigaBiome.tsx
renderOrder
property toprimitive
object.TemperateDesertBiome.tsx
Adjust depth write and render order for TemperateDesertBiome.
client/src/ui/components/models/biomes/TemperateDesertBiome.tsx
renderOrder
property toprimitive
objects.TemperateRainforestBiome.tsx
Adjust depth write and render order for TemperateRainforestBiome.
client/src/ui/components/models/biomes/TemperateRainforestBiome.tsx
renderOrder
property toprimitive
objects.TropicalRainforestBiome.tsx
Adjust depth write and render order for TropicalRainforestBiome.
client/src/ui/components/models/biomes/TropicalRainforestBiome.tsx
renderOrder
property toprimitive
objects.TropicalSeasonalForestBiome.tsx
Adjust depth write and render order for TropicalSeasonalForestBiome.
client/src/ui/components/models/biomes/TropicalSeasonalForestBiome.tsx
renderOrder
property toprimitive
objects.TundraBiome.tsx
Adjust depth write and render order for TundraBiome.
client/src/ui/components/models/biomes/TundraBiome.tsx
renderOrder
property toprimitive
objects.Structures.tsx
Set render order for BuiltStructure component.
client/src/ui/components/models/buildings/worldmap/Structures.tsx - Added `renderOrder` property to `primitive` object.
HexLayers.tsx
Enhance hexagon grid rendering and border handling.
client/src/ui/components/worldmap/hexagon/HexLayers.tsx
renderOrder
property toprimitive
objects.HighlightedHexes.tsx
Remove depth test from HighlightedHexes material.
client/src/ui/components/worldmap/hexagon/HighlightedHexes.tsx - Removed `depthTest` property from `hexMaterial`.
Flags.jsx
Set render order for Flags component.
client/src/ui/components/worldmap/Flags.jsx - Added `renderOrder` property to `primitive` objects.