[client] 3d models code and assets cleanup #2633
Conversation
|
Hi @r0man1337! You need to be added as a user to interact with me. Please ask @ponderingdemocritus to add you on the settings page. You are receiving this comment because I am set to review all PRs. That is configurable here. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThe pull request introduces a comprehensive reorganization of building and biome model management across multiple files. The changes focus on creating a more structured approach to handling building models by introducing new enums and nested map structures. The modifications enhance the categorization of building types, improve model loading mechanisms, and provide more explicit organization of resources and building groups. Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (16)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Failed to generate code suggestions for PR |
r0man1337
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
client/apps/game/src/three/components/BuildingPreview.ts (1)
68-70: Remove debugconsole.logstatements to clean up the code.The
console.logstatements at lines 68 and 70 are likely used for debugging purposes. Consider removing them to improve code cleanliness in production.client/apps/game/src/three/scenes/Hexception.tsx (1)
481-483: Avoid using magic numbers; use constants derived fromResourcesIds.The condition
building.resource < 24uses a magic number (24). It's better to replace it with a constant derived from theResourcesIdsenum to enhance maintainability and readability.Apply this suggestion:
- if (building.resource && (building.resource < 24 || building.resource === ResourcesIds.AncientFragment)) { + // Define MAX_RESOURCE_ID based on ResourcesIds enum + const MAX_RESOURCE_ID = /* maximum value from ResourcesIds */; + if (building.resource && (building.resource <= MAX_RESOURCE_ID || building.resource === ResourcesIds.AncientFragment)) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
client/apps/game/src/three/components/BuildingPreview.ts(5 hunks)client/apps/game/src/three/scenes/Hexception.tsx(4 hunks)client/apps/game/src/three/scenes/constants.ts(2 hunks)
🔇 Additional comments (1)
client/apps/game/src/three/scenes/constants.ts (1)
Line range hint
1-193: The refactoring of constants and enums enhances code organization.The introduction of
BUILDINGS_GROUPS,BUILDINGS_CATEGORIES_TYPES, and the restructuring of model paths improve the clarity and maintainability of the code.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
client/apps/game/src/three/scenes/hexception.tsx (1)
Line range hint
508-509: Check for potential memory leaks with mixersStored animation mixers in
buildingMixersmay lead to memory leaks if not properly disposed when buildings are removed.Ensure that mixers are disposed and removed from
buildingMixerswhen buildings are removed from the scene.if (this.buildingMixers.has(key)) { this.buildingMixers.get(key).stopAllAction(); this.buildingMixers.delete(key); }
🧹 Nitpick comments (9)
client/apps/game/src/three/managers/building-preview.ts (3)
39-45: Avoid usinganytype ingltf.scene.traverseUsing
anyfor thechildparameter loses type safety. Consider specifying a more precise type, such asTHREE.Object3Dor using a type guard to check forTHREE.Mesh.Apply this diff to improve type safety:
-gltf.scene.traverse((child: any) => { +gltf.scene.traverse((child) => { + if ((child as THREE.Mesh).isMesh) { // TypeScript will now recognize child as THREE.MeshAlternatively, use a type guard:
gltf.scene.traverse((child) => { if (child instanceof THREE.Mesh) { // child is now recognized as THREE.Mesh } });
46-46: Ensure consistent key types when usingMapIn
groupMap.set(building as any, model);, castingbuildingtoanycan lead to type inconsistencies. SincegroupMapis aMap<string, THREE.Group>, explicitly convertbuildingto a string.Apply this diff to correct the key type:
-groupMap.set(building as any, model); +groupMap.set(building.toString(), model);
69-71: Remove unnecessaryconsole.logstatementsDebugging statements like
console.logcan clutter the console output in production. Consider removing or properly managing them using a logging framework that can be disabled in production environments.Apply this diff to remove the logs:
- console.log("categoryMap", categoryMap); ... - console.log("building", categoryMap.get(building + ""));client/apps/game/src/three/scenes/hexception.tsx (5)
100-101: Improve readability by adding type aliasesThe
buildingModelsmap uses a complex nested type which can be hard to read. Consider introducing type aliases to enhance readability.Define type aliases at the top of the file:
type BuildingModelData = { model: THREE.Group; animations: THREE.AnimationClip[] }; type BuildingGroupMap = Map<BUILDINGS_CATEGORIES_TYPES, BuildingModelData>;Then update the
buildingModelsdeclaration:-private buildingModels: Map< - BUILDINGS_GROUPS, - Map<BUILDINGS_CATEGORIES_TYPES, { model: THREE.Group; animations: THREE.AnimationClip[] }> -> = new Map(); +private buildingModels: Map<BUILDINGS_GROUPS, BuildingGroupMap> = new Map();
192-229: Duplicate code inloadBuildingModelsacross classesThe
loadBuildingModelsmethod appears similar to the one inBuildingPreview. Consider abstracting this logic into a shared utility function or base class to promote code reuse and reduce duplication.Extract the common loading logic into a separate utility function or a shared class that both
HexceptionSceneandBuildingPreviewcan utilize.
208-215: Avoid usinganyinmodel.traverseUsing
anytype for thechildparameter reduces type safety. UseTHREE.Object3Dor appropriate type guards to maintain type safety.Apply this diff to specify the type:
-model.traverse((child) => { +model.traverse((child: THREE.Object3D) => { if (child instanceof THREE.Mesh) {
477-486: Simplify building group and type determinationThe logic for determining
buildingGroupandbuildingTypecan be simplified for clarity and maintainability.Consider refactoring the code to reduce nesting and improve readability:
let buildingGroup: BUILDINGS_GROUPS; let buildingType: BUILDINGS_CATEGORIES_TYPES; if (building.resource && (building.resource < 24 || building.resource === ResourcesIds.AncientFragment)) { buildingGroup = BUILDINGS_GROUPS.RESOURCES_MINING; buildingType = ResourceIdToMiningType[building.resource as ResourcesIds] as ResourceMiningTypes; } else if (building.structureType === StructureType.Hyperstructure) { buildingGroup = BUILDINGS_GROUPS.HYPERSTRUCTURE; buildingType = hyperstructureStageToModel[this.structureStage as StructureProgress]; } else if (this.tileManager.getWonder(this.state.structureEntityId)) { buildingGroup = BUILDINGS_GROUPS.WONDER; buildingType = WONDER_REALM; } else { buildingGroup = BUILDINGS_GROUPS.BUILDINGS; buildingType = BuildingType[building.category].toString() as BUILDINGS_CATEGORIES_TYPES; } if (buildingGroup === BUILDINGS_GROUPS.BUILDINGS && buildingType === BuildingType.Castle.toString()) { buildingType = castleLevelToRealmCastle[this.structureStage]; buildingGroup = BUILDINGS_GROUPS.REALMS; }
215-216: Redundant emptyPromise.allcallbackThe
Promise.all(this.modelLoadPromises).then(() => {});includes an emptythencallback which is unnecessary.Remove the empty callback to clean up the code:
-Promise.all(this.modelLoadPromises).then(() => {}); +Promise.all(this.modelLoadPromises);client/apps/game/src/three/scenes/constants.ts (1)
99-105: Consider consolidatingBUILDINGS_GROUPSenum valuesThe
BUILDINGS_GROUPSenum has string values that may benefit from being constant variables or a string literal union type instead.If enum semantics are not required, consider using a type alias:
export type BUILDINGS_GROUPS = 'buildings' | 'resources_mining' | 'hyperstructure' | 'realms' | 'wonder';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
client/apps/game/src/three/managers/building-preview.ts(5 hunks)client/apps/game/src/three/scenes/constants.ts(2 hunks)client/apps/game/src/three/scenes/hexception.tsx(4 hunks)
🧰 Additional context used
🪛 GitHub Actions: knip
client/apps/game/src/three/scenes/constants.ts
[warning] 19-19: Unused exported enum 'BiomeFilenames'
[warning] 39-39: Unused exported enum 'BuildingFilenames'
🔇 Additional comments (5)
client/apps/game/src/three/managers/building-preview.ts (1)
14-14: Good use of a nestedMapstructure forbuildingModelsThe change to use a nested
Map<BUILDINGS_GROUPS, Map<string, THREE.Group>>forbuildingModelsimproves the organization and categorization of building models by groups, enhancing code maintainability.client/apps/game/src/three/scenes/constants.ts (4)
19-37: Unused exported enumBiomeFilenamesThe
BiomeFilenamesenum is exported but not used anywhere in the codebase.Given the pipeline warning:
[warning] 19-19: Unused exported enum 'BiomeFilenames'
Consider removing
BiomeFilenamesif it's not needed, or ensure it's used where appropriate.🧰 Tools
🪛 GitHub Actions: knip
[warning] 19-19: Unused exported enum 'BiomeFilenames'
39-68: Unused exported enumBuildingFilenamesSimilarly, the
BuildingFilenamesenum is exported but not used.As per the pipeline warning:
[warning] 39-39: Unused exported enum 'BuildingFilenames'
Review whether
BuildingFilenamesis necessary. If not, remove it to clean up the codebase.🧰 Tools
🪛 GitHub Actions: knip
[warning] 39-39: Unused exported enum 'BuildingFilenames'
114-153: Possible duplication inbuildingModelPathskeysEnsure that all keys in
buildingModelPathsare unique and correctly correspond to the enums and types used elsewhere.Double-check that
BuildingType.ResourceandResourceMiningTypesdo not overlap unintentionally, which could cause model loading issues.
156-172: Adjust import paths for consistencyIn
biomeModelPaths, ensure that all paths are correctly constructed and consistent with the actual file structure.Confirm that
BIOMES_MODELS_PATHand the filenames fromBiomeFilenamescorrectly resolve to the existing model files.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
client/apps/game/src/three/scenes/hexception.tsx (1)
Line range hint
393-404: Prevent potential memory leaks by unsubscribing from subscriptionsIn the
setupmethod, subscriptions tobuildingSubscriptionandrealmSubscriptionare made. Ensure that these subscriptions are properly unsubscribed when the component is destroyed to prevent memory leaks.
🧹 Nitpick comments (5)
client/apps/game/src/three/managers/building-preview.ts (1)
67-70: Remove unnecessaryconsole.logstatementsThe
console.logstatements at lines 67 and 69 appear to be for debugging purposes. It's best practice to remove such statements or replace them with a proper logging mechanism to avoid cluttering the console in production.Apply this diff to remove the
console.logstatements:const categoryMap = this.buildingModels.get(group); - console.log("categoryMap", categoryMap); if (categoryMap) { - console.log("building", categoryMap.get(building + "")); return categoryMap.get(building + "") || null; }client/apps/game/src/three/scenes/constants.ts (2)
1-1: Organize import statements and remove duplicate importsThe
THREElibrary is imported at line 1, which is appropriate. Ensure that all imports are necessary and there are no duplicate imports elsewhere in the file.
27-45: Address unused exported enumsBiomeFilenamesandBuildingFilenamesThe enums
BiomeFilenamesandBuildingFilenamesare exported but not used outside this file, as indicated by the pipeline warnings. If these enums are intended for internal use only, consider removing theexportkeyword to prevent unused export warnings.Apply this diff to make the enums internal:
-export enum BiomeFilenames { +enum BiomeFilenames { Bare = "bare.glb", Beach = "beach.glb", // ... remaining enum entries } - -export enum BuildingFilenames { +enum BuildingFilenames { Bank = "bank.glb", ArcheryRange = "archerrange.glb", // ... remaining enum entries }🧰 Tools
🪛 GitHub Actions: knip
[warning] 27-27: Unused exported enum: BiomeFilenames
client/apps/game/src/three/scenes/hexception.tsx (2)
193-230: Handle model loading errors gracefullyIn the
loadBuildingModelsmethod, consider adding error handling or retries in case a model fails to load. This can improve the robustness of the application.
478-498: Simplify building type determination logicThe logic for determining
buildingGroupandbuildingTypecan be refactored for clarity and maintainability. Consider extracting this logic into a separate method or utilizing a mapping function.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
client/apps/game/src/three/managers/building-preview.ts(5 hunks)client/apps/game/src/three/scenes/constants.ts(3 hunks)client/apps/game/src/three/scenes/hexception.tsx(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
client/apps/game/src/three/scenes/constants.ts
[error] 93-93: Shouldn't redeclare 'StructureProgress'. Consider to delete it or rename it.
'StructureProgress' is defined here:
(lint/suspicious/noRedeclare)
🪛 GitHub Actions: knip
client/apps/game/src/three/scenes/constants.ts
[warning] 27-27: Unused exported enum: BiomeFilenames
[warning] 47-47: Unused exported enum: BuildingFilenames
[warning] 93-93: Unused exported enum: StructureProgress
🔇 Additional comments (4)
client/apps/game/src/three/scenes/constants.ts (2)
164-180: Ensure consistency inbiomeModelPathskeysThe
biomeModelPathsobject uses keys of typeBiomeType | "Outline". Verify that all keys correspond correctly to theBiomeFilenamesentries to prevent any mismatches in model loading.
161-161: Verify the use ofBuildingFilenamesandBiomeFilenamesenumsDespite being declared, the enums
BiomeFilenamesandBuildingFilenamesmight not be utilized effectively. Ensure they are used consistently throughout the file to enhance code maintainability and readability.Please run the following script to check where these enums are being used:
✅ Verification successful
BiomeFilenames and BuildingFilenames enums are properly utilized
Both enums are effectively used throughout the constants file as a centralized source of truth for file naming conventions, providing type safety and consistent path construction for biome and building assets.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of 'BiomeFilenames' and 'BuildingFilenames' in the codebase. # Search for 'BiomeFilenames' usage rg 'BiomeFilenames' # Search for 'BuildingFilenames' usage rg 'BuildingFilenames'Length of output: 6964
client/apps/game/src/three/scenes/hexception.tsx (2)
101-102: Type aliasBUILDINGS_CATEGORIES_TYPESmight not cover all casesThe
buildingModelsmap usesBUILDINGS_CATEGORIES_TYPESas the inner key type. Ensure that this type includes all possible keys used when loading models, especially custom strings likeWONDER_REALM, to avoid runtime errors.
500-503: Handle missing hyperstructure modelsEnsure that
hyperstructureStageToModelcovers all possible stages of the hyperstructure. Also, verify that the corresponding models are loaded and available inbuildingModelsto prevent null references.
| export enum StructureProgress { | ||
| STAGE_1 = 0, | ||
| STAGE_2 = 1, | ||
| STAGE_3 = 2, | ||
| } |
There was a problem hiding this comment.
Fix redeclaration of StructureProgress enum
The StructureProgress enum is being redeclared at lines 93-97, but it is already imported from @bibliothecadao/react at line 1. Redeclaring it can cause conflicts and unexpected behavior. Please remove this redeclaration and use the imported StructureProgress instead.
Apply this diff to remove the redeclaration:
-import { StructureProgress } from "@bibliothecadao/react";
...
-export enum StructureProgress {
- STAGE_1 = 0,
- STAGE_2 = 1,
- STAGE_3 = 2,
-}
+// Use the imported StructureProgress from '@bibliothecadao/react'Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 93-93: Shouldn't redeclare 'StructureProgress'. Consider to delete it or rename it.
'StructureProgress' is defined here:
(lint/suspicious/noRedeclare)
🪛 GitHub Actions: knip
[warning] 93-93: Unused exported enum: StructureProgress
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/apps/landing/src/dojo/setup.ts (1)
13-14: Consider documenting the singleton pattern usage.The use of
ClientConfigManager.instance()indicates a singleton pattern. Consider adding a brief comment explaining why this pattern is used here and its lifecycle management.+ // Use singleton instance to ensure consistent configuration state across the application const configManager = ClientConfigManager.instance();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.knip.json(1 hunks)client/apps/game/src/three/scenes/constants.ts(3 hunks)client/apps/landing/src/dojo/setup.ts(1 hunks)client/apps/landing/src/hooks/helpers/useRealms.tsx(0 hunks)client/sdk/packages/core/src/provider/tick.ts(0 hunks)
💤 Files with no reviewable changes (2)
- client/apps/landing/src/hooks/helpers/useRealms.tsx
- client/sdk/packages/core/src/provider/tick.ts
🔇 Additional comments (5)
client/apps/landing/src/dojo/setup.ts (1)
13-14: Verify the impact of removing the exported configManager.The change from a module-level export to a function-local variable improves encapsulation. However, we should verify that no other modules were depending on the removed export.
Let's check for any direct imports of configManager from this file:
✅ Verification successful
The configManager scope change is safe to proceed with ✅
The codebase consistently uses
ClientConfigManager.instance()to access the singleton instance, and there are no direct imports of the exportedconfigManagerfrom setup.ts. This change improves encapsulation without affecting any dependencies.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any imports of configManager from setup.ts rg -l "import.*\{.*configManager.*\}.*from.*setup" # Search for any direct references to configManager from setup rg "configManager.*setup"Length of output: 80
Script:
#!/bin/bash # Search for any imports from setup.ts with context rg -A 2 "from.*setup" # Search for ClientConfigManager usage rg "ClientConfigManager" # Search for any direct file references to setup.ts rg "setup\.ts"Length of output: 4686
.knip.json (1)
9-9: Verify the relocation of global type definitions.The change suggests that global type definitions have been moved from the
eternumpackage to thecorepackage. This aligns with the cleanup objectives.✅ Verification successful
Global type definitions relocation verified
The file has been successfully moved to
client/sdk/packages/core/global.d.tsand the old location is properly cleaned up. The configuration change is correct.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the relocation of global type definitions # Check if the old file is completely removed and the new file exists echo "Checking for the existence of files:" fd "global.d.ts$" client/sdk/packages/Length of output: 160
client/apps/game/src/three/scenes/constants.ts (3)
Line range hint
1-26: Well-organized path constants!The consolidation of path constants and the clean separation between flat and non-flat modes improves maintainability.
116-195: Excellent organization of building models!The new structure using
BUILDINGS_GROUPSprovides:
- Clear categorization
- Type-safe mappings
- Consistent path construction
47-76: Consolidate duplicate model files.Several building types are using the same model file (
market.glb):
- TradingPost
- Walls
- WatchTower
Consider creating distinct models for these buildings to improve visual differentiation.
✅ Verification successful
Model reuse appears intentional but visual distinction would be beneficial
The codebase currently reuses
market.glbfor TradingPost, Walls, and WatchTower buildings, which appears to be an intentional design choice. While creating distinct models would improve visual clarity, the current implementation is functional.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for duplicate model file usage echo "Checking for model files in the repository:" fd "market.glb|walls.glb|watchtower.glb" "models/buildings/"Length of output: 262
Script:
#!/bin/bash # First, find all .glb files to understand the model directory structure echo "Searching for all .glb files in the repository:" fd -e glb # Then specifically look for the relevant model files echo -e "\nSearching for specific model files:" fd "market.glb|castle|wonder|hyperstructure"Length of output: 6061
Summary by CodeRabbit
Release Notes
Refactor
New Features
Chores
Bug Fixes
These updates significantly enhance the clarity and organization of the building management system within the game, improving overall functionality.