feat: Implement instanced rendering mode for low-end devices #375
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
WalkthroughThis PR introduces selective instanced rendering for full cube blocks, integrating new instancing modes and dynamic control over instanced rendering. It adds new Three.js-based instanced rendering infrastructure, updates mesher and world renderer logic to support per-section instancing, and extends configuration, UI, and debugging tools for instancing features. The chunk mesh management is refactored for pooling and performance-aware visibility. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as User/Options UI
participant WR as WorldRendererThree
participant IM as InstancedRenderer
participant MM as ChunkMeshManager
participant Mesher as Mesher Worker
UI->>WR: Toggle instanced rendering option
WR->>WR: Update config, recalculate instancing mode per section
WR->>Mesher: setSectionDirty(pos, ..., instancingMode)
Mesher->>Mesher: Generate geometry, collect instanced blocks if enabled
Mesher-->>WR: geometry + instancedBlocks
WR->>IM: handleInstancedBlocksFromWorker(instancedBlocks, sectionKey, mode)
WR->>MM: updateSection(sectionKey, geometry)
IM->>IM: Update instanced meshes, add/remove instances as needed
MM->>MM: Update/reuse mesh pool entries, set visibility
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
/deploy |
|
?setting=forceInstancedOnly:true&setting=useInstancedRendering:true&setting=enableSingleColorMode:true |
|
Deployed to Vercel Preview: https://prismarine-gmg3sn3ow-zaro.vercel.app |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
renderer/viewer/three/instancedRenderer.ts (1)
6-137: Skip: Duplicate block list already addressed.This issue was already covered in the review of
models.ts.
🧹 Nitpick comments (5)
src/optionsStorage.ts (1)
55-59: LGTM! Well-structured instanced rendering configuration.The new instanced rendering options are well-designed with conservative defaults. The naming is clear and the values are appropriate for initial rollout.
Consider adding JSDoc comments for the new options to document their purpose:
+ /** Enable instanced rendering for eligible blocks */ useInstancedRendering: false, + /** Force instanced rendering only, disabling regular mesh generation */ forceInstancedOnly: false, + /** Distance in chunks beyond which only instanced rendering is used */ instancedOnlyDistance: 6, + /** Ultra-performance mode using solid colors instead of textures */ enableSingleColorMode: false,renderer/viewer/three/worldrendererThree.ts (1)
27-27: Remove unused import.
INSTANCEABLE_BLOCKSis imported but never used in this file.-import { InstancedRenderer, INSTANCEABLE_BLOCKS } from './instancedRenderer' +import { InstancedRenderer } from './instancedRenderer'renderer/viewer/three/instancedRenderer.ts (3)
196-212: Consider lazy initialization of instanced meshes.Pre-creating instanced meshes for all 137 block types consumes memory even if many blocks are never used in a world.
Consider creating meshes on-demand in
handleInstancedBlocksFromWorkerwhen a block type is first encountered, which already has this logic implemented (lines 316-329).Remove the
initInstancedMeshescall from the constructor and the method itself, relying on the dynamic creation already implemented.
223-232: Fix ESLint warning: unexpected if as only statement in else block.The nested if statement can be simplified by combining conditions or restructuring the logic.
} else { // Use texture from the blocks atlas - if (this.worldRenderer.material.map) { - const material = this.worldRenderer.material.clone() - // The texture is already the correct blocks atlas texture - // Individual block textures are handled by UV coordinates in the geometry - return material - } else { - // Fallback to colored material - const color = this.getBlockColor(blockName) - return new THREE.MeshLambertMaterial({ color }) - } + // Use texture from the blocks atlas + const material = this.worldRenderer.material.map + ? this.worldRenderer.material.clone() + : new THREE.MeshLambertMaterial({ color: this.getBlockColor(blockName) }) + return material }
385-428: Add comments to explain the compaction algorithm.The
compactInstancesForBlockmethod implements a complex algorithm for removing gaps in instance arrays. Additional comments would improve maintainability.Add explanatory comments:
private compactInstancesForBlock (blockName: string, indicesToRemove: number[]) { const mesh = this.instancedMeshes.get(blockName) if (!mesh) return const currentCount = this.blockCounts.get(blockName) || 0 const removeSet = new Set(indicesToRemove) let writeIndex = 0 const tempMatrix = new THREE.Matrix4() - // Compact the instance matrix by removing gaps + // Compact the instance matrix by removing gaps + // This works by copying non-removed instances to fill gaps left by removed ones + // Example: if removing indices [1,3] from [0,1,2,3,4], result is [0,2,4] at positions [0,1,2] for (let readIndex = 0; readIndex < currentCount; readIndex++) { if (!removeSet.has(readIndex)) { if (writeIndex !== readIndex) { + // Copy instance from readIndex to writeIndex to fill the gap mesh.getMatrixAt(readIndex, tempMatrix) mesh.setMatrixAt(writeIndex, tempMatrix) } writeIndex++ } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
renderer/viewer/lib/mesher/models.ts(4 hunks)renderer/viewer/lib/mesher/shared.ts(3 hunks)renderer/viewer/lib/worldrendererCommon.ts(2 hunks)renderer/viewer/three/instancedRenderer.ts(1 hunks)renderer/viewer/three/worldrendererThree.ts(8 hunks)src/optionsStorage.ts(1 hunks)src/react/RendererDebugMenu.tsx(2 hunks)src/watchOptions.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`src/**/*.ts`: You may use the global variable `bot` directly in any file under ...
src/**/*.ts: You may use the global variablebotdirectly in any file under thesrc/directory.
Insrc/code, you may use the global variableappViewerfromsrc/appViewer.tsdirectly. Do not importappVieweror usewindow.appViewer; use the globalappViewervariable as-is.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/vars-usage.mdc)
List of files the instruction was applied to:
src/optionsStorage.tssrc/watchOptions.ts
`renderer/**/*.ts`: Do not use the global variable `bot` directly in any file un...
renderer/**/*.ts: Do not use the global variablebotdirectly in any file under therenderer/directory or its subfolders.
In renderer code, all bot/player state and events must be accessed via explicit interfaces, state managers, or passed-in objects, never by referencingbotdirectly.
In renderer code (such as inWorldRendererThree), use theplayerStateproperty (e.g.,worldRenderer.playerState.gameMode) to access player state. The implementation forplayerStatelives insrc/mineflayer/playerState.ts.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/vars-usage.mdc)
List of files the instruction was applied to:
renderer/viewer/lib/worldrendererCommon.tsrenderer/viewer/lib/mesher/shared.tsrenderer/viewer/lib/mesher/models.tsrenderer/viewer/three/worldrendererThree.tsrenderer/viewer/three/instancedRenderer.ts
🪛 ESLint
renderer/viewer/three/instancedRenderer.ts
[error] 223-232: Unexpected if as the only statement in an else block.
(no-lonely-if)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (17)
src/watchOptions.ts (1)
119-125: LGTM! Proper integration with the options watching system.The instanced rendering options are correctly wired to update the in-world rendering configuration when changed. The implementation follows the established pattern used by other option watchers.
src/react/RendererDebugMenu.tsx (2)
19-19: LGTM! Proper snapshot hook extension.The instanced rendering options are correctly added to the useSnapshot hook to enable reactive updates.
119-148: LGTM! Well-implemented debug controls.The instanced rendering controls are properly structured with:
- Clear labels and appropriate overlay colors for visual feedback
- Correct bindings to the global options
- Reasonable slider range (1-16) for the distance parameter
- Consistent styling with existing debug menu sections
renderer/viewer/lib/worldrendererCommon.ts (2)
58-63: LGTM! Proper configuration defaults.The instanced rendering options are correctly integrated into the world renderer configuration with appropriate defaults and clear documentation.
579-582: LGTM! Correct mesher config integration.The instanced rendering options are properly passed through to the mesher configuration, maintaining the data flow from world renderer config to mesher worker configuration.
renderer/viewer/lib/mesher/shared.ts (3)
15-19: LGTM! Clean mesher configuration integration.The instanced rendering flags are properly integrated into the mesher configuration with clear documentation.
28-38: LGTM! Well-designed type definition.The
InstancedBlockEntrytype is well-structured with:
- Essential block identification (name, stateId)
- Optional texture coordinate information for rendering
- Array of positions for instanced placement
The design supports both textured and single-color rendering modes.
65-66: LGTM! Proper geometry output extension.The
instancedBlocksproperty correctly extends the mesher output to include instanced block data, maintaining the existing structure while adding new functionality.renderer/viewer/lib/mesher/models.ts (3)
654-684: LGTM! Well-implemented culling logic.The function correctly determines if an instanced block should be culled by checking all 6 faces. The early return optimization and special handling for glass/ice blocks are appropriate.
711-726: LGTM! Thorough validation for instanceable blocks.The function properly validates that blocks are full cubes without rotations before considering them instanceable.
686-709: Add defensive checks for texture properties.The function accesses texture properties without validation, which could cause runtime errors if the texture object structure is unexpected.
Add null checks before accessing texture properties:
if (face?.texture) { const texture = face.texture as any return { - u: texture.u || 0, - v: texture.v || 0, - su: texture.su || 1, - sv: texture.sv || 1 + u: texture?.u ?? 0, + v: texture?.v ?? 0, + su: texture?.su ?? 1, + sv: texture?.sv ?? 1 } }Likely an incorrect or invalid review comment.
renderer/viewer/three/worldrendererThree.ts (4)
105-105: LGTM! Proper initialization of instanced renderer.The instanced renderer is correctly initialized after other essential components.
233-234: LGTM! Correct material update timing.The instanced renderer materials are properly updated after texture assets are loaded.
303-311: LGTM! Comprehensive debug statistics.The debug overlay properly displays instanced rendering metrics including total instances, active block types, and draw calls.
901-904: LGTM! Proper resource cleanup.The instanced renderer instances are correctly removed when columns are removed, and the renderer is properly destroyed during cleanup.
Also applies to: 934-934
renderer/viewer/three/instancedRenderer.ts (2)
430-443: Remove unused method or implement its usage.The
shouldUseInstancedRenderingmethod is defined but never called anywhere in the codebase.#!/bin/bash # Description: Check if shouldUseInstancedRendering is called anywhere rg -A 2 "shouldUseInstancedRendering"If this method is intended for future use or for external callers, consider adding a comment explaining its purpose. Otherwise, remove it to reduce code complexity.
139-143: ```shell
#!/bin/bashLocate the definition of INSTANCEABLE_BLOCKS
rg "INSTANCEABLE_BLOCKS" -A20 --type ts
Check if 'ice' and 'spawner' appear in the instanceable blocks list
rg "blockName.['"]ice['"]" -n --type ts
rg "blockName.['"]spawner['"]" -n --type ts</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
renderer/viewer/lib/mesher/models.ts (2)
11-142: Consider centralizing the instanceable blocks list.This hardcoded list defines which blocks can use instanced rendering. While comprehensive, this creates a maintenance burden if duplicated elsewhere.
848-881: Implement missing block count increment for culled blocks.When an instanced block is culled at line 853, the
continuestatement skips it without incrementingblocksCount, leading to incorrect statistics.Apply this fix:
if (shouldCullInstancedBlock(world, cursor, block)) { // Block is completely surrounded, skip rendering + attr.blocksCount++ continue }renderer/viewer/three/instancedRenderer.ts (1)
189-211: Complete the UV mapping implementation.The current implementation only applies simple UV transformations without considering face-specific texture mappings.
🧹 Nitpick comments (1)
renderer/viewer/three/instancedRenderer.ts (1)
238-251: Refactor to eliminate the lonely if statement.The current structure requires an eslint-disable comment. Consider refactoring to improve code clarity:
} else { // Use texture from the blocks atlas - // eslint-disable-next-line no-lonely-if - if (this.worldRenderer.material.map) { + const hasTextureMap = this.worldRenderer.material.map + if (hasTextureMap) { const material = this.worldRenderer.material.clone() // The texture is already the correct blocks atlas texture // Individual block textures are handled by UV coordinates in the geometry return material - } else { - // Fallback to colored material - const color = this.getBlockColor(blockName) - return new THREE.MeshLambertMaterial({ color }) } + // Fallback to colored material + const color = this.getBlockColor(blockName) + return new THREE.MeshLambertMaterial({ color }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
renderer/viewer/lib/mesher/mesher.ts(7 hunks)renderer/viewer/lib/mesher/models.ts(4 hunks)renderer/viewer/lib/mesher/shared.ts(4 hunks)renderer/viewer/lib/mesher/world.ts(2 hunks)renderer/viewer/lib/worldrendererCommon.ts(5 hunks)renderer/viewer/three/instancedRenderer.ts(1 hunks)renderer/viewer/three/worldrendererThree.ts(10 hunks)src/defaultOptions.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- renderer/viewer/lib/worldrendererCommon.ts
- renderer/viewer/three/worldrendererThree.ts
🧰 Additional context used
📓 Path-based instructions (2)
`src/**/*.ts`: You may use the global variable `bot` directly in any file under ...
src/**/*.ts: You may use the global variablebotdirectly in any file under thesrc/directory.
Insrc/code, you may use the global variableappViewerfromsrc/appViewer.tsdirectly. Do not importappVieweror usewindow.appViewer; use the globalappViewervariable as-is.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/vars-usage.mdc)
List of files the instruction was applied to:
src/defaultOptions.ts
`renderer/**/*.ts`: Do not use the global variable `bot` directly in any file un...
renderer/**/*.ts: Do not use the global variablebotdirectly in any file under therenderer/directory or its subfolders.
In renderer code, all bot/player state and events must be accessed via explicit interfaces, state managers, or passed-in objects, never by referencingbotdirectly.
In renderer code (such as inWorldRendererThree), use theplayerStateproperty (e.g.,worldRenderer.playerState.gameMode) to access player state. The implementation forplayerStatelives insrc/mineflayer/playerState.ts.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/vars-usage.mdc)
List of files the instruction was applied to:
renderer/viewer/lib/mesher/world.tsrenderer/viewer/lib/mesher/models.tsrenderer/viewer/lib/mesher/shared.tsrenderer/viewer/lib/mesher/mesher.tsrenderer/viewer/three/instancedRenderer.ts
🧠 Learnings (4)
renderer/viewer/lib/mesher/world.ts (1)
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, files under renderer/ directory must not access the global `bot` variable directly according to .cursor/rules/vars-usage.mdc. The updateNameTagVisibility method in renderer/viewer/three/entities.ts currently violates this rule by accessing bot.teamMap and bot.username. Team information should be passed through entity update events from the world data emitter instead.
renderer/viewer/lib/mesher/models.ts (1)
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, files under renderer/ directory must not access the global `bot` variable directly according to .cursor/rules/vars-usage.mdc. The updateNameTagVisibility method in renderer/viewer/three/entities.ts currently violates this rule by accessing bot.teamMap and bot.username. Team information should be passed through entity update events from the world data emitter instead.
renderer/viewer/lib/mesher/mesher.ts (3)
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, files under renderer/ directory must not access the global `bot` variable directly according to .cursor/rules/vars-usage.mdc. The updateNameTagVisibility method in renderer/viewer/three/entities.ts currently violates this rule by accessing bot.teamMap and bot.username. Team information should be passed through entity update events from the world data emitter instead.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-06-23T17:08:19.477Z
Learning: Globals that are safe to use without window prefixes are documented in `src/globals.d.ts`.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-06-23T17:08:19.477Z
Learning: Global variables defined for use in `src/` (such as `appViewer` from `src/appViewer.ts`) should be accessed directly as globals, not imported or accessed via `window`, to ensure consistency and avoid ambiguity.
renderer/viewer/three/instancedRenderer.ts (1)
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, files under renderer/ directory must not access the global `bot` variable directly according to .cursor/rules/vars-usage.mdc. The updateNameTagVisibility method in renderer/viewer/three/entities.ts currently violates this rule by accessing bot.teamMap and bot.username. Team information should be passed through entity update events from the world data emitter instead.
🧬 Code Graph Analysis (1)
renderer/viewer/lib/mesher/mesher.ts (2)
renderer/viewer/lib/worldrendererCommon.ts (1)
setSectionDirty(926-965)renderer/viewer/lib/mesher/models.ts (1)
getSectionGeometry(741-992)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (27)
src/defaultOptions.ts (1)
45-49: LGTM! Well-structured configuration options.The instanced rendering configuration options are well-designed with safe defaults that require opt-in activation. The naming is clear and the default values are conservative.
renderer/viewer/lib/mesher/world.ts (4)
8-8: Good import addition for type safety.Adding the
MesherConfigimport enables proper typing of the constructor parameter.
45-46: Appropriate data structures for instanced rendering.The
instancedBlocksandinstancedBlockIdsproperties provide the necessary storage for tracking blocks eligible for instanced rendering.
48-48: Improved type safety in constructor.Explicitly typing the
versionparameter asstringenhances type checking.
52-52: Excellent immutability improvement.Replacing direct mutation of
defaultMesherConfig.versionwith object spread creates a new config object, preventing unintended side effects on the shared default configuration.renderer/viewer/lib/mesher/mesher.ts (7)
4-4: Necessary import for instancing mode support.The
InstancingModeimport is required for the new instancing mode tracking functionality.
20-20: Improved dirty section tracking structure.The enhanced structure now properly tracks instancing mode alongside the dirty count, enabling mode-aware geometry generation.
50-63: Correct instancing mode parameter handling.The
setSectionDirtyfunction properly accepts and stores the instancing mode with appropriate defaults.
101-102: Proper initialization of instanced block data.The mesher data handler correctly initializes the world's instanced block properties from the message data.
108-108: Correct instancing mode forwarding.The dirty message handler properly forwards the instancing mode from the message to the
setSectionDirtyfunction.
202-208: Proper instancing mode extraction and usage.The interval loop correctly extracts the instancing mode from dirty sections and passes it to
getSectionGeometryfor mode-aware geometry generation.
218-218: Correct use of times property.The loop properly uses the
timesproperty from the stored dirty section object.renderer/viewer/lib/mesher/shared.ts (4)
3-7: Well-defined instancing mode enum.The
InstancingModeenum provides clear, descriptive values for different instancing rendering strategies.
30-41: Comprehensive instanced block entry type.The
InstancedBlockEntrytype includes all necessary fields for identifying and rendering instanced blocks, including optional texture information.
43-47: Appropriate mesher data type.The
InstancingMesherDatatype provides a clean mapping structure for block state ID to instance ID relationships.
74-75: Clean geometry output extension.Adding the
instancedBlocksproperty toMesherGeometryOutputcleanly extends the interface to support instanced rendering data.renderer/viewer/lib/mesher/models.ts (8)
8-8: Necessary import for instanced rendering types.The import adds the required types and enums for the instanced rendering implementation.
149-150: Good caching strategy for texture info.The texture info cache prevents repeated calculations for the same block state IDs, improving performance.
657-685: Efficient culling logic for instanced blocks.The
shouldCullInstancedBlockfunction properly checks all six faces to determine if a block is completely surrounded by opaque neighbors, enabling performance optimization through culling.
687-721: Well-implemented texture info extraction.The
getInstancedBlockTextureInfofunction correctly extracts texture coordinates from block models with proper caching and fallback logic.
723-738: Robust block instanceability check.The
isBlockInstanceablefunction properly validates that blocks are eligible for instancing by checking they're in the allowed set and have simple full-cube geometry without rotations.
741-746: Clean instancing mode integration.The function signature update and instancing mode checks properly enable conditional instanced rendering behavior.
770-771: Proper geometry output initialization.Adding
blocksCountandinstancedBlocksto the output structure correctly supports the instanced rendering feature.
883-887: Correct force instanced only mode implementation.The logic properly skips non-instanceable blocks when force instanced only mode is enabled, maintaining the intended performance optimization behavior.
renderer/viewer/three/instancedRenderer.ts (3)
1-4: LGTM!Imports are clean and appropriate. No usage of the global
botvariable, which complies with the coding guidelines for renderer code.
141-150: LGTM!The RGB parsing logic is correct with proper error handling and fallback.
223-223: Clarify why frustum culling is disabled.Setting
frustumCulled = falsetypically decreases performance as it prevents Three.js from skipping off-screen meshes. If this is intentional for instanced meshes, please add a more detailed comment explaining the reasoning.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (3)
renderer/viewer/lib/mesher/models.ts (1)
662-665: Fix missing block count increment for culled instanced blocks.When an instanced block is culled, the code skips it without incrementing
blocksCount, leading to incorrect statistics.if (shouldCullInstancedBlock(world, cursor, block)) { // Block is completely surrounded, skip rendering + attr.blocksCount++ continue }renderer/viewer/three/instancedRenderer.ts (2)
483-503: Complete the UV mapping implementation.The UV mapping in
createCustomGeometryis noted as simplified and doesn't handle all 6 faces properly, which could lead to incorrect texture mapping for instanced blocks.
609-614: Fix potential block ID conflict.The method uses
blockIdfrom worker data but the class also has its own ID generation system viagetBlockId(). This could lead to ID collisions.
🧹 Nitpick comments (1)
src/optionsGuiScheme.tsx (1)
156-156: Fix typo in label text."Instacing" should be "Instancing".
- <span>Instacing</span>:{' '} + <span>Instancing</span>:{' '}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
README.MD(0 hunks)renderer/viewer/lib/mesher/instancingUtils.ts(1 hunks)renderer/viewer/lib/mesher/mesher.ts(7 hunks)renderer/viewer/lib/mesher/models.ts(5 hunks)renderer/viewer/lib/mesher/shared.ts(4 hunks)renderer/viewer/lib/mesher/world.ts(3 hunks)renderer/viewer/lib/worldrendererCommon.ts(9 hunks)renderer/viewer/three/entities.ts(2 hunks)renderer/viewer/three/getPreflatBlock.ts(1 hunks)renderer/viewer/three/holdingBlock.ts(3 hunks)renderer/viewer/three/instancedRenderer.ts(1 hunks)renderer/viewer/three/panorama.ts(1 hunks)renderer/viewer/three/threeJsMedia.ts(1 hunks)renderer/viewer/three/worldrendererThree.ts(20 hunks)src/optionsGuiScheme.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- README.MD
🚧 Files skipped from review as they are similar to previous changes (4)
- renderer/viewer/lib/mesher/mesher.ts
- renderer/viewer/lib/mesher/world.ts
- renderer/viewer/lib/mesher/shared.ts
- renderer/viewer/lib/worldrendererCommon.ts
🧰 Additional context used
📓 Path-based instructions (1)
renderer/**/*.ts
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .cursor/rules/vars-usage.mdc
🧠 Learnings (8)
renderer/viewer/three/threeJsMedia.ts (2)
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : Do not use the global variable `bot` directly in any file under the `renderer/` directory or its subfolders (e.g., `renderer/viewer/three/worldrendererThree.ts`).
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, files under renderer/ directory must not access the global `bot` variable directly according to .cursor/rules/vars-usage.mdc. The updateNameTagVisibility method in renderer/viewer/three/entities.ts currently violates this rule by accessing bot.teamMap and bot.username. Team information should be passed through entity update events from the world data emitter instead.
renderer/viewer/three/panorama.ts (3)
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : Do not use the global variable `bot` directly in any file under the `renderer/` directory or its subfolders (e.g., `renderer/viewer/three/worldrendererThree.ts`).
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : In renderer code (such as in `WorldRendererThree`), use the `playerState` property (e.g., `worldRenderer.playerState.gameMode`) to access player state. The implementation for `playerState` lives in `src/mineflayer/playerState.ts`.
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, files under renderer/ directory must not access the global `bot` variable directly according to .cursor/rules/vars-usage.mdc. The updateNameTagVisibility method in renderer/viewer/three/entities.ts currently violates this rule by accessing bot.teamMap and bot.username. Team information should be passed through entity update events from the world data emitter instead.
src/optionsGuiScheme.tsx (1)
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : In renderer code (such as in `WorldRendererThree`), use the `playerState` property (e.g., `worldRenderer.playerState.gameMode`) to access player state. The implementation for `playerState` lives in `src/mineflayer/playerState.ts`.
renderer/viewer/three/holdingBlock.ts (4)
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : Do not use the global variable `bot` directly in any file under the `renderer/` directory or its subfolders (e.g., `renderer/viewer/three/worldrendererThree.ts`).
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : In renderer code (such as in `WorldRendererThree`), use the `playerState` property (e.g., `worldRenderer.playerState.gameMode`) to access player state. The implementation for `playerState` lives in `src/mineflayer/playerState.ts`.
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, files under renderer/ directory must not access the global `bot` variable directly according to .cursor/rules/vars-usage.mdc. The updateNameTagVisibility method in renderer/viewer/three/entities.ts currently violates this rule by accessing bot.teamMap and bot.username. Team information should be passed through entity update events from the world data emitter instead.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : In renderer code, all bot/player state and events must be accessed via explicit interfaces, state managers, or passed-in objects, never by referencing `bot` directly.
renderer/viewer/three/entities.ts (5)
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, files under renderer/ directory must not access the global `bot` variable directly according to .cursor/rules/vars-usage.mdc. The updateNameTagVisibility method in renderer/viewer/three/entities.ts currently violates this rule by accessing bot.teamMap and bot.username. Team information should be passed through entity update events from the world data emitter instead.
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, team information for entities should be inlined into entity update events from the world data emitter rather than accessing the global bot object from renderer code. This maintains better separation of concerns between the data layer and renderer.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : Do not use the global variable `bot` directly in any file under the `renderer/` directory or its subfolders (e.g., `renderer/viewer/three/worldrendererThree.ts`).
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : In renderer code (such as in `WorldRendererThree`), use the `playerState` property (e.g., `worldRenderer.playerState.gameMode`) to access player state. The implementation for `playerState` lives in `src/mineflayer/playerState.ts`.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : In renderer code, all bot/player state and events must be accessed via explicit interfaces, state managers, or passed-in objects, never by referencing `bot` directly.
renderer/viewer/lib/mesher/models.ts (5)
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : In renderer code (such as in `WorldRendererThree`), use the `playerState` property (e.g., `worldRenderer.playerState.gameMode`) to access player state. The implementation for `playerState` lives in `src/mineflayer/playerState.ts`.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : Do not use the global variable `bot` directly in any file under the `renderer/` directory or its subfolders (e.g., `renderer/viewer/three/worldrendererThree.ts`).
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, files under renderer/ directory must not access the global `bot` variable directly according to .cursor/rules/vars-usage.mdc. The updateNameTagVisibility method in renderer/viewer/three/entities.ts currently violates this rule by accessing bot.teamMap and bot.username. Team information should be passed through entity update events from the world data emitter instead.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : In renderer code, all bot/player state and events must be accessed via explicit interfaces, state managers, or passed-in objects, never by referencing `bot` directly.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Some other global variables that can be used without window prefixes are listed in src/globals.d.ts.
renderer/viewer/three/instancedRenderer.ts (5)
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : Do not use the global variable `bot` directly in any file under the `renderer/` directory or its subfolders (e.g., `renderer/viewer/three/worldrendererThree.ts`).
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : In renderer code (such as in `WorldRendererThree`), use the `playerState` property (e.g., `worldRenderer.playerState.gameMode`) to access player state. The implementation for `playerState` lives in `src/mineflayer/playerState.ts`.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : In renderer code, all bot/player state and events must be accessed via explicit interfaces, state managers, or passed-in objects, never by referencing `bot` directly.
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, files under renderer/ directory must not access the global `bot` variable directly according to .cursor/rules/vars-usage.mdc. The updateNameTagVisibility method in renderer/viewer/three/entities.ts currently violates this rule by accessing bot.teamMap and bot.username. Team information should be passed through entity update events from the world data emitter instead.
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, team information for entities should be inlined into entity update events from the world data emitter rather than accessing the global bot object from renderer code. This maintains better separation of concerns between the data layer and renderer.
renderer/viewer/three/worldrendererThree.ts (6)
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : Do not use the global variable `bot` directly in any file under the `renderer/` directory or its subfolders (e.g., `renderer/viewer/three/worldrendererThree.ts`).
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : In renderer code (such as in `WorldRendererThree`), use the `playerState` property (e.g., `worldRenderer.playerState.gameMode`) to access player state. The implementation for `playerState` lives in `src/mineflayer/playerState.ts`.
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, files under renderer/ directory must not access the global `bot` variable directly according to .cursor/rules/vars-usage.mdc. The updateNameTagVisibility method in renderer/viewer/three/entities.ts currently violates this rule by accessing bot.teamMap and bot.username. Team information should be passed through entity update events from the world data emitter instead.
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, team information for entities should be inlined into entity update events from the world data emitter rather than accessing the global bot object from renderer code. This maintains better separation of concerns between the data layer and renderer.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : In renderer code, all bot/player state and events must be accessed via explicit interfaces, state managers, or passed-in objects, never by referencing `bot` directly.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Some other global variables that can be used without window prefixes are listed in src/globals.d.ts.
🧬 Code Graph Analysis (1)
renderer/viewer/lib/mesher/instancingUtils.ts (1)
renderer/viewer/lib/mesher/world.ts (1)
World(34-246)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (18)
renderer/viewer/three/panorama.ts (1)
206-206: LGTM! Correct integration with new scene hierarchy.The change from
scenetorealSceneproperly aligns the panorama renderer with the new layered scene structure introduced in the instanced rendering refactoring.renderer/viewer/three/threeJsMedia.ts (1)
568-568: LGTM! Good performance optimization.Filtering by visibility before raycasting prevents unnecessary intersection tests against invisible objects, improving performance and logical correctness.
renderer/viewer/three/holdingBlock.ts (3)
192-192: LGTM! Simplified method signature aligns with new architecture.Removing the unused parameters (
originalCamera,ambientLight,directionalLight) simplifies the API and aligns with the new shared scene approach.
208-208: LGTM! Efficient use of shared template scene.Using
this.worldRenderer.templateSceneinstead of creating new scenes per frame improves performance and aligns with the new layered scene architecture.
243-243: LGTM! Proper cleanup after rendering.Explicitly removing the
cameraGroupfrom the scene after rendering ensures proper cleanup and prevents memory leaks.renderer/viewer/three/entities.ts (2)
733-733: LGTM!Adding the
meshGeometryproperty provides direct access to the geometry for consumers like the instanced renderer, enabling efficient geometry reuse.
1331-1336: Good improvement to filter by visibility.The rename to
debugRaycastScenefollows a more conventional naming pattern, and filtering for visible objects ensures the debug raycasting behaves consistently with actual gameplay raycasting.renderer/viewer/three/worldrendererThree.ts (11)
11-11: LGTM: Clean import additions.The new imports for
InstancingModeandInstancedRendererare appropriately added to support the instanced rendering feature.Also applies to: 27-27
38-42: LGTM: Well-structured property additions.The property changes support the new instanced rendering and scene management features:
- Making
holdingBlockproperties optional improves null safety- The layered scene structure (
realScene→scenegroup) provides better organization- The
instancedRendererandworldOffsetproperties are appropriately typedAlso applies to: 54-54, 80-80
106-106: LGTM: Proper initialization pattern.The
instancedRendererinitialization and asyncinit()call follow a clean initialization pattern for the new instanced rendering functionality.Also applies to: 115-117
119-123: Note: World origin shifting prepared but not active.The world origin functionality is scaffolded but commented out (lines 121-122). This appears intentional for future large-world coordinate handling, but ensure the incomplete implementation doesn't cause issues.
Also applies to: 535-544, 511-512
155-163: LGTM: Safe configuration exposure.The
getInstancedBlocksDatamethod properly handles null checks and exposes the necessary configuration for worker messaging.
169-173: LGTM: Improved scene management architecture.The layered scene structure with
realScene→scenegroup and separatetemplateSceneprovides better organization. The reactive light updates properly recreate the template scene when lighting changes.Also applies to: 177-190, 196-196, 201-201, 206-206
232-232: LGTM: Consistent null safety pattern.The null checks for
holdingBlockandholdingBlockLeftare consistently applied across swing animations, texture loading, and rendering, preventing potential runtime errors.Also applies to: 234-234, 274-281, 798-799
261-262: LGTM: Comprehensive instanced renderer integration.The instanced renderer is properly integrated across the lifecycle:
- Initialization after asset updates
- Statistics reporting in debug overlay
- Section-based instance management from worker data
- Proper cleanup in
removeColumnanddestroymethodsAlso applies to: 339-339, 345-349, 396-405, 983-984, 1025-1025
995-1008: LGTM: Clear instancing mode determination.The instancing mode logic correctly prioritizes configuration flags with clear precedence:
enableSingleColorMode→ColorOnly,forceInstancedOnly→BlockInstancingOnly, otherwiseBlockInstancingwhenuseInstancedRenderingis enabled.
285-285: LGTM: Updated rendering pipeline.The rendering pipeline correctly uses
realSceneas the root and includes a defensive check for empty geometry before creating meshes.Also applies to: 789-789, 414-414
1-1202: LGTM: Coding guidelines compliance.The file correctly avoids direct
botusage and appropriately usesthis.playerStateReactiveandthis.playerStateUtilsfor accessing player state, in compliance with the renderer coding guidelines.
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-4tb6vdx9h-zaro.vercel.app |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
renderer/viewer/lib/mesher/models.ts (1)
135-138: Document the liquid instancing implementation plan.The early return with TODO comment indicates liquid instancing is not yet implemented. Consider documenting the intended approach for liquid instancing or creating a tracking issue.
Would you like me to help design the liquid instancing implementation or create a tracking issue for this feature?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
renderer/viewer/lib/mesher/instancingUtils.ts(1 hunks)renderer/viewer/lib/mesher/models.ts(5 hunks)renderer/viewer/lib/mesher/shared.ts(4 hunks)renderer/viewer/lib/mesher/world.ts(3 hunks)renderer/viewer/three/instancedRenderer.ts(1 hunks)renderer/viewer/three/worldrendererThree.ts(20 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- renderer/viewer/lib/mesher/shared.ts
- renderer/viewer/lib/mesher/world.ts
- renderer/viewer/three/worldrendererThree.ts
🧰 Additional context used
📓 Path-based instructions (1)
renderer/**/*.ts
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .cursor/rules/vars-usage.mdc
🧠 Learnings (3)
renderer/viewer/lib/mesher/instancingUtils.ts (2)
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, files under renderer/ directory must not access the global `bot` variable directly according to .cursor/rules/vars-usage.mdc. The updateNameTagVisibility method in renderer/viewer/three/entities.ts currently violates this rule by accessing bot.teamMap and bot.username. Team information should be passed through entity update events from the world data emitter instead.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : Do not use the global variable `bot` directly in any file under the `renderer/` directory or its subfolders (e.g., `renderer/viewer/three/worldrendererThree.ts`).
renderer/viewer/lib/mesher/models.ts (5)
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : In renderer code (such as in `WorldRendererThree`), use the `playerState` property (e.g., `worldRenderer.playerState.gameMode`) to access player state. The implementation for `playerState` lives in `src/mineflayer/playerState.ts`.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : Do not use the global variable `bot` directly in any file under the `renderer/` directory or its subfolders (e.g., `renderer/viewer/three/worldrendererThree.ts`).
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, files under renderer/ directory must not access the global `bot` variable directly according to .cursor/rules/vars-usage.mdc. The updateNameTagVisibility method in renderer/viewer/three/entities.ts currently violates this rule by accessing bot.teamMap and bot.username. Team information should be passed through entity update events from the world data emitter instead.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : In renderer code, all bot/player state and events must be accessed via explicit interfaces, state managers, or passed-in objects, never by referencing `bot` directly.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Some other global variables that can be used without window prefixes are listed in src/globals.d.ts.
renderer/viewer/three/instancedRenderer.ts (5)
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : In renderer code (such as in `WorldRendererThree`), use the `playerState` property (e.g., `worldRenderer.playerState.gameMode`) to access player state. The implementation for `playerState` lives in `src/mineflayer/playerState.ts`.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : Do not use the global variable `bot` directly in any file under the `renderer/` directory or its subfolders (e.g., `renderer/viewer/three/worldrendererThree.ts`).
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : In renderer code, all bot/player state and events must be accessed via explicit interfaces, state managers, or passed-in objects, never by referencing `bot` directly.
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, files under renderer/ directory must not access the global `bot` variable directly according to .cursor/rules/vars-usage.mdc. The updateNameTagVisibility method in renderer/viewer/three/entities.ts currently violates this rule by accessing bot.teamMap and bot.username. Team information should be passed through entity update events from the world data emitter instead.
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, team information for entities should be inlined into entity update events from the world data emitter rather than accessing the global bot object from renderer code. This maintains better separation of concerns between the data layer and renderer.
🧬 Code Graph Analysis (1)
renderer/viewer/lib/mesher/instancingUtils.ts (2)
renderer/viewer/three/instancedRenderer.ts (1)
isBlockInstanceable(729-731)renderer/viewer/lib/mesher/world.ts (1)
World(34-246)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (10)
renderer/viewer/lib/mesher/instancingUtils.ts (1)
3-8: LGTM! Clean and correct implementation.The function properly checks if a block is instanceable by looking up its
stateIdin the world'sinstancedBlocksrecord. The implementation includes appropriate null safety and follows the expected pattern for this utility function.renderer/viewer/lib/mesher/models.ts (3)
525-553: Excellent occlusion culling implementation.The
shouldCullInstancedBlockfunction correctly implements face-based culling by checking all 6 faces for neighboring blocks. The logic properly handles special cases like glass/ice blocks and uses efficient Vec3 reuse to avoid allocations.
556-586: Well-structured instancing mode integration.The function signature update and instancing mode handling is clean and follows a logical flow. The
instancedBlocksaddition to the output structure properly supports the instanced rendering pipeline.
657-680: Solid instancing logic with proper fallback behavior.The instancing detection, culling, and data collection logic is well-implemented. The fallback behavior for non-instanceable blocks when
forceInstancedOnlyis disabled ensures compatibility with existing rendering.renderer/viewer/three/instancedRenderer.ts (6)
64-76: Well-designed dynamic instance management.The instance limits, growth factors, and memory tracking provide good protection against memory issues while allowing efficient scaling. The separation of per-block and total instance limits is a solid design choice.
124-175: Robust mesh resizing implementation.The
resizeInstancedMeshmethod correctly handles capacity growth by creating new meshes, copying existing instances, and properly disposing of old resources. The instance count tracking and verification logic ensures consistency.
476-573: Comprehensive UV mapping with proper texture atlas support.The
createCustomGeometrymethod correctly handles UV coordinate mapping for all 6 cube faces with rotation support. The face index mapping and texture atlas coordinate application is properly implemented.
586-650: Excellent instance lifecycle management.The
handleInstancedBlocksFromWorkermethod properly manages instance updates by removing old instances, validating capacity, and adding new instances with correct matrix transformations. The section tracking ensures proper cleanup.
671-727: Correct index remapping after compaction.The
removeInstancesFromBlockmethod properly handles instance removal by building an index mapping during compaction and using it to update all section tracking. This addresses the previous review concern about incorrect index remapping.
767-805: Comprehensive statistics and monitoring.The
getStatsmethod provides detailed insights into instance utilization, memory usage, and performance metrics. The visibility state tracking and utilization calculations are valuable for debugging and optimization.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
renderer/viewer/lib/mesher/models.ts (1)
680-682: Fix missing block count increment for culled instanced blocks.When an instanced block is culled, the code skips it without incrementing
blocksCount, leading to incorrect statistics.Apply this fix:
if (shouldCullInstancedBlock(world, cursor, block)) { // Block is completely surrounded, skip rendering + attr.blocksCount++ continue }
🧹 Nitpick comments (7)
src/index.ts (1)
279-282: Improve Chrome extension error detection to avoid false positives.The logic to filter Chrome extension errors is sound for preventing external interference with the application's error handling. However, the current implementation could potentially ignore legitimate application errors that happen to mention chrome extensions in their stack trace.
Consider a more robust check that verifies the error actually originates from a Chrome extension:
- if (e.reason?.stack?.includes('chrome-extension://')) { - // ignore issues caused by chrome extension - return - } + if (e.reason?.stack?.split('\n').some(line => + line.trim().startsWith('at ') && line.includes('chrome-extension://') + )) { + // ignore issues caused by chrome extension + return + }This checks if any stack frame actually originates from a chrome-extension URL rather than just containing the string anywhere in the stack trace.
Note: This change appears unrelated to the main PR objective of implementing instanced rendering mode. Consider if this should be in a separate PR for better change tracking.
renderer/viewer/lib/mesher/mesher.ts (2)
194-237: Improve debug system to reduce potential performance impact.The debug geometry tracking system has several concerns:
- Global variables pollute the namespace
- Storing geometry objects may prevent garbage collection
- 20ms interval could impact performance when enabled
Consider these improvements:
-// Debug flag to spam last geometry output -globalThis.DEBUG_GEOMETRY_SPAM = false // set to true to enable geometry spam for performance testing -globalThis.lastGeometryKey = null +// Move debug system to a namespaced object +const DEBUG_CONFIG = { + GEOMETRY_SPAM: false, + lastGeometryKey: null as string | null +} -const lastGeometryEntries: GeometryEntry[] = [] +// Consider using WeakMap or clearing geometry references after storing +const lastGeometryKeys: string[] = [] // Store only keys, not full geometry objectsAlso consider increasing the debug spam interval from 20ms to reduce performance impact when enabled.
Also applies to: 268-287
224-237: Optimize debug tracking to reduce performance overhead.The debug geometry tracking runs in the main processing loop and stores full geometry objects, which could impact performance and memory usage.
Consider these optimizations:
- // Store last geometry for debug spam - globalThis.lastGeometryKey = key - - // Track unique geometry entries for aggressive debugging - const existingIndex = lastGeometryEntries.findIndex(entry => entry.key === key) - if (existingIndex >= 0) { - // Update existing entry with new geometry - lastGeometryEntries[existingIndex].geometry = geometry - } else { - // Add new entry - lastGeometryEntries.push({ key, geometry }) - if (lastGeometryEntries.length > MAX_GEOMETRY_ENTRIES) { - lastGeometryEntries.shift() // Remove oldest - } - } + if (DEBUG_CONFIG.GEOMETRY_SPAM) { + // Only track when debug mode is actually enabled + DEBUG_CONFIG.lastGeometryKey = key + // Consider storing only essential data instead of full geometry + updateDebugGeometryTracking(key, geometry) + }renderer/viewer/lib/mesher/models.ts (1)
135-139: Document liquid rendering limitation in instancing mode.The early return skips liquid rendering when instancing is enabled. Consider adding a more descriptive comment about this limitation.
- if (instancingEnabled) { - return // todo - } + if (instancingEnabled) { + // TODO: Implement instanced liquid rendering + // For now, liquids are skipped in instanced mode + return + }renderer/viewer/three/worldrendererThree.ts (3)
122-125: Consider removing commented code or implementing world origin feature.The
updateWorldOriginmethod is commented out, suggesting incomplete implementation. Either implement this feature or remove the placeholder code to avoid confusion.- // Add this method to update world origin - private updateWorldOrigin (pos: THREE.Vector3) { - // this.worldOffset.copy(pos) - // this.scene.position.copy(this.worldOffset).multiplyScalar(-1) - }
1043-1066: Optimize instancing mode calculation performance.The
getInstancingModemethod performs distance calculations and could be called frequently during camera movement. Consider:
- Caching results based on camera section position
- Using squared distance to avoid Math.hypot overhead
- Adding early returns for static modes
getInstancingMode (pos: Vec3) { const { useInstancedRendering, enableSingleColorMode, forceInstancedOnly, dynamicInstancing, dynamicInstancingModeDistance, dynamicColorModeDistance } = this.worldRendererConfig let instancingMode = InstancingMode.None if (useInstancedRendering || enableSingleColorMode) { instancingMode = enableSingleColorMode ? InstancingMode.ColorOnly : forceInstancedOnly ? InstancingMode.BlockInstancingOnly : InstancingMode.BlockInstancing } else if (dynamicInstancing) { const dx = pos.x / 16 - this.cameraSectionPos.x const dz = pos.z / 16 - this.cameraSectionPos.z - const distance = Math.floor(Math.hypot(dx, dz)) + const distanceSquared = dx * dx + dz * dz + const distance = Math.floor(Math.sqrt(distanceSquared)) if (distance > dynamicColorModeDistance) { instancingMode = InstancingMode.ColorOnly } else if (distance > dynamicInstancingModeDistance) { instancingMode = InstancingMode.BlockInstancingOnly } } return instancingMode }
372-391: Consider optimizing dynamic instancing updates.The camera section position update triggers section updates when instancing modes change. This could cause performance issues during rapid camera movement. Consider:
- Debouncing section updates
- Batching multiple section changes
- Adding hysteresis to prevent mode flickering at distance boundaries
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
renderer/viewer/lib/mesher/instancingUtils.ts(1 hunks)renderer/viewer/lib/mesher/mesher.ts(7 hunks)renderer/viewer/lib/mesher/models.ts(5 hunks)renderer/viewer/lib/mesher/shared.ts(4 hunks)renderer/viewer/lib/mesher/world.ts(3 hunks)renderer/viewer/lib/worldrendererCommon.ts(9 hunks)renderer/viewer/three/instancedRenderer.ts(1 hunks)renderer/viewer/three/worldrendererThree.ts(22 hunks)src/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- renderer/viewer/lib/mesher/instancingUtils.ts
- renderer/viewer/lib/mesher/world.ts
- renderer/viewer/lib/mesher/shared.ts
- renderer/viewer/lib/worldrendererCommon.ts
🧰 Additional context used
🧠 Learnings (4)
renderer/viewer/lib/mesher/models.ts (5)
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : Do not use the global variable `bot` directly in any file under the `renderer/` directory or its subfolders (e.g., `renderer/viewer/three/worldrendererThree.ts`).
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, files under renderer/ directory must not access the global `bot` variable directly according to .cursor/rules/vars-usage.mdc. The updateNameTagVisibility method in renderer/viewer/three/entities.ts currently violates this rule by accessing bot.teamMap and bot.username. Team information should be passed through entity update events from the world data emitter instead.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : In renderer code, all bot/player state and events must be accessed via explicit interfaces, state managers, or passed-in objects, never by referencing `bot` directly.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : In renderer code (such as in `WorldRendererThree`), use the `playerState` property (e.g., `worldRenderer.playerState.gameMode`) to access player state. The implementation for `playerState` lives in `src/mineflayer/playerState.ts`.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Some other global variables that can be used without window prefixes are listed in src/globals.d.ts.
renderer/viewer/lib/mesher/mesher.ts (4)
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : In renderer code (such as in `WorldRendererThree`), use the `playerState` property (e.g., `worldRenderer.playerState.gameMode`) to access player state. The implementation for `playerState` lives in `src/mineflayer/playerState.ts`.
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, files under renderer/ directory must not access the global `bot` variable directly according to .cursor/rules/vars-usage.mdc. The updateNameTagVisibility method in renderer/viewer/three/entities.ts currently violates this rule by accessing bot.teamMap and bot.username. Team information should be passed through entity update events from the world data emitter instead.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : Do not use the global variable `bot` directly in any file under the `renderer/` directory or its subfolders (e.g., `renderer/viewer/three/worldrendererThree.ts`).
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Some other global variables that can be used without window prefixes are listed in src/globals.d.ts.
renderer/viewer/three/instancedRenderer.ts (5)
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : In renderer code (such as in `WorldRendererThree`), use the `playerState` property (e.g., `worldRenderer.playerState.gameMode`) to access player state. The implementation for `playerState` lives in `src/mineflayer/playerState.ts`.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : Do not use the global variable `bot` directly in any file under the `renderer/` directory or its subfolders (e.g., `renderer/viewer/three/worldrendererThree.ts`).
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : In renderer code, all bot/player state and events must be accessed via explicit interfaces, state managers, or passed-in objects, never by referencing `bot` directly.
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, files under renderer/ directory must not access the global `bot` variable directly according to .cursor/rules/vars-usage.mdc. The updateNameTagVisibility method in renderer/viewer/three/entities.ts currently violates this rule by accessing bot.teamMap and bot.username. Team information should be passed through entity update events from the world data emitter instead.
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, team information for entities should be inlined into entity update events from the world data emitter rather than accessing the global bot object from renderer code. This maintains better separation of concerns between the data layer and renderer.
renderer/viewer/three/worldrendererThree.ts (6)
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : In renderer code (such as in `WorldRendererThree`), use the `playerState` property (e.g., `worldRenderer.playerState.gameMode`) to access player state. The implementation for `playerState` lives in `src/mineflayer/playerState.ts`.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : Do not use the global variable `bot` directly in any file under the `renderer/` directory or its subfolders (e.g., `renderer/viewer/three/worldrendererThree.ts`).
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, files under renderer/ directory must not access the global `bot` variable directly according to .cursor/rules/vars-usage.mdc. The updateNameTagVisibility method in renderer/viewer/three/entities.ts currently violates this rule by accessing bot.teamMap and bot.username. Team information should be passed through entity update events from the world data emitter instead.
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, team information for entities should be inlined into entity update events from the world data emitter rather than accessing the global bot object from renderer code. This maintains better separation of concerns between the data layer and renderer.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : In renderer code, all bot/player state and events must be accessed via explicit interfaces, state managers, or passed-in objects, never by referencing `bot` directly.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Some other global variables that can be used without window prefixes are listed in src/globals.d.ts.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (30)
renderer/viewer/lib/mesher/mesher.ts (6)
4-4: LGTM: Proper import and data structure enhancement.The new
InstancingModeimport and enhanceddirtySectionsdata structure appropriately support the instanced rendering feature by tracking both dirty count and instancing mode per section.Also applies to: 20-20
50-50: LGTM: Backward-compatible function enhancement.The updated
setSectionDirtyfunction correctly adds instancing mode support while maintaining backward compatibility through the default parameter value.Also applies to: 63-63
100-101: LGTM: Proper worker message handling for instancing.The message handlers correctly propagate instancing data with appropriate fallbacks (
data.instancedBlockIds || {}anddata.instancingMode || InstancingMode.None).Also applies to: 107-107
213-213: LGTM: Proper instancing mode integration.The main processing loop correctly extracts
instancingModefrom the enhanceddirtySectionsstructure and passes it togetSectionGeometry.Also applies to: 219-219
221-221: Verify impact of removing transferable buffers.The transferable buffer optimization was removed from the
postMessagecall. This could impact performance by forcing buffer copying instead of transferring ownership.Was this removal intentional? If so, please document the reason. If the goal is to support the debug geometry spam feature, consider making this conditional:
-postMessage({ type: 'geometry', key, geometry, workerIndex }/* , transferable */) +const transferable = globalThis.DEBUG_GEOMETRY_SPAM ? [] : [geometry.positions?.buffer, geometry.normals?.buffer, geometry.colors?.buffer, geometry.uvs?.buffer].filter(Boolean) +postMessage({ type: 'geometry', key, geometry, workerIndex }, transferable)
243-243: LGTM: Correct message count handling.Using
dirtyTimes.timesensures the proper number ofsectionFinishedmessages are posted, maintaining correct worker-main thread message balance.renderer/viewer/lib/mesher/models.ts (7)
8-9: LGTM: Necessary imports for instancing support.The new imports properly support the instanced rendering functionality.
525-553: LGTM: Well-implemented occlusion culling for instanced blocks.The
shouldCullInstancedBlockfunction correctly implements occlusion culling logic with proper handling of transparent blocks and special cases for glass/ice. The Vec3 caching optimization is good for performance.
556-572: LGTM: Correct 4x4 matrix generation for instancing.The
calculateInstanceMatrixfunction correctly creates a transformation matrix with proper translation values for Three.js instanced rendering.
576-576: LGTM: Proper function extension for instancing support.The
getSectionGeometryfunction signature and initialization correctly add instancing mode support while maintaining backward compatibility. The new output properties appropriately extend the mesher output structure.Also applies to: 579-606
668-673: LGTM: Consistent liquid rendering parameter updates.The
renderLiquidcalls correctly pass theforceInstancedOnlyparameter to maintain consistency with the instancing mode.
685-710: LGTM: Well-structured instanced block data collection.The instanced block processing correctly:
- Groups blocks by name in the
instancedBlocksmap- Stores positions and pre-calculated transformation matrices
- Uses appropriate offset based on instancing mode
- Increments block count and skips regular geometry generation
The logic is sound and efficient.
713-716: LGTM: Correct forceInstancedOnly mode implementation.The logic properly skips non-instanceable blocks when
forceInstancedOnlymode is enabled, creating the intended simplified rendering mode.renderer/viewer/three/instancedRenderer.ts (6)
12-21: LGTM: Well-structured class initialization and helper functions.The
parseRgbColorfunction properly handles RGB string parsing with fallback, and the class constructor establishes reasonable defaults for instance management and proper resource initialization.Also applies to: 83-92
133-184: LGTM: Robust mesh resizing implementation.The
resizeInstancedMeshfunction correctly:
- Creates new mesh with increased capacity
- Copies only actual instances (using
actualInstanceCount)- Properly disposes old resources
- Updates tracking and scene management
The implementation avoids common pitfalls like copying uninitialized instances.
230-301: LGTM: Comprehensive block data preparation.The block preparation logic correctly:
- Validates full cube blocks for instanceability
- Maps texture coordinates and rotations per face
- Handles special cases like texture overrides and debug mappings
- Stores necessary data for instanced rendering
The implementation is thorough and handles edge cases appropriately.
Also applies to: 303-409
620-681: LGTM: Efficient worker data handling with proper cleanup.The
handleInstancedBlocksFromWorkerfunction correctly:
- Tracks instances per section for efficient updates
- Removes old instances before adding new ones
- Uses pre-calculated matrices for performance
- Handles dynamic capacity management
- Defers scene addition until meshes are actually used
The implementation prevents instance accumulation and manages resources efficiently.
702-758: LGTM: Correct instance removal and compaction logic.The
removeInstancesFromBlockfunction properly implements:
- Index mapping during compaction to handle gaps
- Correct remapping of section tracking indices
- Proper cleanup of empty meshes and resources
- Accurate instance count updates
This addresses the previous review comments about index remapping issues.
777-843: LGTM: Comprehensive resource management and monitoring.The resource cleanup and statistics methods provide:
- Proper disposal of all meshes, materials, and data structures
- Detailed performance metrics including memory usage and utilization
- Useful debugging information for optimization
- Proper visibility management across all instances
The implementation enables good observability and prevents resource leaks.
renderer/viewer/three/worldrendererThree.ts (11)
11-11: LGTM: Import additions for instanced rendering support.The new imports for
MesherGeometryOutput,InstancingMode, andInstancedRendererare appropriate for the instanced rendering feature implementation.Also applies to: 27-27
34-34: Good: Per-section instancing mode tracking.The
sectionInstancingModeproperty enables efficient tracking of rendering modes per section, supporting dynamic instancing based on distance or other criteria.
108-108: LGTM: Instanced renderer initialization.The
InstancedRendererinitialization follows the established pattern of other renderer components.
161-165: Good: Template scene management for light cloning.The
resetTemplateScenemethod provides a clean way to maintain consistent lighting across different rendering contexts, supporting the holding block rendering optimization mentioned in the AI summary.
214-222: LGTM: Clean API for instanced blocks configuration.The
getInstancedBlocksDatamethod provides a clean interface for accessing instanced renderer configuration with proper null checking.
409-424: Good: Proper instanced blocks handling from worker.The worker message handling correctly:
- Removes existing section instances before processing new data
- Checks for instanced blocks data before processing
- Passes the appropriate instancing mode to the instanced renderer
935-962: LGTM: Well-implemented chunk border visualization.The chunk border methods properly:
- Handle visibility toggling without recreating objects
- Use static geometry for performance
- Clean up resources appropriately
- Follow consistent naming conventions
1031-1033: Good: Proper resource cleanup for instanced renderer.The cleanup code correctly removes instanced renderer resources in both
removeColumnanddestroymethods, preventing memory leaks and ensuring clean state management.Also applies to: 1091-1091
1-1268: Overall assessment: Well-architected instanced rendering integration.This is a comprehensive implementation of instanced rendering that:
Strengths:
- Clean separation of concerns with dedicated InstancedRenderer class
- Proper resource management and cleanup
- Flexible instancing modes (none, color-only, block instancing)
- Distance-based dynamic instancing for performance optimization
- Consistent integration with existing meshing pipeline
Areas for optimization:
- Dynamic instancing distance calculations could benefit from caching/optimization
- Scene graph restructuring needs verification to ensure no regressions
- Consider debouncing frequent section updates during camera movement
The implementation follows established patterns and maintains good code organization while adding significant new functionality.
813-813: Render target consistency confirmedAll child objects continue to be added/removed on
this.scene, which is attached once tothis.realSceneinresetScene, and onlythis.realSceneis ever passed torenderer.render. No lingeringrenderer.render(this.scene, …)calls remain in the viewer code (the only other calls are in experiments or non-renderer files). Panorama correctly rebinds itsscenetoworldRenderer.realScenewhen usingWorldRendererThree. No further changes needed.
40-44: Scene graph restructuring verified
All objects continue to be added to thesceneGroup, which is in turn added torealScene; rendering and template‐scene usage are correct—no changes needed.• In
renderer/viewer/three/worldrendererThree.ts:
–this.realScene.add(this.scene)confirms the Group is attached to the root.
–this.renderer.render(this.realScene, cam)(line 813) uses the new root scene.
–this.templateSceneis populated with cloned lights and used inresetTemplateScene().
• Inrenderer/viewer/three/holdingBlock.tsthetemplateSceneis correctly referenced for cameraGroup placement.All original
scene.add()calls now target the child Group and render calls target the rootrealScene.
| } else { | ||
| postMessage({ type: 'sectionFinished', key, workerIndex }) | ||
| } | ||
| } | ||
|
|
||
| const softCleanup = () => { | ||
| // clean block cache and loaded chunks | ||
| world = new World(world.config.version) | ||
| globalThis.world = world | ||
| world.blockCache = {} |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify that partial cleanup is sufficient.
The softCleanup function now only clears blockCache instead of recreating the entire World instance. Ensure this targeted cleanup doesn't leave stale state in other World properties that might affect instanced rendering.
🏁 Script executed:
#!/bin/bash
# Search for World instance properties that might need cleanup
ast-grep --pattern 'class World {
$$$
}'Length of output: 16342
I’d like to confirm exactly what the new softCleanup method does. Let’s locate its definition:
🏁 Script executed:
#!/bin/bash
rg -n "softCleanup" -C5 renderer/viewer/lib/mesher/mesher.tsLength of output: 741
🏁 Script executed:
#!/bin/bash
rg -n "new World" -C3 renderer/viewer/lib/mesher/mesher.tsLength of output: 286
Address incomplete cleanup in softCleanup
The new softCleanup only resets world.blockCache, but the previous approach of recreating the World instance cleared all internal state. This partial cleanup can leave stale data in other caches and arrays, leading to incorrect instanced rendering or model lookups when chunks are reloaded.
Please extend softCleanup to clear all relevant World properties (or revert to re-instantiating World):
• Clear custom block models and state info
– world.customBlockModels.clear()
– world.blockStateModelInfo.clear()
– world.sentBlockStateModels.clear()
• Reset instancing buffers
– world.instancedBlocks = []
– world.instancedBlockIds = {}
• (Optional) Verify that world.columns is empty or clear it
Example patch:
const softCleanup = () => {
- // clean block cache and loaded chunks
- world.blockCache = {}
+ // reset all world caches to avoid stale state
+ world.blockCache = {}
+ world.customBlockModels.clear()
+ world.blockStateModelInfo.clear()
+ world.sentBlockStateModels.clear()
+ world.instancedBlocks = []
+ world.instancedBlockIds = {}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| world.blockCache = {} | |
| const softCleanup = () => { | |
| // reset all world caches to avoid stale state | |
| world.blockCache = {} | |
| world.customBlockModels.clear() | |
| world.blockStateModelInfo.clear() | |
| world.sentBlockStateModels.clear() | |
| world.instancedBlocks = [] | |
| world.instancedBlockIds = {} | |
| } |
🤖 Prompt for AI Agents
In renderer/viewer/lib/mesher/mesher.ts at line 71, the softCleanup function
currently only resets world.blockCache, which leaves other internal state like
customBlockModels, blockStateModelInfo, sentBlockStateModels, instancedBlocks,
and instancedBlockIds uncleared, causing stale data issues. Extend softCleanup
to also clear these collections and reset instancing buffers by calling clear()
on the maps and resetting arrays and objects to empty, ensuring all relevant
World properties are fully cleaned up to avoid incorrect rendering or lookups.
| void this.init() | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify async initialization handling.
The void this.init() pattern suggests fire-and-forget async initialization. Ensure that:
- The
init()method handles its own errors - Any initialization failures are properly logged or handled
- The renderer can function gracefully if initialization is incomplete
🏁 Script executed:
#!/bin/bash
# Description: Find the init method implementation and verify error handling
# Search for the init method definition
echo "=== Init method implementation ==="
ast-grep --pattern $'async init() {
$$$
}'
# Search for error handling in init-related code
echo "=== Error handling patterns ==="
rg "\.init\(\)" -A 5 -B 2Length of output: 1627
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Searching for async init definitions across the repo ==="
rg -n "async init" .
echo "=== Searching for all init method signatures in TypeScript files ==="
rg -n "init\\s*\\(" --type ts .
echo "=== Searching for base renderer classes in renderer/viewer/three ==="
rg -n "class .*Renderer" -n renderer/viewer/three
echo "=== Extracting context around the void this.init() call ==="
rg -n "void this.init" -A 10 -B 10 renderer/viewer/three/worldrendererThree.tsLength of output: 3259
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Extracting async init implementation from worldrendererCommon.ts ==="
rg -n "async init" -A 30 -B 10 renderer/viewer/lib/worldrendererCommon.tsLength of output: 1467
Add explicit error handling around async initialization
The fire-and-forget void this.init() in WorldRendererThree can lead to unhandled promise rejections since init() in WorldRendererCommon doesn’t catch its own errors. To address this:
• In renderer/viewer/three/worldrendererThree.ts (around line 118), wrap the call with a .catch to log or handle failures:
- void this.init()
+ void this.init().catch(err => {
+ console.error('WorldRendererThree initialization failed:', err)
+ // TODO: optionally show fallback UI or disable renderer
+ })• Consider adding a try/catch inside async init() in renderer/viewer/lib/worldrendererCommon.ts to surface errors more clearly and ensure any downstream listeners aren’t left hanging:
async init() {
if (this.active) throw new Error('WorldRendererCommon is already initialized')
try {
await Promise.all([ /* ... */ ])
// existing setup…
} catch (err) {
console.error('WorldRendererCommon.init error:', err)
throw err
}
}• Verify that on init failure the app either degrades gracefully or retries initialization.
🤖 Prompt for AI Agents
In renderer/viewer/three/worldrendererThree.ts around line 118, the call to
this.init() is fire-and-forget and can cause unhandled promise rejections. Fix
this by appending a .catch handler to log or handle any errors from init().
Additionally, in renderer/viewer/lib/worldrendererCommon.ts inside the async
init() method, wrap the existing await calls in a try/catch block that logs the
error and rethrows it to make errors explicit. Finally, ensure the application
handles init failures gracefully or retries initialization as appropriate.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
renderer/viewer/three/worldrendererThree.ts (1)
123-124: Add explicit error handling around async initializationThe fire-and-forget
void this.init()can lead to unhandled promise rejections.
🧹 Nitpick comments (6)
renderer/viewer/three/chunkMeshManager.ts (3)
727-728: Document the bug being investigated.The TODO comment mentions investigating a bug but doesn't provide context. Please add more details about what bug is being worked around here.
- // todo investigate bug and remove this so don't need to clean in section dirty + // TODO: Investigate bug where textures persist after chunk updates, causing visual artifacts. + // This check prevents duplicate texture creation but ideally shouldn't be needed. if (textures[texturekey]) return textures[texturekey]
614-633: Optimize string parsing in updateSectionsVisibility.The method parses section keys using string split and map in a potentially hot path. Consider caching parsed coordinates or using a more efficient data structure.
Cache the parsed coordinates to avoid repeated string operations:
+ private sectionCoordinatesCache = new Map<string, { x: number, y: number, z: number }>() + updateSectionsVisibility (): void { const cameraPos = this.worldRenderer.cameraSectionPos for (const [sectionKey, sectionObject] of Object.entries(this.sectionObjects)) { if (!this.performanceOverrideDistance) { sectionObject.visible = true continue } - const [x, y, z] = sectionKey.split(',').map(Number) - const sectionPos = { x: x / 16, y: y / 16, z: z / 16 } + let sectionPos = this.sectionCoordinatesCache.get(sectionKey) + if (!sectionPos) { + const [x, y, z] = sectionKey.split(',').map(Number) + sectionPos = { x: x / 16, y: y / 16, z: z / 16 } + this.sectionCoordinatesCache.set(sectionKey, sectionPos) + } // Calculate distance using hypot (same as render distance calculation) const dx = sectionPos.x - cameraPos.x const dz = sectionPos.z - cameraPos.z const distance = Math.floor(Math.hypot(dx, dz)) sectionObject.visible = distance <= this.performanceOverrideDistance } }Also clear the cache when sections are released:
releaseSection (sectionKey: string): boolean { this.cleanupSection(sectionKey) + this.sectionCoordinatesCache.delete(sectionKey)
649-653: Use URL constructor for safer URL manipulation.String replacement for URLs can be error-prone. Consider using the URL constructor for more robust URL handling.
- if (skinTexturesProxy) { - skinUrl = skinUrl?.replace('http://textures.minecraft.net/', skinTexturesProxy) - .replace('https://textures.minecraft.net/', skinTexturesProxy) - } + if (skinTexturesProxy && skinUrl) { + try { + const url = new URL(skinUrl) + if (url.hostname === 'textures.minecraft.net') { + skinUrl = skinUrl.replace(url.origin + '/', skinTexturesProxy) + } + } catch (e) { + console.warn('Invalid skin URL:', skinUrl) + } + }renderer/viewer/three/worldrendererThree.ts (3)
875-878: Document debug methods clearly.The
removeCurrentChunkmethod appears to be a debug utility but lacks documentation about its purpose and when it should be used.+ /** + * Debug method: Removes the chunk at the current camera position. + * This is useful for testing chunk loading/unloading behavior. + * @internal + */ removeCurrentChunk () { const currentChunk = this.cameraSectionPos this.removeColumn(currentChunk.x * 16, currentChunk.z * 16) }
294-302: Simplify conditional rendering logic.The holdingBlock ready state and updateItem calls are duplicated. Consider extracting to a helper method.
onAllTexturesLoaded () { - if (this.holdingBlock) { - this.holdingBlock.ready = true - this.holdingBlock.updateItem() - } - if (this.holdingBlockLeft) { - this.holdingBlockLeft.ready = true - this.holdingBlockLeft.updateItem() - } + const updateHoldingBlock = (block: HoldingBlock | undefined) => { + if (block) { + block.ready = true + block.updateItem() + } + } + updateHoldingBlock(this.holdingBlock) + updateHoldingBlock(this.holdingBlockLeft) }
855-865: Mark temporary debug method appropriately.The
readdChunksmethod removes and re-adds chunks with a timeout, which appears to be for debugging purposes.+ /** + * Debug method: Temporarily removes all chunks from the scene and re-adds them after 500ms. + * Useful for testing scene graph updates and chunk rendering. + * @internal + */ readdChunks () { const { sectionObjects } = this.chunkMeshManager
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
experiments/three.ts(2 hunks)renderer/viewer/lib/worldDataEmitter.ts(2 hunks)renderer/viewer/lib/worldrendererCommon.ts(9 hunks)renderer/viewer/three/chunkMeshManager.ts(1 hunks)renderer/viewer/three/worldrendererThree.ts(28 hunks)src/controls.ts(1 hunks)src/defaultOptions.ts(1 hunks)src/watchOptions.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/watchOptions.ts
- src/defaultOptions.ts
- renderer/viewer/lib/worldrendererCommon.ts
🧰 Additional context used
🧠 Learnings (4)
renderer/viewer/lib/worldDataEmitter.ts (5)
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, files under renderer/ directory must not access the global `bot` variable directly according to .cursor/rules/vars-usage.mdc. The updateNameTagVisibility method in renderer/viewer/three/entities.ts currently violates this rule by accessing bot.teamMap and bot.username. Team information should be passed through entity update events from the world data emitter instead.
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, team information for entities should be inlined into entity update events from the world data emitter rather than accessing the global bot object from renderer code. This maintains better separation of concerns between the data layer and renderer.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : Do not use the global variable `bot` directly in any file under the `renderer/` directory or its subfolders (e.g., `renderer/viewer/three/worldrendererThree.ts`).
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : In renderer code (such as in `WorldRendererThree`), use the `playerState` property (e.g., `worldRenderer.playerState.gameMode`) to access player state. The implementation for `playerState` lives in `src/mineflayer/playerState.ts`.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : In renderer code, all bot/player state and events must be accessed via explicit interfaces, state managers, or passed-in objects, never by referencing `bot` directly.
experiments/three.ts (5)
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, files under renderer/ directory must not access the global `bot` variable directly according to .cursor/rules/vars-usage.mdc. The updateNameTagVisibility method in renderer/viewer/three/entities.ts currently violates this rule by accessing bot.teamMap and bot.username. Team information should be passed through entity update events from the world data emitter instead.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : Do not use the global variable `bot` directly in any file under the `renderer/` directory or its subfolders (e.g., `renderer/viewer/three/worldrendererThree.ts`).
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : In renderer code (such as in `WorldRendererThree`), use the `playerState` property (e.g., `worldRenderer.playerState.gameMode`) to access player state. The implementation for `playerState` lives in `src/mineflayer/playerState.ts`.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to src/**/*.ts : In `src/` code, you may use the global variable `appViewer` from `src/appViewer.ts` directly. Do not import `appViewer` or use `window.appViewer`; use the global `appViewer` variable as-is.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Some other global variables that can be used without window prefixes are listed in src/globals.d.ts.
renderer/viewer/three/chunkMeshManager.ts (5)
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : In renderer code (such as in `WorldRendererThree`), use the `playerState` property (e.g., `worldRenderer.playerState.gameMode`) to access player state. The implementation for `playerState` lives in `src/mineflayer/playerState.ts`.
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, files under renderer/ directory must not access the global `bot` variable directly according to .cursor/rules/vars-usage.mdc. The updateNameTagVisibility method in renderer/viewer/three/entities.ts currently violates this rule by accessing bot.teamMap and bot.username. Team information should be passed through entity update events from the world data emitter instead.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : Do not use the global variable `bot` directly in any file under the `renderer/` directory or its subfolders (e.g., `renderer/viewer/three/worldrendererThree.ts`).
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, team information for entities should be inlined into entity update events from the world data emitter rather than accessing the global bot object from renderer code. This maintains better separation of concerns between the data layer and renderer.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : In renderer code, all bot/player state and events must be accessed via explicit interfaces, state managers, or passed-in objects, never by referencing `bot` directly.
renderer/viewer/three/worldrendererThree.ts (9)
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : Do not use the global variable `bot` directly in any file under the `renderer/` directory or its subfolders (e.g., `renderer/viewer/three/worldrendererThree.ts`).
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : In renderer code (such as in `WorldRendererThree`), use the `playerState` property (e.g., `worldRenderer.playerState.gameMode`) to access player state. The implementation for `playerState` lives in `src/mineflayer/playerState.ts`.
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, files under renderer/ directory must not access the global `bot` variable directly according to .cursor/rules/vars-usage.mdc. The updateNameTagVisibility method in renderer/viewer/three/entities.ts currently violates this rule by accessing bot.teamMap and bot.username. Team information should be passed through entity update events from the world data emitter instead.
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, team information for entities should be inlined into entity update events from the world data emitter rather than accessing the global bot object from renderer code. This maintains better separation of concerns between the data layer and renderer.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : In renderer code, all bot/player state and events must be accessed via explicit interfaces, state managers, or passed-in objects, never by referencing `bot` directly.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Some other global variables that can be used without window prefixes are listed in src/globals.d.ts.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to src/**/*.ts : In `src/` code, you may use the global variable `appViewer` from `src/appViewer.ts` directly. Do not import `appViewer` or use `window.appViewer`; use the global `appViewer` variable as-is.
Learnt from: zardoy
PR: zardoy/minecraft-web-client#391
File: renderer/viewer/three/threeJsUtils.ts:56-56
Timestamp: 2025-07-10T16:03:10.838Z
Learning: The minecraft-web-client project uses core-js auto polyfill injection, which means newer JavaScript features like Promise.withResolvers() are automatically polyfilled for compatibility across different environments.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to src/**/*.ts : You may use the global variable `bot` directly in any file under the `src/` directory (e.g., `src/mineflayer/playerState.ts`).
🔇 Additional comments (3)
src/controls.ts (1)
507-509: Good defensive programming improvement.The type check before invoking
toggleMicrophoneMutedis a solid improvement over optional chaining, ensuring the function exists and is callable before invocation. This prevents potential runtime errors if the microphone functionality is not available or loaded.renderer/viewer/lib/worldDataEmitter.ts (1)
23-23: LGTM! Good addition for chunk management.The updated event signature properly communicates both view distance and chunk retention parameters, which aligns well with the instanced rendering and chunk mesh management requirements.
Also applies to: 80-82
renderer/viewer/three/chunkMeshManager.ts (1)
98-116: Add null check after acquireMesh.While the code logs a warning when
acquireMeshreturns null, it should handle this case more gracefully to prevent potential errors.The current implementation already returns null properly, but consider adding error recovery:
// Get or create mesh from pool let poolEntry = this.activeSections.get(sectionKey) if (!poolEntry) { poolEntry = this.acquireMesh() if (!poolEntry) { console.warn(`ChunkMeshManager: No available mesh in pool for section ${sectionKey}`) + // Consider triggering pool expansion or cleanup of least recently used sections return null }Likely an incorrect or invalid review comment.
experiments/three.ts
Outdated
| } catch (error) { | ||
| console.error('Error creating instanced block:', error) | ||
|
|
||
| // Fallback: create a colored cube | ||
| const geometry = new THREE.BoxGeometry(1, 1, 1) | ||
| const material = new THREE.MeshLambertMaterial({ color: 0xff0000 }) | ||
| currentRefCube = new THREE.Mesh(geometry, material) | ||
| scene.add(currentRefCube) | ||
| console.log('Created fallback colored cube') | ||
| } |
There was a problem hiding this comment.
Fix fallback handling in error case.
In the error handling block, you're creating a fallback cube and assigning it to currentRefCube, but currentInstancedMesh remains null. This inconsistency could cause issues if other code expects both to be set together.
Apply this diff to ensure consistent state:
} catch (error) {
console.error('Error creating instanced block:', error)
// Fallback: create a colored cube
const geometry = new THREE.BoxGeometry(1, 1, 1)
const material = new THREE.MeshLambertMaterial({ color: 0xff0000 })
currentRefCube = new THREE.Mesh(geometry, material)
+ currentRefCube.position.set(2.5, 0.5, 0.5) // Match the intended position
scene.add(currentRefCube)
+ currentInstancedMesh = null // Explicitly set to null for consistency
console.log('Created fallback colored cube')
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (error) { | |
| console.error('Error creating instanced block:', error) | |
| // Fallback: create a colored cube | |
| const geometry = new THREE.BoxGeometry(1, 1, 1) | |
| const material = new THREE.MeshLambertMaterial({ color: 0xff0000 }) | |
| currentRefCube = new THREE.Mesh(geometry, material) | |
| scene.add(currentRefCube) | |
| console.log('Created fallback colored cube') | |
| } | |
| } catch (error) { | |
| console.error('Error creating instanced block:', error) | |
| // Fallback: create a colored cube | |
| const geometry = new THREE.BoxGeometry(1, 1, 1) | |
| const material = new THREE.MeshLambertMaterial({ color: 0xff0000 }) | |
| currentRefCube = new THREE.Mesh(geometry, material) | |
| currentRefCube.position.set(2.5, 0.5, 0.5) // Match the intended position | |
| scene.add(currentRefCube) | |
| currentInstancedMesh = null // Explicitly set to null for consistency | |
| console.log('Created fallback colored cube') | |
| } |
🤖 Prompt for AI Agents
In experiments/three.ts around lines 131 to 140, the fallback error handling
creates a fallback cube and assigns it to currentRefCube but leaves
currentInstancedMesh null, causing inconsistent state. To fix this, assign the
fallback mesh to both currentRefCube and currentInstancedMesh or ensure
currentInstancedMesh is also set appropriately in the catch block so that both
variables are consistently initialized.
experiments/three.ts
Outdated
| renderer.domElement.addEventListener('mousedown', (event) => { | ||
| mouseDown = true | ||
| mouseX = event.clientX | ||
| mouseY = event.clientY | ||
| }) | ||
|
|
||
| renderer.domElement.addEventListener('mousemove', (event) => { | ||
| if (!mouseDown) return | ||
|
|
||
| const deltaX = event.clientX - mouseX | ||
| const deltaY = event.clientY - mouseY | ||
|
|
||
| // Rotate camera around the cube | ||
| const spherical = new THREE.Spherical() | ||
| spherical.setFromVector3(camera.position) | ||
| spherical.theta -= deltaX * 0.01 | ||
| spherical.phi += deltaY * 0.01 | ||
| spherical.phi = Math.max(0.1, Math.min(Math.PI - 0.1, spherical.phi)) | ||
|
|
||
| camera.position.setFromSpherical(spherical) | ||
| camera.lookAt(0, 0, 0) | ||
|
|
||
| mouseX = event.clientX | ||
| mouseY = event.clientY | ||
|
|
||
| render() | ||
| }) | ||
|
|
||
| renderer.domElement.addEventListener('mouseup', () => { | ||
| mouseDown = false | ||
| }) | ||
|
|
||
| // Add button to recreate blocks (for testing) | ||
| const button = document.createElement('button') | ||
| button.textContent = 'Recreate Blocks' | ||
| button.style.position = 'fixed' | ||
| button.style.top = '10px' | ||
| button.style.left = '10px' | ||
| button.addEventListener('click', () => { | ||
| createInstancedBlock() | ||
| render() | ||
| }) | ||
| document.body.appendChild(button) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add cleanup for event listeners and DOM elements.
The mouse event listeners and button element are never cleaned up, which could cause memory leaks if this code is used in a single-page application.
Add a cleanup function to properly dispose resources:
+// Add cleanup function
+function cleanup() {
+ // Remove event listeners
+ renderer.domElement.removeEventListener('mousedown', handleMouseDown)
+ renderer.domElement.removeEventListener('mousemove', handleMouseMove)
+ renderer.domElement.removeEventListener('mouseup', handleMouseUp)
+
+ // Remove button
+ button.remove()
+
+ // Dispose Three.js resources
+ currentInstancedMesh?.geometry.dispose()
+ currentRefCube?.geometry.dispose()
+ sharedMaterial.dispose()
+ renderer.dispose()
+}
+
+// Store references to event handlers for removal
+const handleMouseDown = (event) => {
mouseDown = true
mouseX = event.clientX
mouseY = event.clientY
-})
+}
-renderer.domElement.addEventListener('mousemove', (event) => {
+const handleMouseMove = (event) => {
if (!mouseDown) return
// ... rest of the code
-})
+}
-renderer.domElement.addEventListener('mouseup', () => {
+const handleMouseUp = () => {
mouseDown = false
-})
+}
+// Add event listeners with named functions
+renderer.domElement.addEventListener('mousedown', handleMouseDown)
+renderer.domElement.addEventListener('mousemove', handleMouseMove)
+renderer.domElement.addEventListener('mouseup', handleMouseUp)
+// Handle page unload
+window.addEventListener('beforeunload', cleanup)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In experiments/three.ts around lines 156 to 198, the mouse event listeners on
renderer.domElement and the dynamically created button are not removed, which
can cause memory leaks in single-page applications. Add a cleanup function that
removes the 'mousedown', 'mousemove', and 'mouseup' event listeners from
renderer.domElement and removes the button element from the DOM. Ensure this
cleanup function is called appropriately when the component or scene is disposed
or no longer needed.
| private updateWorldOrigin (pos: THREE.Vector3) { | ||
| // this.worldOffset.copy(pos) | ||
| // this.scene.position.copy(this.worldOffset).multiplyScalar(-1) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove or implement the updateWorldOrigin method.
The method contains only commented-out code, suggesting an incomplete implementation. Either implement the world origin update functionality or remove this method entirely.
If this feature is planned for future implementation, add a TODO comment explaining the intended functionality:
private updateWorldOrigin (pos: THREE.Vector3) {
+ // TODO: Implement world origin shifting to prevent floating point precision issues
+ // at large distances from origin. This would involve:
+ // 1. Updating this.worldOffset
+ // 2. Shifting scene position to compensate
+ // For now, this is disabled pending further testing
// this.worldOffset.copy(pos)
// this.scene.position.copy(this.worldOffset).multiplyScalar(-1)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private updateWorldOrigin (pos: THREE.Vector3) { | |
| // this.worldOffset.copy(pos) | |
| // this.scene.position.copy(this.worldOffset).multiplyScalar(-1) | |
| } | |
| private updateWorldOrigin (pos: THREE.Vector3) { | |
| // TODO: Implement world origin shifting to prevent floating point precision issues | |
| // at large distances from origin. This would involve: | |
| // 1. Updating this.worldOffset | |
| // 2. Shifting scene position to compensate | |
| // For now, this is disabled pending further testing | |
| // this.worldOffset.copy(pos) | |
| // this.scene.position.copy(this.worldOffset).multiplyScalar(-1) | |
| } |
🤖 Prompt for AI Agents
In renderer/viewer/three/worldrendererThree.ts around lines 127 to 130, the
updateWorldOrigin method contains only commented-out code and is effectively
unused. Either fully implement the method to update the world origin by copying
the position to worldOffset and adjusting the scene position accordingly, or
remove the method entirely if it is not needed. If you plan to implement this
feature later, add a clear TODO comment inside the method explaining the
intended functionality.
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-t512jtm48-zaro.vercel.app |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
renderer/viewer/lib/mesher/models.ts (1)
680-683: Fix missing block count increment for culled instanced blocks.When an instanced block is culled, the code skips it without incrementing
blocksCount, leading to incorrect statistics. This issue was previously flagged and is still present.Apply this fix:
if (shouldCullInstancedBlock(world, cursor, block)) { // Block is completely surrounded, skip rendering + attr.blocksCount++ continue }renderer/viewer/three/worldrendererThree.ts (1)
942-945: Add holding block cleanup to destroy method.While the new components are properly cleaned up, the holding blocks still need explicit disposal as mentioned in previous reviews.
destroy (): void { this.instancedRenderer?.destroy() this.chunkMeshManager.dispose() + this.holdingBlock?.destroy?.() + this.holdingBlockLeft?.destroy?.() super.destroy() }
🧹 Nitpick comments (2)
renderer/viewer/lib/mesher/models.ts (1)
533-538: Consider caching neighbor positions for better performance.While reusing
offsetCursoris good, callingset()repeatedly may still cause internal allocations in some Vec3 implementations. Consider pre-calculating and caching the 6 neighbor offsets.const shouldCullInstancedBlock = (world: World, cursor: Vec3, block: Block): boolean => { // Early return for blocks that should never be culled if (block.transparent) return false // Check if all 6 faces would be culled (hidden by neighbors) const cullIfIdentical = block.name.includes('glass') || block.name.includes('ice') - // Cache cursor offset to avoid creating new Vec3 instances - const offsetCursor = new Vec3(0, 0, 0) - // eslint-disable-next-line guard-for-in for (const face in elemFaces) { const { dir } = elemFaces[face] - offsetCursor.set(cursor.x + dir[0], cursor.y + dir[1], cursor.z + dir[2]) - const neighbor = world.getBlock(offsetCursor, blockProvider, {}) + const neighbor = world.getBlock(cursor.offset(dir[0], dir[1], dir[2]), blockProvider, {})renderer/viewer/three/chunkMeshManager.ts (1)
586-588: Consider making target FPS configurable.The target render time is hardcoded to 60 FPS (16.67ms). For low-end devices, users might prefer 30 FPS for stability.
Add a configurable target FPS option:
+ private targetFPS = 60 + private checkPerformance (): void { if (this.renderTimes.length < this.maxRenderTimeSamples) return const avgRenderTime = this.getAverageRenderTime() - const targetRenderTime = 16.67 // 60 FPS target (16.67ms per frame) + const targetRenderTime = 1000 / this.targetFPS const performanceThreshold = targetRenderTime * 1.5 // 25ms threshold
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
renderer/viewer/lib/mesher/models.ts(5 hunks)renderer/viewer/three/chunkMeshManager.ts(1 hunks)renderer/viewer/three/worldrendererThree.ts(27 hunks)src/optionsGuiScheme.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/optionsGuiScheme.tsx
🧰 Additional context used
🧠 Learnings (3)
renderer/viewer/lib/mesher/models.ts (5)
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : Do not use the global variable `bot` directly in any file under the `renderer/` directory or its subfolders (e.g., `renderer/viewer/three/worldrendererThree.ts`).
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, files under renderer/ directory must not access the global `bot` variable directly according to .cursor/rules/vars-usage.mdc. The updateNameTagVisibility method in renderer/viewer/three/entities.ts currently violates this rule by accessing bot.teamMap and bot.username. Team information should be passed through entity update events from the world data emitter instead.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : In renderer code, all bot/player state and events must be accessed via explicit interfaces, state managers, or passed-in objects, never by referencing `bot` directly.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : In renderer code (such as in `WorldRendererThree`), use the `playerState` property (e.g., `worldRenderer.playerState.gameMode`) to access player state. The implementation for `playerState` lives in `src/mineflayer/playerState.ts`.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Some other global variables that can be used without window prefixes are listed in src/globals.d.ts.
renderer/viewer/three/chunkMeshManager.ts (5)
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, files under renderer/ directory must not access the global `bot` variable directly according to .cursor/rules/vars-usage.mdc. The updateNameTagVisibility method in renderer/viewer/three/entities.ts currently violates this rule by accessing bot.teamMap and bot.username. Team information should be passed through entity update events from the world data emitter instead.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : In renderer code (such as in `WorldRendererThree`), use the `playerState` property (e.g., `worldRenderer.playerState.gameMode`) to access player state. The implementation for `playerState` lives in `src/mineflayer/playerState.ts`.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : Do not use the global variable `bot` directly in any file under the `renderer/` directory or its subfolders (e.g., `renderer/viewer/three/worldrendererThree.ts`).
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, team information for entities should be inlined into entity update events from the world data emitter rather than accessing the global bot object from renderer code. This maintains better separation of concerns between the data layer and renderer.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : In renderer code, all bot/player state and events must be accessed via explicit interfaces, state managers, or passed-in objects, never by referencing `bot` directly.
renderer/viewer/three/worldrendererThree.ts (6)
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : Do not use the global variable `bot` directly in any file under the `renderer/` directory or its subfolders (e.g., `renderer/viewer/three/worldrendererThree.ts`).
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : In renderer code (such as in `WorldRendererThree`), use the `playerState` property (e.g., `worldRenderer.playerState.gameMode`) to access player state. The implementation for `playerState` lives in `src/mineflayer/playerState.ts`.
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, files under renderer/ directory must not access the global `bot` variable directly according to .cursor/rules/vars-usage.mdc. The updateNameTagVisibility method in renderer/viewer/three/entities.ts currently violates this rule by accessing bot.teamMap and bot.username. Team information should be passed through entity update events from the world data emitter instead.
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, team information for entities should be inlined into entity update events from the world data emitter rather than accessing the global bot object from renderer code. This maintains better separation of concerns between the data layer and renderer.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : In renderer code, all bot/player state and events must be accessed via explicit interfaces, state managers, or passed-in objects, never by referencing `bot` directly.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Some other global variables that can be used without window prefixes are listed in src/globals.d.ts.
🧬 Code Graph Analysis (2)
renderer/viewer/lib/mesher/models.ts (1)
renderer/viewer/lib/mesher/shared.ts (1)
MesherGeometryOutput(44-70)
renderer/viewer/three/worldrendererThree.ts (4)
renderer/viewer/three/holdingBlock.ts (1)
HoldingBlock(95-533)renderer/viewer/three/instancedRenderer.ts (1)
InstancedRenderer(52-860)renderer/viewer/three/chunkMeshManager.ts (1)
ChunkMeshManager(31-633)renderer/viewer/lib/mesher/shared.ts (1)
MesherGeometryOutput(44-70)
🔇 Additional comments (1)
renderer/viewer/three/worldrendererThree.ts (1)
762-763: Good use of optional chaining for holding block rendering.The optional chaining (
?.) prevents potential runtime errors if holding blocks are not initialized, which is an improvement over the previous implementation.
| if (instancingEnabled) { | ||
| return // todo | ||
| } |
There was a problem hiding this comment.
Implement liquid rendering for instanced mode.
The early return with a TODO comment means liquids (water/lava) won't be rendered when instancing is enabled. This could lead to missing visual elements in the game world.
Would you like me to help implement liquid rendering support for instanced mode or create an issue to track this task?
🤖 Prompt for AI Agents
In renderer/viewer/lib/mesher/models.ts at lines 136 to 138, the code currently
returns early with a TODO comment when instancingEnabled is true, which prevents
liquid rendering in instanced mode. To fix this, implement the liquid rendering
logic for instanced mode instead of returning early, ensuring that water and
lava are properly rendered when instancing is enabled. If the implementation is
complex, create a detailed issue to track this task for future completion.
| getSignTexture (position: Vec3, blockEntity, isHanging, backSide = false) { | ||
| const chunk = chunkPos(position) | ||
| let textures = this.chunkTextures.get(`${chunk[0]},${chunk[1]}`) | ||
| if (!textures) { | ||
| textures = {} | ||
| this.chunkTextures.set(`${chunk[0]},${chunk[1]}`, textures) | ||
| } | ||
| const texturekey = `${position.x},${position.y},${position.z}` | ||
| // todo investigate bug and remove this so don't need to clean in section dirty | ||
| if (textures[texturekey]) return textures[texturekey] | ||
|
|
||
| const PrismarineChat = PrismarineChatLoader(this.worldRendererThree.version) | ||
| const canvas = renderSign(blockEntity, isHanging, PrismarineChat) | ||
| if (!canvas) return | ||
| const tex = new THREE.Texture(canvas) | ||
| tex.magFilter = THREE.NearestFilter | ||
| tex.minFilter = THREE.NearestFilter | ||
| tex.needsUpdate = true | ||
| textures[texturekey] = tex | ||
| return tex | ||
| } |
There was a problem hiding this comment.
Add texture cleanup when chunks are unloaded.
The chunkTextures Map caches sign textures but there's no cleanup mechanism when chunks are unloaded. This could lead to memory leaks over time.
Add a cleanup method:
cleanupChunkTextures(chunkKey: string) {
const textures = this.chunkTextures.get(chunkKey)
if (textures) {
for (const texture of Object.values(textures)) {
texture.dispose()
}
this.chunkTextures.delete(chunkKey)
}
}🤖 Prompt for AI Agents
In renderer/viewer/three/chunkMeshManager.ts around lines 718 to 738, the
chunkTextures Map caches sign textures but lacks a cleanup mechanism when chunks
are unloaded, risking memory leaks. Add a cleanupChunkTextures method that takes
a chunkKey string, retrieves the textures object from chunkTextures, disposes
each THREE.Texture in it by calling texture.dispose(), and then deletes the
chunkKey entry from chunkTextures to free memory properly.
|
/deploy |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
renderer/viewer/three/chunkMeshManager.ts (1)
722-742: Sign texture caching lacks cleanup mechanism.The
chunkTexturesMap caches sign textures but there's no cleanup mechanism when chunks are unloaded. This could lead to memory leaks over time.Based on the past review comments, this issue was already identified. A cleanup method should be added to dispose textures when chunks are unloaded.
🧹 Nitpick comments (4)
renderer/viewer/three/chunkMeshManager.ts (4)
98-198: Complex section update method needs performance consideration.The
updateSectionmethod handles mesh acquisition, geometry updates, and container management. However, there are some performance considerations:
- The method creates a new
THREE.Group()for every section update, even when updating existing sections- The bounding box and sphere are hardcoded to fixed values, which may not reflect actual geometry bounds
Consider optimizing section object reuse:
- // Create or update the section object container - sectionObject = new THREE.Group() as SectionObject - sectionObject.add(mesh) + // Create or reuse the section object container + if (!sectionObject) { + sectionObject = new THREE.Group() as SectionObject + } else { + // Clear existing mesh if present + if (sectionObject.mesh) { + sectionObject.remove(sectionObject.mesh) + } + } + sectionObject.add(mesh)
156-191: Error handling is good but could be more specific.The try-catch block properly handles errors when adding signs and heads, but the error handling could be more granular to identify specific failure points.
Consider more specific error logging:
try { // Add signs container if (Object.keys(geometryData.signs).length > 0) { // ... signs logic } // Add heads container if (Object.keys(geometryData.heads).length > 0) { // ... heads logic } } catch (err) { - console.error('ChunkMeshManager: Error adding signs or heads to section', err) + console.error(`ChunkMeshManager: Error adding signs or heads to section ${sectionKey}:`, err) }
292-308: View distance calculation logic is reasonable but could be optimized.The pool sizing calculations are logical, but the
avgSectionsPerChunkconstant (5) seems arbitrary and may not reflect real-world usage patterns across different world types.Consider making the average sections per chunk configurable or calculated based on world height:
- const avgSectionsPerChunk = 5 + const avgSectionsPerChunk = Math.min(5, Math.ceil(this.worldHeight / 16 / 2))
587-613: Performance monitoring logic is sophisticated but could be more configurable.The automatic render distance adjustment based on frame time is a good optimization, but the thresholds are hardcoded. Different devices may need different performance targets.
Consider making performance thresholds configurable:
+ interface PerformanceConfig { + targetFrameTime: number + performanceThreshold: number + restorationThreshold: number + } /** * Check if performance is degraded and adjust render distance */ - private checkPerformance (): void { + private checkPerformance (config: PerformanceConfig = { + targetFrameTime: 16.67, + performanceThreshold: 1.5, + restorationThreshold: 1.1 + }): void { if (this.renderTimes.length < this.maxRenderTimeSamples) return const avgRenderTime = this.getAverageRenderTime() - const targetRenderTime = 16.67 // 60 FPS target (16.67ms per frame) - const performanceThreshold = targetRenderTime * 1.5 // 25ms threshold + const { targetFrameTime, performanceThreshold, restorationThreshold } = config + const threshold = targetFrameTime * performanceThreshold - if (avgRenderTime > performanceThreshold) { + if (avgRenderTime > threshold) { // ... rest of the logic - } else if (this.performanceOverrideDistance && avgRenderTime < targetRenderTime * 1.1) { + } else if (this.performanceOverrideDistance && avgRenderTime < targetFrameTime * restorationThreshold) { // ... rest of the logic } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
renderer/viewer/three/chunkMeshManager.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
renderer/viewer/three/chunkMeshManager.ts (5)
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, files under renderer/ directory must not access the global `bot` variable directly according to .cursor/rules/vars-usage.mdc. The updateNameTagVisibility method in renderer/viewer/three/entities.ts currently violates this rule by accessing bot.teamMap and bot.username. Team information should be passed through entity update events from the world data emitter instead.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : In renderer code (such as in `WorldRendererThree`), use the `playerState` property (e.g., `worldRenderer.playerState.gameMode`) to access player state. The implementation for `playerState` lives in `src/mineflayer/playerState.ts`.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : Do not use the global variable `bot` directly in any file under the `renderer/` directory or its subfolders (e.g., `renderer/viewer/three/worldrendererThree.ts`).
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, team information for entities should be inlined into entity update events from the world data emitter rather than accessing the global bot object from renderer code. This maintains better separation of concerns between the data layer and renderer.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-02T02:50:46.443Z
Learning: Applies to renderer/**/*.ts : In renderer code, all bot/player state and events must be accessed via explicit interfaces, state managers, or passed-in objects, never by referencing `bot` directly.
🔇 Additional comments (8)
renderer/viewer/three/chunkMeshManager.ts (8)
13-29: Well-structured interfaces for chunk mesh management.The interfaces
ChunkMeshPoolandSectionObjectare well-designed with appropriate properties for mesh pooling and section management. The optional properties and clear typing support the pooling strategy effectively.
31-73: Constructor design looks solid with proper dependency injection.The constructor properly accepts dependencies and initializes the mesh manager. The performance monitoring properties are well-initialized. The use of the
SignHeadsRendererhelper class promotes good separation of concerns.
75-93: Initial pool creation is efficient and properly configured.The pool initialization correctly creates meshes with appropriate settings (
visible: false,matrixAutoUpdate: false) for performance. The decision to not add meshes to the scene initially is correct since they'll be added to containers later.
485-501: Geometry attribute update is well-optimized for performance.The method efficiently reuses existing attributes when possible and only creates new ones when necessary. The type assertion for Float32Array is appropriate here.
618-636: Section visibility update uses correct distance calculation.The visibility update method correctly uses the same hypot distance calculation as the render distance logic. The performance is good with early returns for non-override cases.
640-678: Head rendering handles security concerns with texture URLs.The
renderHeadmethod properly handles base64 decoding and applies proxy URL replacement for security. The error handling for malformed texture data is appropriate.
680-720: Sign rendering has good positioning logic but complex calculations.The sign rendering method handles different sign types (wall, hanging) with appropriate positioning and scaling. The calculations are complex but appear correct for Minecraft sign placement.
1-11: Imports and Global Access Check Passed
- No direct
botvariable references found inrenderer/viewer/three/chunkMeshManager.ts.- All imported modules align with project conventions and correctly support Three.js chunk mesh management.
You can proceed with the detailed review of the constructor, section management methods, pool management, private helpers, and the
SignHeadsRendererclass.
| // Set bounding box and sphere for the 16x16x16 section | ||
| mesh.geometry.boundingBox = new THREE.Box3( | ||
| new THREE.Vector3(-8, -8, -8), | ||
| new THREE.Vector3(8, 8, 8) | ||
| ) | ||
| mesh.geometry.boundingSphere = new THREE.Sphere( | ||
| new THREE.Vector3(0, 0, 0), | ||
| Math.sqrt(3 * 8 ** 2) | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Hardcoded bounding volumes may cause culling issues.
The bounding box and sphere use fixed dimensions (-8 to 8) regardless of the actual geometry content. This could lead to incorrect frustum culling if sections have different actual bounds.
Consider calculating bounds from actual geometry or making them configurable:
- // Set bounding box and sphere for the 16x16x16 section
- mesh.geometry.boundingBox = new THREE.Box3(
- new THREE.Vector3(-8, -8, -8),
- new THREE.Vector3(8, 8, 8)
- )
- mesh.geometry.boundingSphere = new THREE.Sphere(
- new THREE.Vector3(0, 0, 0),
- Math.sqrt(3 * 8 ** 2)
- )
+ // Calculate bounds from actual geometry for accurate culling
+ mesh.geometry.computeBoundingBox()
+ mesh.geometry.computeBoundingSphere()🤖 Prompt for AI Agents
In renderer/viewer/three/chunkMeshManager.ts around lines 129 to 137, the
bounding box and sphere are hardcoded with fixed dimensions, which can cause
incorrect frustum culling if the geometry size varies. To fix this, compute the
bounding box and bounding sphere dynamically from the actual geometry data using
appropriate THREE.js methods like computeBoundingBox and computeBoundingSphere,
or make the bounding volumes configurable based on the geometry's real
dimensions.
| private acquireMesh (): ChunkMeshPool | undefined { | ||
| if (this.bypassPooling) { | ||
| return { | ||
| mesh: new THREE.Mesh(new THREE.BufferGeometry(), this.material), | ||
| inUse: true, | ||
| lastUsedTime: performance.now() | ||
| } | ||
| } | ||
|
|
||
| // Find first available mesh | ||
| const availableMesh = this.meshPool.find(entry => !entry.inUse) | ||
|
|
||
| if (availableMesh) { | ||
| availableMesh.inUse = true | ||
| this.hits++ | ||
| return availableMesh | ||
| } | ||
|
|
||
| // No available mesh, expand pool to accommodate new sections | ||
| let newPoolSize = Math.min(this.poolSize + 16, this.maxPoolSize) | ||
| if (newPoolSize === this.poolSize) { | ||
| newPoolSize = this.poolSize + 8 | ||
| this.maxPoolSize = newPoolSize | ||
| console.warn(`ChunkMeshManager: Pool exhausted (${this.poolSize}/${this.maxPoolSize}). Emergency expansion to ${newPoolSize}`) | ||
| } | ||
|
|
||
| this.misses++ | ||
| this.expandPool(newPoolSize) | ||
| return this.acquireMesh() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mesh acquisition logic handles pool expansion well but recursive call could cause issues.
The acquireMesh method properly handles pool expansion, but the recursive call at the end could potentially cause stack overflow in extreme scenarios where pool expansion repeatedly fails.
Replace recursive call with iterative approach:
private acquireMesh (): ChunkMeshPool | undefined {
+ let attempts = 0
+ const maxAttempts = 3
+
+ while (attempts < maxAttempts) {
if (this.bypassPooling) {
return {
mesh: new THREE.Mesh(new THREE.BufferGeometry(), this.material),
inUse: true,
lastUsedTime: performance.now()
}
}
// Find first available mesh
const availableMesh = this.meshPool.find(entry => !entry.inUse)
if (availableMesh) {
availableMesh.inUse = true
this.hits++
return availableMesh
}
// No available mesh, expand pool
+ attempts++
let newPoolSize = Math.min(this.poolSize + 16, this.maxPoolSize)
if (newPoolSize === this.poolSize) {
newPoolSize = this.poolSize + 8
this.maxPoolSize = newPoolSize
console.warn(`ChunkMeshManager: Pool exhausted (${this.poolSize}/${this.maxPoolSize}). Emergency expansion to ${newPoolSize}`)
}
this.misses++
this.expandPool(newPoolSize)
+ }
+
+ console.error('ChunkMeshManager: Failed to acquire mesh after maximum attempts')
+ return undefined
- return this.acquireMesh()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private acquireMesh (): ChunkMeshPool | undefined { | |
| if (this.bypassPooling) { | |
| return { | |
| mesh: new THREE.Mesh(new THREE.BufferGeometry(), this.material), | |
| inUse: true, | |
| lastUsedTime: performance.now() | |
| } | |
| } | |
| // Find first available mesh | |
| const availableMesh = this.meshPool.find(entry => !entry.inUse) | |
| if (availableMesh) { | |
| availableMesh.inUse = true | |
| this.hits++ | |
| return availableMesh | |
| } | |
| // No available mesh, expand pool to accommodate new sections | |
| let newPoolSize = Math.min(this.poolSize + 16, this.maxPoolSize) | |
| if (newPoolSize === this.poolSize) { | |
| newPoolSize = this.poolSize + 8 | |
| this.maxPoolSize = newPoolSize | |
| console.warn(`ChunkMeshManager: Pool exhausted (${this.poolSize}/${this.maxPoolSize}). Emergency expansion to ${newPoolSize}`) | |
| } | |
| this.misses++ | |
| this.expandPool(newPoolSize) | |
| return this.acquireMesh() | |
| } | |
| private acquireMesh(): ChunkMeshPool | undefined { | |
| let attempts = 0 | |
| const maxAttempts = 3 | |
| while (attempts < maxAttempts) { | |
| if (this.bypassPooling) { | |
| return { | |
| mesh: new THREE.Mesh(new THREE.BufferGeometry(), this.material), | |
| inUse: true, | |
| lastUsedTime: performance.now() | |
| } | |
| } | |
| // Find first available mesh | |
| const availableMesh = this.meshPool.find(entry => !entry.inUse) | |
| if (availableMesh) { | |
| availableMesh.inUse = true | |
| this.hits++ | |
| return availableMesh | |
| } | |
| // No available mesh, expand pool to accommodate new sections | |
| attempts++ | |
| let newPoolSize = Math.min(this.poolSize + 16, this.maxPoolSize) | |
| if (newPoolSize === this.poolSize) { | |
| newPoolSize = this.poolSize + 8 | |
| this.maxPoolSize = newPoolSize | |
| console.warn(`ChunkMeshManager: Pool exhausted (${this.poolSize}/${this.maxPoolSize}). Emergency expansion to ${newPoolSize}`) | |
| } | |
| this.misses++ | |
| this.expandPool(newPoolSize) | |
| } | |
| console.error('ChunkMeshManager: Failed to acquire mesh after maximum attempts') | |
| return undefined | |
| } |
🤖 Prompt for AI Agents
In renderer/viewer/three/chunkMeshManager.ts around lines 429 to 458, the
acquireMesh method uses recursion to retry mesh acquisition after expanding the
pool, which risks stack overflow if expansion repeatedly fails. Refactor this
method to use a loop instead of recursion: attempt to find or create a mesh
inside a loop that continues until a mesh is acquired or no further expansion is
possible, thereby eliminating the recursive call and preventing potential stack
overflow.
| private clearGeometry (geometry: THREE.BufferGeometry) { | ||
| // Clear attributes but keep the attribute objects for reuse | ||
| const attributes = ['position', 'normal', 'color', 'uv'] | ||
| for (const name of attributes) { | ||
| const attr = geometry.getAttribute(name) | ||
| if (attr) { | ||
| // Just mark as needing update but don't dispose to avoid recreation costs | ||
| attr.needsUpdate = true | ||
| } | ||
| } | ||
|
|
||
| if (geometry.index) { | ||
| geometry.index.needsUpdate = true | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Geometry clearing strategy may not be optimal.
The clearGeometry method only marks attributes as needing update but doesn't actually clear the data. This may not free memory as intended and could leave stale data.
Consider actually clearing the data or at least zeroing arrays:
private clearGeometry (geometry: THREE.BufferGeometry) {
- // Clear attributes but keep the attribute objects for reuse
const attributes = ['position', 'normal', 'color', 'uv']
for (const name of attributes) {
const attr = geometry.getAttribute(name)
if (attr) {
- // Just mark as needing update but don't dispose to avoid recreation costs
+ // Zero out array data to free memory while keeping attribute structure
+ attr.array.fill(0)
attr.needsUpdate = true
}
}
if (geometry.index) {
+ if (geometry.index.array) {
+ geometry.index.array.fill(0)
+ }
geometry.index.needsUpdate = true
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private clearGeometry (geometry: THREE.BufferGeometry) { | |
| // Clear attributes but keep the attribute objects for reuse | |
| const attributes = ['position', 'normal', 'color', 'uv'] | |
| for (const name of attributes) { | |
| const attr = geometry.getAttribute(name) | |
| if (attr) { | |
| // Just mark as needing update but don't dispose to avoid recreation costs | |
| attr.needsUpdate = true | |
| } | |
| } | |
| if (geometry.index) { | |
| geometry.index.needsUpdate = true | |
| } | |
| } | |
| private clearGeometry (geometry: THREE.BufferGeometry) { | |
| const attributes = ['position', 'normal', 'color', 'uv'] | |
| for (const name of attributes) { | |
| const attr = geometry.getAttribute(name) | |
| if (attr) { | |
| // Zero out array data to free memory while keeping attribute structure | |
| attr.array.fill(0) | |
| attr.needsUpdate = true | |
| } | |
| } | |
| if (geometry.index) { | |
| if (geometry.index.array) { | |
| geometry.index.array.fill(0) | |
| } | |
| geometry.index.needsUpdate = true | |
| } | |
| } |
🤖 Prompt for AI Agents
In renderer/viewer/three/chunkMeshManager.ts around lines 503 to 517, the
clearGeometry method currently only marks attributes as needing update without
clearing or zeroing their data, which may leave stale data and not free memory
as intended. Modify the method to explicitly clear or zero out the attribute
arrays (e.g., setting their array contents to zero) to ensure the geometry data
is properly cleared and memory is freed or reused safely.
|
Deployed to Vercel Preview: https://prismarine-1jbmcihv1-zaro.vercel.app |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
experiments/three.ts (1)
253-303: Mouse controls well-implemented, but cleanup still needed.The spherical coordinate-based camera controls are smooth and well-constrained. However, the memory leak concerns from previous reviews remain unaddressed.
The event listeners and button element still need cleanup to prevent memory leaks, as mentioned in previous reviews.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
experiments/three.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: in the minecraft-web-client project, files under renderer/ directory must not access the global `bot...
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, files under renderer/ directory must not access the global `bot` variable directly according to .cursor/rules/vars-usage.mdc. The updateNameTagVisibility method in renderer/viewer/three/entities.ts currently violates this rule by accessing bot.teamMap and bot.username. Team information should be passed through entity update events from the world data emitter instead.
Applied to files:
experiments/three.ts
📚 Learning: applies to renderer/**/*.ts : in renderer code (such as in `worldrendererthree`), use the `playersta...
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-27T13:24:58.174Z
Learning: Applies to renderer/**/*.ts : In renderer code (such as in `WorldRendererThree`), use the `playerState` property (e.g., `worldRenderer.playerState.gameMode`) to access player state. The implementation for `playerState` lives in `src/mineflayer/playerState.ts`.
Applied to files:
experiments/three.ts
📚 Learning: applies to renderer/**/*.ts : do not use the global variable `bot` directly in any file under the `r...
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-27T13:24:58.174Z
Learning: Applies to renderer/**/*.ts : Do not use the global variable `bot` directly in any file under the `renderer/` directory or its subfolders (e.g., `renderer/viewer/three/worldrendererThree.ts`).
Applied to files:
experiments/three.ts
📚 Learning: in the minecraft-web-client project, team information for entities should be inlined into entity upd...
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, team information for entities should be inlined into entity update events from the world data emitter rather than accessing the global bot object from renderer code. This maintains better separation of concerns between the data layer and renderer.
Applied to files:
experiments/three.ts
📚 Learning: applies to renderer/**/*.ts : in renderer code, all bot/player state and events must be accessed via...
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-27T13:24:58.174Z
Learning: Applies to renderer/**/*.ts : In renderer code, all bot/player state and events must be accessed via explicit interfaces, state managers, or passed-in objects, never by referencing `bot` directly.
Applied to files:
experiments/three.ts
📚 Learning: applies to src/**/*.ts : in `src/` code, you may use the global variable `appviewer` from `src/appvi...
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-27T13:24:58.174Z
Learning: Applies to src/**/*.ts : In `src/` code, you may use the global variable `appViewer` from `src/appViewer.ts` directly. Do not import `appViewer` or use `window.appViewer`; use the global `appViewer` variable as-is.
Applied to files:
experiments/three.ts
🔇 Additional comments (5)
experiments/three.ts (5)
1-31: LGTM! Well-structured Three.js scene setup.The imports, scene configuration, lighting setup, and visual aids (grid helper) are properly implemented and follow Three.js best practices.
32-39: LGTM! Appropriate material configuration for block rendering.The shared material setup with vertex colors, transparency, and alpha test is well-suited for Minecraft-style block rendering.
126-165: LGTM! Proper texture atlas handling.The texture loading, pixel art configuration, and UV coordinate calculation for the atlas are correctly implemented. The mock texture object structure appropriately matches the renderer's expectations.
166-228: LGTM! Proper instanced mesh implementation.The block model creation, instanced mesh setup, and positioning logic are well-implemented. The positioning strategy for the half block (line 218) to align tops with full blocks is particularly thoughtful.
229-241: LGTM! Improved error handling with visible fallback.The error handling creates a clearly identifiable fallback (red wireframe cube) that aids in debugging while preventing the application from breaking.
| uv: [0, 0, 16, 8] // Half height UV | ||
| }, | ||
| south: { | ||
| texture: textureObj, | ||
| uv: [0, 0, 16, 8] // Half height UV | ||
| }, | ||
| east: { | ||
| texture: textureObj, | ||
| uv: [0, 0, 16, 8] // Half height UV | ||
| }, | ||
| west: { | ||
| texture: textureObj, | ||
| uv: [0, 0, 16, 8] // Half height UV |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Correct UV mapping for half block side faces.
The UV coordinates for the north, south, east, and west faces use [0, 0, 16, 8] which assumes the texture starts at the bottom. However, since texture.flipY = false is set (line 134), textures are not flipped, so the UV should likely be [0, 8, 16, 16] to show the top half of the texture.
Apply this diff to correct the UV mapping:
north: {
texture: textureObj,
- uv: [0, 0, 16, 8] // Half height UV
+ uv: [0, 8, 16, 16] // Top half of texture
},
south: {
texture: textureObj,
- uv: [0, 0, 16, 8] // Half height UV
+ uv: [0, 8, 16, 16] // Top half of texture
},
east: {
texture: textureObj,
- uv: [0, 0, 16, 8] // Half height UV
+ uv: [0, 8, 16, 16] // Top half of texture
},
west: {
texture: textureObj,
- uv: [0, 0, 16, 8] // Half height UV
+ uv: [0, 8, 16, 16] // Top half of texture
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uv: [0, 0, 16, 8] // Half height UV | |
| }, | |
| south: { | |
| texture: textureObj, | |
| uv: [0, 0, 16, 8] // Half height UV | |
| }, | |
| east: { | |
| texture: textureObj, | |
| uv: [0, 0, 16, 8] // Half height UV | |
| }, | |
| west: { | |
| texture: textureObj, | |
| uv: [0, 0, 16, 8] // Half height UV | |
| north: { | |
| texture: textureObj, | |
| uv: [0, 8, 16, 16] // Top half of texture | |
| }, | |
| south: { | |
| texture: textureObj, | |
| uv: [0, 8, 16, 16] // Top half of texture | |
| }, | |
| east: { | |
| texture: textureObj, | |
| uv: [0, 8, 16, 16] // Top half of texture | |
| }, | |
| west: { | |
| texture: textureObj, | |
| uv: [0, 8, 16, 16] // Top half of texture | |
| } |
🤖 Prompt for AI Agents
In experiments/three.ts around lines 92 to 104, the UV coordinates for the
north, south, east, and west faces are set to [0, 0, 16, 8], which incorrectly
maps the bottom half of the texture. Since texture.flipY is false (line 134),
update these UV coordinates to [0, 8, 16, 16] to correctly map the top half of
the texture on these side faces.
|
/deploy |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/customChannels.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.ts
📄 CodeRabbit Inference Engine (.cursor/rules/vars-usage.mdc)
src/**/*.ts: You may use the global variablebotdirectly in any file under thesrc/directory (e.g.,src/mineflayer/playerState.ts).
Insrc/code, you may use the global variableappViewerfromsrc/appViewer.tsdirectly. Do not importappVieweror usewindow.appViewer; use the globalappViewervariable as-is.
Files:
src/customChannels.ts
🧠 Learnings (3)
📚 Learning: applies to src/**/*.ts : you may use the global variable `bot` directly in any file under the `src/`...
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-27T13:24:58.186Z
Learning: Applies to src/**/*.ts : You may use the global variable `bot` directly in any file under the `src/` directory (e.g., `src/mineflayer/playerState.ts`).
Applied to files:
src/customChannels.ts
📚 Learning: in the minecraft-web-client project, files under renderer/ directory must not access the global `bot...
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, files under renderer/ directory must not access the global `bot` variable directly according to .cursor/rules/vars-usage.mdc. The updateNameTagVisibility method in renderer/viewer/three/entities.ts currently violates this rule by accessing bot.teamMap and bot.username. Team information should be passed through entity update events from the world data emitter instead.
Applied to files:
src/customChannels.ts
📚 Learning: the mineflayer `teammemberadded` event is emitted with only one argument: the team object, not with ...
Learnt from: Phoenix616
PR: zardoy/minecraft-web-client#373
File: src/entities.ts:192-199
Timestamp: 2025-06-23T17:04:53.409Z
Learning: The mineflayer `teamMemberAdded` event is emitted with only one argument: the team object, not with both team and username parameters.
Applied to files:
src/customChannels.ts
🔇 Additional comments (1)
src/customChannels.ts (1)
18-18: LGTM: Proper integration with existing channel registration pattern.The function call is appropriately placed after other channel registrations during the
mineflayerBotCreatedevent, maintaining consistency with the established pattern.
| const sendTokenChannel = () => { | ||
| const CHANNEL_NAME = 'minecraft-web-client:token' | ||
| bot._client.registerChannel(CHANNEL_NAME, [ | ||
| 'container', | ||
| [ | ||
| { | ||
| name: 'token', | ||
| type: ['pstring', { countType: 'i16' }] | ||
| }, | ||
| { | ||
| name: 'userId', | ||
| type: ['pstring', { countType: 'i16' }] | ||
| }, | ||
| ] | ||
| ], true) | ||
| bot._client.writeChannel(CHANNEL_NAME, { | ||
| token: new URLSearchParams(window.location.search).get('token') ?? '', | ||
| userId: new URLSearchParams(window.location.search).get('userId') ?? '' | ||
| }) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Consider security implications and add error handling for token transmission.
The implementation correctly follows the established pattern for custom channels, but there are several considerations:
-
Security: Transmitting tokens via URL parameters and custom channels may expose sensitive authentication data. Consider if this is the intended behavior for your use case.
-
Error handling: The function lacks error handling for potential failures during channel registration or packet transmission.
-
Code structure: The implementation is consistent with other channel registration patterns in the file.
Consider adding basic error handling:
const sendTokenChannel = () => {
const CHANNEL_NAME = 'minecraft-web-client:token'
- bot._client.registerChannel(CHANNEL_NAME, [
- 'container',
- [
- {
- name: 'token',
- type: ['pstring', { countType: 'i16' }]
- },
- {
- name: 'userId',
- type: ['pstring', { countType: 'i16' }]
- },
- ]
- ], true)
- bot._client.writeChannel(CHANNEL_NAME, {
- token: new URLSearchParams(window.location.search).get('token') ?? '',
- userId: new URLSearchParams(window.location.search).get('userId') ?? ''
- })
+ try {
+ bot._client.registerChannel(CHANNEL_NAME, [
+ 'container',
+ [
+ {
+ name: 'token',
+ type: ['pstring', { countType: 'i16' }]
+ },
+ {
+ name: 'userId',
+ type: ['pstring', { countType: 'i16' }]
+ },
+ ]
+ ], true)
+ bot._client.writeChannel(CHANNEL_NAME, {
+ token: new URLSearchParams(window.location.search).get('token') ?? '',
+ userId: new URLSearchParams(window.location.search).get('userId') ?? ''
+ })
+ console.debug(`registered custom channel ${CHANNEL_NAME} channel`)
+ } catch (error) {
+ console.error(`Failed to register or send data on ${CHANNEL_NAME} channel:`, error)
+ }
}Please ensure that transmitting authentication tokens via custom channels aligns with your security requirements and that the server properly validates these tokens.
Add error handling and validate tokens in sendTokenChannel
The function currently assumes that channel registration and packet transmission always succeed, and that URL-parameters for token/userId are present and valid. To prevent uncaught errors and avoid sending empty or malformed credentials, please:
- Wrap the calls to
bot._client.registerChannelandbot._client.writeChannelin atry/catchblock. - Verify that
tokenanduserIdare non-empty (and match your expected format) before attempting to write to the channel. - Log (or surface) both successes and failures so you can monitor when registration or transmission fails.
Suggested refactor:
const sendTokenChannel = () => {
const CHANNEL_NAME = 'minecraft-web-client:token'
- bot._client.registerChannel(CHANNEL_NAME, [
+ const token = new URLSearchParams(window.location.search).get('token') ?? ''
+ const userId = new URLSearchParams(window.location.search).get('userId') ?? ''
+
+ if (!token || !userId) {
+ console.warn(`sendTokenChannel aborted: missing token or userId (token="${token}", userId="${userId}")`)
+ return
+ }
+
+ try {
+ bot._client.registerChannel(
+ CHANNEL_NAME,
+ [
+ 'container',
+ [
+ { name: 'token', type: ['pstring', { countType: 'i16' }] },
+ { name: 'userId', type: ['pstring', { countType: 'i16' }] },
+ ]
+ ],
+ true
+ )
+ bot._client.writeChannel(CHANNEL_NAME, { token, userId })
+ console.debug(`Custom channel "${CHANNEL_NAME}" registered and payload sent`)
+ } catch (error) {
+ console.error(`Failed to register/send on "${CHANNEL_NAME}":`, error)
+ }
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/customChannels.ts around lines 22 to 41, the sendTokenChannel function
lacks error handling and validation for token and userId parameters. Wrap the
calls to bot._client.registerChannel and bot._client.writeChannel in a try/catch
block to catch and log any errors. Before writing to the channel, validate that
token and userId are non-empty and conform to expected formats, skipping the
write if validation fails. Add logging for both successful
registration/transmission and any failures to aid monitoring.
|
Deployed to Vercel Preview: https://prismarine-nntqou0ln-zaro.vercel.app |
…ormance testing
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Chores