feat: Server + Client Light Engine! #304
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
WalkthroughThis update introduces a new hybrid lighting system combining server-side parsing with a client-side worker-based lighting engine. It enhances world data handling, mesher logic, and rendering to support dynamic lighting strategies, skylight tracking, and debugging. Configuration options are migrated and centralized for flexible lighting control. Worker initialization and messaging are refactored to integrate lighting data. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AppViewer
participant WorldDataEmitter
participant LightEngine (Worker)
participant MesherWorker
User->>AppViewer: Change lighting option / join world
AppViewer->>WorldDataEmitter: Initialize world view with config
WorldDataEmitter->>LightEngine (Worker): createLightEngineIfNeededNew()
LightEngine (Worker)-->>WorldDataEmitter: Light engine ready
loop On chunk/block update
WorldDataEmitter->>LightEngine (Worker): processLightChunk() / updateBlockLight()
LightEngine (Worker)-->>WorldDataEmitter: Lighting update result
WorldDataEmitter->>MesherWorker: Send chunk + lightData
MesherWorker-->>AppViewer: Render updated section with lighting
end
Assessment against linked issues
Suggested labels
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-8lcpkaej0-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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
package.json(1 hunks)renderer/viewer/lib/lightEngine.ts(1 hunks)renderer/viewer/lib/worldDataEmitter.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (7)
renderer/viewer/lib/lightEngine.ts (2)
1-8: Good implementation of light engine module with proper error handlingThe module correctly imports required dependencies and implements a proper singleton pattern with the
getLightEnginefunction that includes error handling for uninitialized state.
19-25: Well-structured chunk processing function with async supportThe
processLightChunkfunction correctly handles chunk coordinates conversion and properly uses the light engine API.renderer/viewer/lib/worldDataEmitter.ts (5)
11-11: Good modular import of light engine functionsThe light engine functions are properly imported from the new module.
55-55: Light updates correctly integrated with block state changesBlock light updates are now properly triggered whenever a block state changes, which is essential for realistic lighting in the Minecraft environment.
149-149: Light engine initialization in the world initializationThe light engine is properly initialized during the world data emitter initialization, ensuring it's available when needed.
182-186: Code readability improvement with intermediate variablesThe changes improve code readability by introducing intermediate variables
chunkXandchunkZfor chunk coordinate calculations.
190-197: Advanced light processing with related chunks updatesThis code nicely handles the cascading effect of light changes by:
- Processing the light for the current chunk
- Identifying affected neighboring chunks
- Triggering updates for already loaded affected chunks
This implementation ensures lighting consistency across chunk boundaries.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/optionsStorage.ts (1)
91-91: Enabling new lighting system by default is a significant changeChanging
newVersionsLightingfromfalsetotruemakes the new lighting engine enabled by default for all users. This aligns with the PR's purpose of introducing a new light engine feature.Consider adding a brief comment to explain what this option controls, similar to other commented options in this file.
smoothLighting: true, + /** Controls whether to use the new minecraft-lighting engine */ newVersionsLighting: true,Have you verified the performance impact of enabling this by default, especially on lower-end devices? If performance issues arise, you might consider implementing a progressive rollout strategy based on device capabilities.
renderer/viewer/lib/mesher/world.ts (2)
66-67: Consider removing commented code instead of keeping itIt appears the calculation for sky light has been intentionally commented out, likely because it's now being handled by the new light engine mentioned in the PR title. However, keeping commented code can lead to confusion and maintenance issues over time.
If the code is no longer needed due to the new light engine implementation, consider removing it entirely instead of commenting it out. If it's kept for historical or reference purposes, add a more descriptive comment explaining why it's disabled and how the functionality is now handled.
- // Math.min(skyLight, column.getSkyLight(posInChunk(pos))) + // Sky light calculation now handled by the light engine in lightEngine.ts
84-84: Address TODO comment with clear implementationThis line overrides the light level to 15 when both
isNeighboris true andresultis 2, with a TODO comment indicating this is not the intended final implementation. Now that you're implementing a proper light engine, this is a good opportunity to resolve this longstanding TODO.Consider either:
- Implementing the correct behavior based on lighting physics
- Documenting why this special case exists and what the correct implementation should be
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
package.json(1 hunks)renderer/viewer/lib/mesher/world.ts(1 hunks)src/optionsStorage.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (2)
renderer/viewer/lib/mesher/world.ts (2)
57-57: Consider how the new light engine interacts with the enableLighting configurationLine 57 shows that lighting can be completely disabled through the
enableLightingconfig option. Make sure the new light engine properly respects this configuration option and doesn't perform unnecessary calculations when lighting is disabled.
53-86:❓ Verification inconclusive
Verify that the light calculation changes work with existing code
The removal of sky light calculation from this method might affect any code that relies on this method to provide combined block and sky light calculations. The current implementation now only considers block light, which could cause rendering issues if any parts of the codebase expect the previous behavior.
Make sure that all places using
getLight()are aware of this change and that the new light engine properly integrates with the existing rendering system.
🏁 Script executed:
#!/bin/bash # Find all places where getLight is called to ensure they work with the new implementation rg "getLight\(" --type tsLength of output: 1207
Action: Confirm that the updated light calculations integrate correctly with rendering
The revised implementation of
getLight()now relies solely on block light (with sky light calculation code commented out). Our search confirms that all calls togetLight()occur in the following locations:
renderer/viewer/lib/mesher/world.ts(including recursive calls for handling stairs, slabs, and glass panes)renderer/viewer/lib/mesher/models.ts(for computingbaseLight, side light, and corner light)renderer/viewer/lib/mesher/standaloneRenderer.ts(a commented-out invocation)Please ensure that:
- Consumers of
getLight()(e.g., in the mesher models) are updated to work with block-light-only values.- Rendering outcomes—especially where
baseLightis normalized by dividing by 15—are validated through integration or visual tests.- Any assumptions based on a combined skylight and block light approach are either revised or documented as intentional changes.
|
why not process lighting in WASM? |
cause I don't have wasm, no one is gonna make it for me |
maybe u could use assemblyscript which is like typescript that is compiled to wasm, would be easier than creating manual WASM code |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
renderer/viewer/lib/worldDataEmitter.ts (3)
226-226: Remove unused ESLint disable directiveThe
@typescript-eslint/await-thenablerule is disabled but never reported, suggesting the directive is unnecessary.- // eslint-disable-next-line @typescript-eslint/await-thenable -- todo allow to use async world provider but not sure if needed + // todo allow to use async world provider but not sure if needed🧰 Tools
🪛 ESLint
[error] 226-226: '@typescript-eslint/await-thenable' rule is disabled but never reported.
(eslint-comments/no-unused-disable)
229-231: Add error handling for light processingThe code doesn't currently handle potential errors from
processLightChunk. While the early return on!resultprovides some protection, explicit error handling would make the code more robust.- const result = await processLightChunk(pos.x, pos.z) - if (!result) return + let result; + try { + result = await processLightChunk(pos.x, pos.z) + if (!result) return + } catch (error) { + console.error('Error processing light for chunk at', pos.x, pos.z, error) + // Continue with chunk loading even if light processing fails + }
231-236: Consider performance optimization for multiple light updatesFor frequent updates or large worlds, processing each affected chunk individually could lead to performance issues, especially when chunks affect each other in chains.
Consider batch processing or debouncing light updates:
+ // Track chunks that need light updates to avoid duplicate processing + const pendingLightUpdates = new Set<string>() async loadChunk (pos: ChunkPos, isLightUpdate = false) { // existing code... const result = await processLightChunk(pos.x, pos.z) if (!result) return + // Batch process light updates + const chunksToUpdate = [] for (const affectedChunk of result) { if (affectedChunk.x === chunkX && affectedChunk.z === chunkZ) continue const loadedChunk = this.loadedChunks[`${affectedChunk.x},${affectedChunk.z}`] if (!loadedChunk) continue - void this.loadChunk(new Vec3(affectedChunk.x * 16, 0, affectedChunk.z * 16), true) + const chunkKey = `${affectedChunk.x},${affectedChunk.z}` + if (!pendingLightUpdates.has(chunkKey)) { + pendingLightUpdates.add(chunkKey) + chunksToUpdate.push(new Vec3(affectedChunk.x * 16, 0, affectedChunk.z * 16)) + } } + // Process batched updates after a slight delay to allow for consolidation + if (chunksToUpdate.length > 0) { + setTimeout(() => { + for (const chunkPos of chunksToUpdate) { + const key = `${Math.floor(chunkPos.x / 16)},${Math.floor(chunkPos.z / 16)}` + pendingLightUpdates.delete(key) + void this.loadChunk(chunkPos, true) + } + }, 10) // Small delay to batch updates + } // rest of the existing code... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
package.json(1 hunks)renderer/viewer/lib/worldDataEmitter.ts(4 hunks)src/optionsStorage.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/optionsStorage.ts
- package.json
🧰 Additional context used
🧬 Code Definitions (1)
renderer/viewer/lib/worldDataEmitter.ts (1)
renderer/viewer/lib/lightEngine.ts (3)
updateBlockLight(27-30)createLightEngine(10-17)processLightChunk(19-25)
🪛 ESLint
renderer/viewer/lib/worldDataEmitter.ts
[error] 226-226: '@typescript-eslint/await-thenable' rule is disabled but never reported.
(eslint-comments/no-unused-disable)
🔇 Additional comments (5)
renderer/viewer/lib/worldDataEmitter.ts (5)
13-13: Good addition of light engine importsThe import statement correctly brings in the necessary light engine functions from the new module, which will be used for initialization, block light updates, and chunk light processing.
73-73: LGTM: Block light update integrationThe call to
updateBlockLightis appropriately placed after the block state update but before emitting the event, ensuring light states are updated properly when blocks change.
181-181: LGTM: Light engine initializationThe initialization of the light engine during the world setup process is well-placed in the
initmethod, which is called only once when the world is created.
221-224: Good refactoring of chunk position calculationsThe refactoring improves code readability by using explicit intermediate variables
chunkXandchunkZfor calculations, making the code easier to understand while maintaining the same functionality.
229-236:❓ Verification inconclusive
Verify WASM potential for light processing performance
Following the PR comments discussion about using WebAssembly for light processing, it's worth exploring this option for better performance, especially given the recursive nature of light propagation.
🌐 Web query:
AssemblyScript vs TypeScript performance for computationally intensive tasks💡 Result:
AssemblyScript, a TypeScript-like language compiling to WebAssembly, shows significant performance advantages over TypeScript/JavaScript for computationally intensive tasks, particularly in numerical computing and algorithms requiring predictable execution. However, the benefits depend on code structure and data handling:
Performance Advantages
Numerical computations:
- AssemblyScript demonstrates 1.1–7.2x speedups in benchmarks like matrix operations and recursive algorithms (e.g., factorial calculations)[1][3].
- Example: Factorial recursion runs 2x faster in AssemblyScript than JavaScript[3].
Predictable performance:
- WebAssembly avoids JavaScript's JIT compilation/deoptimization cycle, providing stable execution times[1][2].
- Browser engines (e.g., V8) optimize JavaScript dynamically, leading to inconsistent "fast path" performance[1][8].
Memory efficiency:
- Direct control over WebAssembly's linear memory enables optimized data layouts (e.g., 1D arrays for matrix operations)[1][3].
Task AssemblyScript Speedup vs JavaScript Matrix Multiplication 3.1–7.2x[1] Factorial (i32) 2x[3] Lower-Upper Decomposition 1.5–3x[1] Limitations and Overheads
Data transfer costs:
- Array-heavy tasks involving JS/Wasm memory copying can be slower in AssemblyScript. For example, square array operations are 2x slower due to data marshaling[3][4].
Idiomatic code pitfalls:
- TypeScript patterns like 2D nested arrays incur reference-counting penalties in AssemblyScript. Using 1D arrays improves performance[1][6].
Missing features:
- Lack of closures, regex, and GC (as of 2025) complicates porting some TypeScript logic[2][6].
Optimization Strategies
- Minimize JS/Wasm interaction:
- Batch process data within WebAssembly memory[7].
- Use manual memory management:
- Replace
Array<T>with typed buffers (e.g.,Uint8Array)[4][5].- Leverage low-level ops:
- Direct WebAssembly instructions via
unchecked()andload/store[9][10].Use Case Recommendations
Scenario Preferred Language Physics simulations AssemblyScript Image processing AssemblyScript DOM-heavy apps TypeScript Rapid prototyping TypeScript While AssemblyScript excels in raw computation, TypeScript remains preferable for tasks requiring extensive DOM manipulation or quick iteration. Future WebAssembly improvements (GC, SIMD) may expand AssemblyScript's advantages[1][8].
Citations:
- 1: https://dr.lib.iastate.edu/bitstreams/cfd12617-3042-4724-b98c-2b6a65d22279/download
- 2: https://www.fastly.com/blog/meet-assemblyscript-your-next-computing-language
- 3: https://leanylabs.com/blog/assemblyscript-intro/
- 4: https://surma.dev/things/js-to-asc/
- 5: https://tomorrowdesk.com/info/assemblyscript
- 6: https://blog.suborbital.dev/assemblyscript-vs-typescript
- 7: https://wasmbyexample.dev/examples/reading-and-writing-graphics/reading-and-writing-graphics.assemblyscript.en-us
- 8: https://www.assemblyscript.org/frequently-asked-questions.html
- 9: https://www.assemblyscript.org/introduction.html
- 10: https://www.assemblyscript.org/built-with-assemblyscript.html
Evaluate WASM (AssemblyScript) for Enhanced Light Processing Performance
The recent benchmarks comparing AssemblyScript and TypeScript indicate that AssemblyScript can yield significant speedups (often 2× or more) in computationally intensive and recursive tasks. Given that light propagation inherently involves recursion, this approach appears promising. However, note that benefits hinge on minimizing data transfer overhead—especially with array-heavy operations—and possibly optimizing data representations (e.g., using 1D typed arrays).
- File:
renderer/viewer/lib/worldDataEmitter.ts, Lines 229-236- Consider benchmarking the current approach against a WASM implementation.
- If migrating, explore optimizing data layouts to reduce JS/WASM marshaling costs.
| const result = await processLightChunk(pos.x, pos.z) | ||
| if (!result) return | ||
| for (const affectedChunk of result) { | ||
| if (affectedChunk.x === chunkX && affectedChunk.z === chunkZ) continue | ||
| const loadedChunk = this.loadedChunks[`${affectedChunk.x},${affectedChunk.z}`] | ||
| if (!loadedChunk) continue | ||
| void this.loadChunk(new Vec3(affectedChunk.x * 16, 0, affectedChunk.z * 16), true) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider adding safeguards for recursive light propagation
The implementation correctly processes light updates and propagates them to affected neighboring chunks. However, if chunks affect each other in a circular pattern, this could potentially lead to excessive recursion.
Consider adding a recursion depth counter or tracking processed chunks to prevent potential infinite loops:
async loadChunk (pos: ChunkPos, isLightUpdate = false, recursionDepth = 0) {
// existing code...
const column = await this.world.getColumnAt(pos['y'] ? pos as Vec3 : new Vec3(pos.x, 0, pos.z))
if (column) {
const result = await processLightChunk(pos.x, pos.z)
if (!result) return
+ // Prevent excessive recursion
+ if (recursionDepth >= 5) return
for (const affectedChunk of result) {
if (affectedChunk.x === chunkX && affectedChunk.z === chunkZ) continue
const loadedChunk = this.loadedChunks[`${affectedChunk.x},${affectedChunk.z}`]
if (!loadedChunk) continue
- void this.loadChunk(new Vec3(affectedChunk.x * 16, 0, affectedChunk.z * 16), true)
+ void this.loadChunk(new Vec3(affectedChunk.x * 16, 0, affectedChunk.z * 16), true, recursionDepth + 1)
}
// rest of the existing code...
}Let's verify if there are any similar safeguards in the current implementation:
🏁 Script executed:
#!/bin/bash
# Check if there's any existing recursion prevention in light processing
# Look for depth tracking or visited chunks in light-related functions
rg -A 3 -B 3 "recursion|depth|visited|processed" --glob "*.ts"
# Look for any maps/sets used to track processed chunks in light engine
ast-grep --pattern $'const $_ = new (Set|Map)<$_>'Length of output: 41150
Action Required: Add Safeguards for Recursive Light Propagation
Our verification did not uncover any existing safeguard for recursion in light processing within renderer/viewer/lib/worldDataEmitter.ts. In its current form, the logic propagates light updates by recursively calling loadChunk without any check to prevent infinite recursion when chunks create circular dependencies.
Please consider introducing a recursion depth counter (or a mechanism to track already processed chunks) to cap recursive calls. For example, a diff similar to the one suggested below would add a guard that stops further recursion after a fixed depth (e.g., 5), making the propagation robust against potential infinite loops:
async loadChunk (pos: ChunkPos, isLightUpdate = false, recursionDepth = 0) {
// existing code...
const column = await this.world.getColumnAt(pos['y'] ? pos as Vec3 : new Vec3(pos.x, 0, pos.z))
if (column) {
const result = await processLightChunk(pos.x, pos.z)
if (!result) return
+ // Prevent excessive recursion by capping the recursion depth
+ if (recursionDepth >= 5) return
for (const affectedChunk of result) {
if (affectedChunk.x === chunkX && affectedChunk.z === chunkZ) continue
const loadedChunk = this.loadedChunks[`${affectedChunk.x},${affectedChunk.z}`]
if (!loadedChunk) continue
- void this.loadChunk(new Vec3(affectedChunk.x * 16, 0, affectedChunk.z * 16), true)
+ void this.loadChunk(new Vec3(affectedChunk.x * 16, 0, affectedChunk.z * 16), true, recursionDepth + 1)
}
// rest of the existing code...
}
}Implementing this safeguard should mitigate the risk of infinite recursion during light propagation.
📝 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.
| const result = await processLightChunk(pos.x, pos.z) | |
| if (!result) return | |
| for (const affectedChunk of result) { | |
| if (affectedChunk.x === chunkX && affectedChunk.z === chunkZ) continue | |
| const loadedChunk = this.loadedChunks[`${affectedChunk.x},${affectedChunk.z}`] | |
| if (!loadedChunk) continue | |
| void this.loadChunk(new Vec3(affectedChunk.x * 16, 0, affectedChunk.z * 16), true) | |
| } | |
| async loadChunk (pos: ChunkPos, isLightUpdate = false, recursionDepth = 0) { | |
| // existing code... | |
| const column = await this.world.getColumnAt(pos['y'] ? pos as Vec3 : new Vec3(pos.x, 0, pos.z)) | |
| if (column) { | |
| const result = await processLightChunk(pos.x, pos.z) | |
| if (!result) return | |
| // Prevent excessive recursion by capping the recursion depth | |
| if (recursionDepth >= 5) return | |
| for (const affectedChunk of result) { | |
| if (affectedChunk.x === chunkX && affectedChunk.z === chunkZ) continue | |
| const loadedChunk = this.loadedChunks[`${affectedChunk.x},${affectedChunk.z}`] | |
| if (!loadedChunk) continue | |
| void this.loadChunk(new Vec3(affectedChunk.x * 16, 0, affectedChunk.z * 16), true, recursionDepth + 1) | |
| } | |
| // rest of the existing code... | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
renderer/viewer/lib/worldDataEmitter.ts (1)
217-234:⚠️ Potential issueAdd safeguards for recursive light propagation
The implementation correctly processes light updates and propagates them to affected neighboring chunks. However, if chunks affect each other in a circular pattern, this could potentially lead to excessive recursion.
Consider adding a recursion depth counter or tracking processed chunks to prevent potential infinite loops.
- async loadChunk (pos: ChunkPos, isLightUpdate = false) { + async loadChunk (pos: ChunkPos, isLightUpdate = false, recursionDepth = 0) { const [botX, botZ] = chunkPos(this.lastPos) const chunkX = Math.floor(pos.x / 16) const chunkZ = Math.floor(pos.z / 16) const dx = Math.abs(botX - chunkX) const dz = Math.abs(botZ - chunkZ) if (dx <= this.viewDistance && dz <= this.viewDistance) { // eslint-disable-next-line @typescript-eslint/await-thenable -- todo allow to use async world provider but not sure if needed const column = await this.world.getColumnAt(pos['y'] ? pos as Vec3 : new Vec3(pos.x, 0, pos.z)) if (column) { const result = await processLightChunk(pos.x, pos.z) if (!result) return + // Prevent excessive recursion + if (recursionDepth >= 5) return for (const affectedChunk of result) { if (affectedChunk.x === chunkX && affectedChunk.z === chunkZ) continue const loadedChunk = this.loadedChunks[`${affectedChunk.x},${affectedChunk.z}`] if (!loadedChunk) continue - void this.loadChunk(new Vec3(affectedChunk.x * 16, 0, affectedChunk.z * 16), true) + void this.loadChunk(new Vec3(affectedChunk.x * 16, 0, affectedChunk.z * 16), true, recursionDepth + 1) }🧰 Tools
🪛 ESLint
[error] 224-224: '@typescript-eslint/await-thenable' rule is disabled but never reported.
(eslint-comments/no-unused-disable)
renderer/viewer/lib/lightEngine.ts (1)
32-35:⚠️ Potential issueUndefined variable in updateBlockLight function
The function uses
mcDatawhich isn't imported or declared in this file.+import { mcData } from './mcData' // Adjust this to the actual location of mcData export const updateBlockLight = (x: number, y: number, z: number, stateId: number) => { const engine = getLightEngine() engine.setBlock(x, y, z, convertPrismarineBlockToWorldBlock(mcData.blocks[stateId], loadedData)) }
🧹 Nitpick comments (6)
renderer/viewer/lib/mesher/world.ts (2)
55-93: Lighting system transition implemented correctlyThe refactoring of the getLight method to use the new lighting system is well done. Using the IS_USING_SERVER_LIGHTING flag to control the code path is a clean way to transition between systems.
One suggestion: Consider making IS_USING_SERVER_LIGHTING a configurable option rather than a hardcoded constant, which would allow for easier testing and toggling between lighting systems.
- const IS_USING_SERVER_LIGHTING = false + const IS_USING_SERVER_LIGHTING = this.config.useServerLighting ?? false
95-103: Helper methods for accessing light dataThe new getBlockLight and getSkyLight methods provide a clean interface for accessing light data. Consider removing the commented-out code since it's no longer needed and can cause confusion.
getBlockLight (pos: Vec3) { return this.lightHolder.getBlockLight(pos.x, pos.y, pos.z) - // return column.getBlockLight(posInChunk(pos)) } getSkyLight (pos: Vec3) { return this.lightHolder.getSkyLight(pos.x, pos.y, pos.z) - // return column.getSkyLight(posInChunk(pos)) }renderer/viewer/lib/worldDataEmitter.ts (1)
224-224: Remove unused ESLint directiveThe static analysis hints indicate that the
@typescript-eslint/await-thenableESLint disable directive is never reported, meaning it's unnecessary. Consider removing it or actually using it if needed.- // eslint-disable-next-line @typescript-eslint/await-thenable -- todo allow to use async world provider but not sure if needed + // todo allow to use async world provider but not sure if needed🧰 Tools
🪛 ESLint
[error] 224-224: '@typescript-eslint/await-thenable' rule is disabled but never reported.
(eslint-comments/no-unused-disable)
renderer/viewer/lib/lightEngine.ts (1)
24-30: Good implementation of chunk light processingThe processLightChunk function correctly converts chunk positions and calls the appropriate method on the light engine.
Consider documenting the return value of
engine.receiveUpdateColumnsince it's being returned and used by callers.export const processLightChunk = async (x: number, z: number) => { const chunkX = Math.floor(x / 16) const chunkZ = Math.floor(z / 16) const engine = getLightEngine() // fillColumnWithZeroLight(engine.externalWorld, chunkX, chunkZ) + // Returns an array of affected chunks that need to be updated return engine.receiveUpdateColumn(chunkX, chunkZ) }renderer/viewer/lib/worldrendererCommon.ts (2)
166-166: Consider adding error handling for light engine initializationThe light engine is initialized in the constructor without any error handling. If the
createLightEnginefunction throws an exception, it could prevent theWorldRendererCommonclass from being properly instantiated.- createLightEngine(this) + try { + createLightEngine(this) + } catch (error) { + console.warn('Failed to initialize light engine:', error) + // Continue initialization without light engine + }
580-602: Consider performance optimizations for light dataBased on PR comments, there was a discussion about processing lighting in WebAssembly (WASM) for better performance. While the current TypeScript implementation works, you might want to consider future optimizations for light calculations, especially for larger worlds or lower-end devices.
The current implementation calls
getLightEngineSafe()?.worldLightHolder.dumpChunk(x, z)for each addColumn operation, which could be resource-intensive. Consider:
- Implementing a caching mechanism for light data
- Processing light updates only when necessary
- Exploring AssemblyScript as suggested in the PR comments for more performant light calculations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
package.json(1 hunks)renderer/viewer/lib/lightEngine.ts(1 hunks)renderer/viewer/lib/mesher/mesher.ts(1 hunks)renderer/viewer/lib/mesher/world.ts(4 hunks)renderer/viewer/lib/worldDataEmitter.ts(3 hunks)renderer/viewer/lib/worldrendererCommon.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧬 Code Graph Analysis (1)
renderer/viewer/lib/worldrendererCommon.ts (1)
renderer/viewer/lib/lightEngine.ts (2)
createLightEngine(14-22)getLightEngineSafe(10-12)
🪛 ESLint
renderer/viewer/lib/worldDataEmitter.ts
[error] 224-224: '@typescript-eslint/await-thenable' rule is disabled but never reported.
(eslint-comments/no-unused-disable)
🔇 Additional comments (9)
renderer/viewer/lib/mesher/mesher.ts (1)
110-112: Light data handling integration looks good!The integration of light data processing when receiving chunk messages is well implemented. This ensures that light information is properly loaded into the light holder when a chunk is received.
renderer/viewer/lib/mesher/world.ts (2)
1-1: New lighting dependency added correctlyGood job importing the WorldLightHolder from the minecraft-lighting package.
36-36: New lightHolder property initialization looks goodThe lightHolder is properly initialized with coordinates (0, 0). This will serve as the foundation for the client-side lighting system.
renderer/viewer/lib/worldDataEmitter.ts (2)
12-12: New light engine functions imported correctlyGood job importing the necessary functions from the lightEngine module.
72-72: Block light update integration looks goodAdding the updateBlockLight call ensures that lighting is updated whenever a block is changed, keeping the client-side lighting in sync with block updates.
renderer/viewer/lib/lightEngine.ts (2)
1-4: Good imports for the lighting systemThe imports for the lighting system components look good. The minecraft-lighting package provides the necessary tools for implementing lighting in the Minecraft-like environment.
5-12: Proper light engine initialization and accessThe light engine state management is well done with appropriate initialization checks and error handling.
renderer/viewer/lib/worldrendererCommon.ts (2)
22-22: Light engine integration is well-structuredThe import statement cleanly imports the necessary light engine functions from the new module, following the existing import pattern in the file.
597-598:✅ Verification successful
Verify handling of undefined light data
The code correctly uses optional chaining when retrieving light data, but there's no explicit handling for when
getLightEngineSafe()returnsundefined. Worker code should be resilient to receivingundefinedforlightData.
🏁 Script executed:
#!/bin/bash # Check if the worker code handles undefined lightData correctly # Search for lightData in the worker code rg --type ts "lightData" -A 5 -B 5Length of output: 1802
Handling of undefined light data is correctly implemented
The verification shows that the worker code (inrenderer/viewer/lib/mesher/mesher.ts) checks for the existence oflightDatabefore callingworld.lightHolder.loadChunk. This conditional (if (data.lightData) { ... }) ensures that even ifgetLightEngineSafe()returnsundefined, no unintended behavior occurs. No changes are needed.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
renderer/viewer/lib/worldDataEmitter.ts (1)
253-260:⚠️ Potential issueAdd safeguards for recursive light propagation.
The implementation correctly processes light updates and propagates them to affected neighboring chunks. However, if chunks affect each other in a circular pattern, this could potentially lead to excessive recursion.
Consider adding a recursion depth counter or tracking processed chunks to prevent potential infinite loops:
-async loadChunk (pos: ChunkPos, isLightUpdate = false) { +async loadChunk (pos: ChunkPos, isLightUpdate = false, recursionDepth = 0) { const [botX, botZ] = chunkPos(this.lastPos) const chunkX = Math.floor(pos.x / 16) const chunkZ = Math.floor(pos.z / 16) const dx = Math.abs(botX - chunkX) const dz = Math.abs(botZ - chunkZ) if (dx <= this.viewDistance && dz <= this.viewDistance) { const column = await this.world.getColumnAt(pos['y'] ? pos as Vec3 : new Vec3(pos.x, 0, pos.z)) if (column) { const result = await processLightChunk(pos.x, pos.z) if (!result) return + // Prevent excessive recursion + if (recursionDepth >= 5) return for (const affectedChunk of result) { if (affectedChunk.x === chunkX && affectedChunk.z === chunkZ) continue const loadedChunk = this.loadedChunks[`${affectedChunk.x},${affectedChunk.z}`] if (!loadedChunk) continue - void this.loadChunk(new Vec3(affectedChunk.x * 16, 0, affectedChunk.z * 16), true) + void this.loadChunk(new Vec3(affectedChunk.x * 16, 0, affectedChunk.z * 16), true, recursionDepth + 1) }
🧹 Nitpick comments (3)
renderer/viewer/lib/worldDataEmitter.ts (3)
250-250: Remove or fix the eslint-disable comment.The
@typescript-eslint/await-thenablerule is disabled but never reported in this line.Either remove the comment if it's no longer needed or replace it with a more specific explanation if there's a legitimate reason to disable this rule:
- // eslint-disable-next-line @typescript-eslint/await-thenable -- todo allow to use async world provider but not sure if needed + // World provider might be async in the future, so we use await here🧰 Tools
🪛 ESLint
[error] 250-250: '@typescript-eslint/await-thenable' rule is disabled but never reported.
(eslint-comments/no-unused-disable)
253-260: Consider tracking processed chunks with a Set.As an alternative to using a recursion depth counter, you could also track processed chunks using a Set to prevent redundant processing and potential circular dependencies.
-async loadChunk (pos: ChunkPos, isLightUpdate = false) { +async loadChunk (pos: ChunkPos, isLightUpdate = false, processedChunks = new Set<string>()) { const [botX, botZ] = chunkPos(this.lastPos) const chunkX = Math.floor(pos.x / 16) const chunkZ = Math.floor(pos.z / 16) const dx = Math.abs(botX - chunkX) const dz = Math.abs(botZ - chunkZ) if (dx <= this.viewDistance && dz <= this.viewDistance) { const column = await this.world.getColumnAt(pos['y'] ? pos as Vec3 : new Vec3(pos.x, 0, pos.z)) if (column) { + const chunkKey = `${chunkX},${chunkZ}` + if (processedChunks.has(chunkKey)) return + processedChunks.add(chunkKey) const result = await processLightChunk(pos.x, pos.z) if (!result) return for (const affectedChunk of result) { if (affectedChunk.x === chunkX && affectedChunk.z === chunkZ) continue const loadedChunk = this.loadedChunks[`${affectedChunk.x},${affectedChunk.z}`] if (!loadedChunk) continue - void this.loadChunk(new Vec3(affectedChunk.x * 16, 0, affectedChunk.z * 16), true) + void this.loadChunk(new Vec3(affectedChunk.x * 16, 0, affectedChunk.z * 16), true, processedChunks) }
253-260: Consider implementing light chunking more efficiently.The light propagation could potentially be made more efficient by batching multiple light updates instead of processing them one by one with recursive calls.
In high-performance light engines, updates are often:
- Queued in a priority data structure (based on light level)
- Processed in batches
- Propagated using iterative algorithms rather than recursive calls
This would be a future optimization if performance becomes an issue with the current implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
package.json(1 hunks)renderer/viewer/lib/worldDataEmitter.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🪛 ESLint
renderer/viewer/lib/worldDataEmitter.ts
[error] 250-250: '@typescript-eslint/await-thenable' rule is disabled but never reported.
(eslint-comments/no-unused-disable)
🔇 Additional comments (3)
renderer/viewer/lib/worldDataEmitter.ts (3)
12-12: Properly importing light engine functions.Good addition of the import for light engine functions. This sets up the integration of the new light engine with the world data emission system.
75-75: Light system integration looks good.Excellent addition of the call to
updateBlockLightwhen a block's state changes. This ensures lighting is updated whenever block states change, which is essential for maintaining consistent lighting effects in the world.
245-248: Code refactoring improves readability.Breaking down the calculation into
chunkXandchunkZvariables before computing distances makes the code more readable and maintainable. These intermediate variables are also reused later in the code, which is efficient.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
renderer/viewer/lib/worldDataEmitter.ts (1)
264-271:⚠️ Potential issueImplement safeguards for recursive light propagation.
The current implementation processes light updates and propagates them to affected neighboring chunks without any safeguards against excessive recursion. If chunks affect each other in a circular pattern, this could potentially lead to infinite recursion.
Please add a recursion depth counter to prevent potential infinite loops:
-async loadChunk (pos: ChunkPos, isLightUpdate = false, reason = 'spiral') { +async loadChunk (pos: ChunkPos, isLightUpdate = false, reason = 'spiral', recursionDepth = 0) { const [botX, botZ] = chunkPos(this.lastPos) const chunkX = Math.floor(pos.x / 16) const chunkZ = Math.floor(pos.z / 16) const dx = Math.abs(botX - chunkX) const dz = Math.abs(botZ - chunkZ) if (dx <= this.viewDistance && dz <= this.viewDistance) { // eslint-disable-next-line @typescript-eslint/await-thenable -- todo allow to use async world provider but not sure if needed const column = await this.world.getColumnAt(pos['y'] ? pos as Vec3 : new Vec3(pos.x, 0, pos.z)) if (column) { const result = await processLightChunk(pos.x, pos.z) if (!result) return + // Prevent excessive recursion + if (recursionDepth >= 5) return for (const affectedChunk of result) { if (affectedChunk.x === chunkX && affectedChunk.z === chunkZ) continue const loadedChunk = this.loadedChunks[`${affectedChunk.x},${affectedChunk.z}`] if (!loadedChunk) continue - void this.loadChunk(new Vec3(affectedChunk.x * 16, 0, affectedChunk.z * 16), true) + void this.loadChunk(new Vec3(affectedChunk.x * 16, 0, affectedChunk.z * 16), true, 'light_update', recursionDepth + 1) }
🧹 Nitpick comments (1)
renderer/viewer/lib/worldDataEmitter.ts (1)
261-261: Remove unused ESLint disable comment.The
@typescript-eslint/await-thenablerule is disabled but never reported according to the static analysis hints.- // eslint-disable-next-line @typescript-eslint/await-thenable -- todo allow to use async world provider but not sure if needed + // todo allow to use async world provider but not sure if neededOr if the rule is actually needed, consider using a more specific disable:
- // eslint-disable-next-line @typescript-eslint/await-thenable -- todo allow to use async world provider but not sure if needed + /* eslint-disable-next-line @typescript-eslint/await-thenable */ // todo allow to use async world provider but not sure if needed🧰 Tools
🪛 ESLint
[error] 261-261: '@typescript-eslint/await-thenable' rule is disabled but never reported.
(eslint-comments/no-unused-disable)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
package.json(1 hunks)renderer/viewer/lib/worldDataEmitter.ts(3 hunks)renderer/viewer/lib/worldrendererCommon.ts(3 hunks)src/optionsStorage.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/optionsStorage.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- renderer/viewer/lib/worldrendererCommon.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
renderer/viewer/lib/worldDataEmitter.ts (1)
renderer/viewer/lib/lightEngine.ts (2)
updateBlockLight(32-35)processLightChunk(24-30)
🪛 ESLint
renderer/viewer/lib/worldDataEmitter.ts
[error] 261-261: '@typescript-eslint/await-thenable' rule is disabled but never reported.
(eslint-comments/no-unused-disable)
🔇 Additional comments (4)
renderer/viewer/lib/worldDataEmitter.ts (4)
12-12: Import for light engine integration looks good.The new import statement correctly brings in the necessary functions from the light engine module for block light updates and chunk light processing.
83-83: Good integration of light updates with block state changes.Calling
updateBlockLightafter changing a block state ensures that lighting is properly updated whenever block modifications occur, maintaining visual consistency in the world.
255-260: Improved chunk coordinate handling.Extracting the chunk coordinates and distance calculations into explicit variables improves code readability and makes the logic clearer.
28-28: Verify event handler signatures align with usage.The
loadChunkevent now includes anisLightUpdateboolean parameter in the event data, which is being properly passed on line 283. This is a good addition that helps consumers distinguish between normal chunk loads and light-only updates.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
renderer/viewer/lib/lightEngine.ts (2)
39-42:⚠️ Potential issueUndefined variables in updateBlockLight function
The function uses
mcDataandloadedDatawhich aren't imported or defined in this file.+import { mcData } from './mcData' // Adjust import path as needed export const updateBlockLight = (x: number, y: number, z: number, stateId: number) => { const engine = getLightEngine() + // Get the loadedData from the appropriate source + const loadedData = engine.externalWorld.getData?.() || {} // Adjust according to the actual source engine.setBlock(x, y, z, convertPrismarineBlockToWorldBlock(mcData.blocks[stateId], loadedData)) }
15-22:⚠️ Potential issueMultiple undefined variables in createLightEngine function
The function uses
loadedDatawhich isn't imported or defined in this file.export const createLightEngine = (world: WorldRendererCommon) => { - lightEngine = createLightEngineForSyncWorld(world.displayOptions.worldView.world as unknown as world.WorldSync, loadedData, { + // Get the world and loaded data from the appropriate sources + const worldView = world.displayOptions.worldView + const loadedData = world.displayOptions.loadedData // Adjust according to the actual source + lightEngine = createLightEngineForSyncWorld(worldView.world as unknown as world.WorldSync, loadedData, { minY: world.worldSizeParams.minY, height: world.worldSizeParams.worldHeight, // enableSkyLight: false, }) lightEngine.PARALLEL_CHUNK_PROCESSING = false globalThis.lightEngine = lightEngine }renderer/viewer/three/worldrendererThree.ts (1)
157-161: Consider removing commented code instead of keeping itThe old code is commented out but kept in the file. This can lead to confusion and unnecessary clutter.
Either completely remove the commented code or add a comment explaining why it's being kept:
this.onReactiveValueUpdated('directionalLight', (value) => { if (!value) return - // this.directionalLight.intensity = value this.directionalLight.intensity = 1 })
🧹 Nitpick comments (4)
renderer/viewer/lib/lightEngine.ts (3)
6-12: Improve error handling in getLightEngine functionsThe error message in
getLightEnginecould be more descriptive. Also, consider adding documentation to clarify when to usegetLightEnginevsgetLightEngineSafe.export const getLightEngine = () => { - if (!lightEngine) throw new Error('Light engine not initialized') + if (!lightEngine) throw new Error('Light engine not initialized. Call createLightEngine first.') return lightEngine } +/** + * Returns the light engine instance or null if not initialized. + * Use this when you want to avoid throwing errors if the light engine isn't ready. + */ export const getLightEngineSafe = () => { return lightEngine }
20-21: Consider implications of setting global stateAssigning to
globalThis.lightEnginemakes it available globally, which can cause issues with testing or if multiple instances are needed.Consider whether this global assignment is necessary, or if it could be replaced with a more controlled approach like dependency injection or a singleton pattern with explicit access methods.
24-32: Improve commenting for processLightChunk functionThe commented-out line and lack of documentation make it unclear what this function does and how it should be used.
+/** + * Process lighting for a chunk and its neighbors. + * This should be called after loading a chunk or when a block changes. + * @param x - The x coordinate (block position) + * @param z - The z coordinate (block position) + * @returns A promise that resolves when the lighting update is complete + */ export const processLightChunk = async (x: number, z: number) => { const chunkX = Math.floor(x / 16) const chunkZ = Math.floor(z / 16) const engine = getLightEngine() - // fillColumnWithZeroLight(engine.externalWorld, chunkX, chunkZ) const updated = engine.receiveUpdateColumn(chunkX, chunkZ) return updated }renderer/viewer/three/worldrendererThree.ts (1)
153-156: Consider removing commented code instead of keeping itThe old code is commented out but kept in the file. This can lead to confusion and unnecessary clutter.
Either completely remove the commented code or add a comment explaining why it's being kept:
this.onReactiveValueUpdated('ambientLight', (value) => { if (!value) return - // this.ambientLight.intensity = value this.ambientLight.intensity = 1 })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
package.json(1 hunks)renderer/viewer/lib/lightEngine.ts(1 hunks)renderer/viewer/lib/mesher/mesher.ts(1 hunks)renderer/viewer/lib/mesher/world.ts(4 hunks)renderer/viewer/lib/worldrendererCommon.ts(4 hunks)renderer/viewer/three/worldrendererThree.ts(1 hunks)src/optionsStorage.ts(1 hunks)src/watchOptions.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/optionsStorage.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- package.json
- renderer/viewer/lib/mesher/mesher.ts
- renderer/viewer/lib/mesher/world.ts
- renderer/viewer/lib/worldrendererCommon.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/watchOptions.ts (3)
src/optionsStorage.ts (1)
options(225-230)src/appViewer.ts (1)
appViewer(261-261)src/customChannels.ts (1)
customEvents(6-19)
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-d3vgoa72n-zaro.vercel.app |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/mineflayer/playerState.ts (2)
27-27: The newlightingDisabledflag lacks documentation.While the property name is somewhat descriptive, adding JSDoc comments would help other developers understand its purpose, usage, and when it might be set to true.
+/** Flag indicating whether lighting effects should be disabled based on the dimension (true for Nether, End, or dimensions without skylight) */ public lightingDisabled = false
55-63: Good error handling for dimension data extraction.The
handleDimensionDatahelper function correctly handles potential missing properties in the dimension data with a try-catch block. However, the function parameter lacks type information.Consider adding TypeScript type annotations for better code documentation and IDE support:
-const handleDimensionData = (data) => { +const handleDimensionData = (data: any) => { let hasSkyLight = 1 try { hasSkyLight = data.dimension.value.has_skylight.value } catch (err) { hasSkyLight = 0 } this.lightingDisabled = bot.game.dimension === 'the_nether' || bot.game.dimension === 'the_end' || !hasSkyLight }src/react/DebugOverlay.tsx (1)
131-143: Consider extracting light fetching logic to a separate function.The conditional light value fetching logic is well-structured but could be extracted to a dedicated function to improve readability and maintainability.
+ const getLightValues = () => { + const lightingEnabled = appViewer.inWorldRenderingConfig.enableLighting + const serverLighting = !appViewer.inWorldRenderingConfig.clientSideLighting + + if (!lightingEnabled || serverLighting) { + return { + sky: bot.world.getSkyLight(bot.entity.position), + block: bot.world.getBlockLight(bot.entity.position), + info: lightingEnabled ? 'Server Lighting' : 'Lighting Disabled' + } + } else { + // client engine is used + const lights = getDebugLightValues( + Math.floor(bot.entity.position.x), + Math.floor(bot.entity.position.y), + Math.floor(bot.entity.position.z) + ) + return { + sky: lights.skyLight, + block: lights.blockLight, + info: 'Client Light Engine' + } + } + } const freqUpdateInterval = setInterval(() => { - const lightingEnabled = appViewer.inWorldRenderingConfig.enableLighting - const serverLighting = !appViewer.inWorldRenderingConfig.clientSideLighting - if (!lightingEnabled || serverLighting) { - setLightInfo({ - sky: bot.world.getSkyLight(bot.entity.position), - block: bot.world.getBlockLight(bot.entity.position), - info: lightingEnabled ? 'Server Lighting' : 'Lighting Disabled' - }) - } else { - // client engine is used - const lights = getDebugLightValues(Math.floor(bot.entity.position.x), Math.floor(bot.entity.position.y), Math.floor(bot.entity.position.z)) - setLightInfo({ sky: lights.skyLight, block: lights.blockLight, info: 'Client Light Engine' }) - } + setLightInfo(getLightValues()) setPos({ ...bot.entity.position })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
renderer/viewer/lib/basePlayerState.ts(1 hunks)renderer/viewer/lib/mesher/models.ts(5 hunks)renderer/viewer/lib/mesher/shared.ts(1 hunks)renderer/viewer/lib/mesher/world.ts(3 hunks)renderer/viewer/lib/worldrendererCommon.ts(5 hunks)src/mineflayer/playerState.ts(2 hunks)src/react/ChunksDebugScreen.tsx(1 hunks)src/react/DebugOverlay.tsx(4 hunks)src/watchOptions.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- renderer/viewer/lib/basePlayerState.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- renderer/viewer/lib/mesher/shared.ts
- src/watchOptions.ts
- renderer/viewer/lib/worldrendererCommon.ts
- renderer/viewer/lib/mesher/world.ts
- renderer/viewer/lib/mesher/models.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (5)
src/react/ChunksDebugScreen.tsx (1)
33-33: Good use of optional chaining for error prevention.Using optional chaining (
?.) here provides a defensive approach to prevent potential runtime errors ifchunk.loadsis undefined. This is particularly important in debug UI that needs to be resilient even when chunk data might be partially loaded or in transition states.src/mineflayer/playerState.ts (1)
65-70: Good reuse of helper function.Using the same
handleDimensionDatafunction for both packet handlers reduces code duplication and ensures consistent behavior. This is a good practice.src/react/DebugOverlay.tsx (3)
4-4: Added import for the new light engine.This import correctly brings in the
getDebugLightValuesfunction from the new light engine module.
34-34: Improved state management with combined light information.Consolidating the separate sky and block light states into a single
lightInfoobject with additional metadata is a good design choice that makes the component's state management more maintainable.
204-204: Updated UI to display combined light information.The UI now displays the combined light values and source information, making the debug overlay more informative for developers.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
renderer/viewer/lib/worldDataEmitter.ts (1)
310-320:⚠️ Potential issueAdd recursion depth limit to prevent stack overflow.
The current implementation recursively loads affected chunks without any depth limit. This could potentially lead to stack overflow or excessive recursion if chunks affect each other in a circular pattern.
Add a recursion depth parameter and limit:
-async loadChunk (pos: ChunkPos, isLightUpdate = false, reason = 'spiral') { +async loadChunk (pos: ChunkPos, isLightUpdate = false, reason = 'spiral', recursionDepth = 0) { createLightEngineIfNeeded(this) const [botX, botZ] = chunkPos(this.lastPos) const chunkX = Math.floor(pos.x / 16) const chunkZ = Math.floor(pos.z / 16) const dx = Math.abs(botX - chunkX) const dz = Math.abs(botZ - chunkZ) if (dx <= this.viewDistance && dz <= this.viewDistance) { const column = await this.world.getColumnAt(pos['y'] ? pos as Vec3 : new Vec3(pos.x, 0, pos.z)) if (column) { let result = [] as Array<{ x: number, z: number }> if (!isLightUpdate && this.worldRendererConfig.clientSideLighting) { result = await processLightChunk(pos.x, pos.z) ?? [] } if (!result) return + // Prevent excessive recursion + if (recursionDepth >= 5) return for (const affectedChunk of result) { if (affectedChunk.x === chunkX && affectedChunk.z === chunkZ) continue const loadedChunk = this.loadedChunks[`${affectedChunk.x},${affectedChunk.z}`] if (!loadedChunk) continue - void this.loadChunk(new Vec3(affectedChunk.x * 16, 0, affectedChunk.z * 16), true) + void this.loadChunk(new Vec3(affectedChunk.x * 16, 0, affectedChunk.z * 16), true, reason, recursionDepth + 1) }renderer/viewer/lib/lightEngine.ts (1)
52-66:⚠️ Potential issueSame undefined 'loadedData' variable in updateBlockLight function.
The function uses the same undefined
loadedDatavariable as seen in createLightEngineIfNeeded. Additionally, the function is passing the stateId directly to convertPrismarineBlockToWorldBlock, which might expect a full block object rather than just an ID.Fix the loadedData reference and handle the block conversion properly:
export const updateBlockLight = (x: number, y: number, z: number, stateId: number) => { const engine = getLightEngineSafe() if (!engine) return const affected = engine['affectedChunksTimestamps'] as Map<string, number> const noAffected = affected.size === 0 - engine.setBlock(x, y, z, convertPrismarineBlockToWorldBlock(stateId, loadedData)) + // Get the appropriate mcData + const mcData = globalThis.mcData || (globalThis.lightEngine?.mcData) + if (!mcData || !mcData.blocks) { + console.error('mcData or mcData.blocks not available for light update') + return + } + // Get the block definition from the state ID + const blockDef = mcData.blocks[stateId] + if (!blockDef) { + console.warn(`No block definition found for stateId ${stateId}`) + return + } + // Convert to the format expected by the light engine + engine.setBlock(x, y, z, convertPrismarineBlockToWorldBlock(blockDef, mcData)) if (affected.size > 0) {
🧹 Nitpick comments (2)
renderer/viewer/lib/worldDataEmitter.ts (1)
79-94: Consider removing commented-out code rather than leaving it in place.The entire setBlockStateId method has been commented out, but keeping large blocks of inactive code makes the file harder to maintain and understand. Since this functionality appears to have been moved to the blockUpdate event handler, it would be cleaner to remove this commented section entirely.
-// setBlockStateId (position: Vec3, stateId: number) { -// const val = this.world.setBlockStateId(position, stateId) as Promise<void> | void -// if (val) throw new Error('setBlockStateId returned promise (not supported)') -// // const chunkX = Math.floor(position.x / 16) -// // const chunkZ = Math.floor(position.z / 16) -// // if (!this.loadedChunks[`${chunkX},${chunkZ}`] && !this.waitingSpiralChunksLoad[`${chunkX},${chunkZ}`]) { -// // void this.loadChunk({ x: chunkX, z: chunkZ }) -// // return -// // } - -// const updateChunks = this.worldRendererConfig.clientSideLighting ? updateBlockLight(position.x, position.y, position.z, stateId) ?? [] : [] -// this.emit('blockUpdate', { pos: position, stateId }) -// for (const chunk of updateChunks) { -// void this.loadChunk(new Vec3(chunk[0] * 16, 0, chunk[1] * 16), true, 'setBlockStateId light update') -// } -// }renderer/viewer/lib/lightEngine.ts (1)
22-22: Consider documenting the purpose of disabling the external world's setBlock.The line
lightEngine.externalWorld.setBlock = () => {}disables the setBlock method on the external world. While this may be intentional to prevent unwanted side effects, it would be helpful to add a comment explaining why this is done.lightEngine = createLightEngineForSyncWorld(worldView.world as unknown as world.WorldSync, loadedData, { minY: worldView.minY, height: worldView.worldHeight, // enableSkyLight: false, }) - lightEngine.externalWorld.setBlock = () => {} + // Disable the external world's setBlock to prevent it from modifying the actual world + // We only want the light engine to calculate lighting, not modify blocks + lightEngine.externalWorld.setBlock = () => {}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
renderer/viewer/lib/lightEngine.ts(1 hunks)renderer/viewer/lib/mesher/mesher.ts(1 hunks)renderer/viewer/lib/worldDataEmitter.ts(7 hunks)renderer/viewer/lib/worldrendererCommon.ts(5 hunks)src/appViewer.ts(1 hunks)src/optionsGuiScheme.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/appViewer.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- renderer/viewer/lib/mesher/mesher.ts
- src/optionsGuiScheme.tsx
- renderer/viewer/lib/worldrendererCommon.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
renderer/viewer/lib/lightEngine.ts (1)
renderer/viewer/lib/worldDataEmitter.ts (1)
WorldDataEmitter(39-401)
🪛 ESLint
renderer/viewer/lib/worldDataEmitter.ts
[error] 307-307: '@typescript-eslint/await-thenable' rule is disabled but never reported.
(eslint-comments/no-unused-disable)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (12)
renderer/viewer/lib/worldDataEmitter.ts (6)
12-13: Good imports for the light engine integration.The imports correctly include the necessary functions from the lightEngine module that will be used throughout this file. This is a good starting point for integrating the new lighting system.
40-45: Well-designed new properties to support the light engine integration.The added properties provide the necessary dimension information (minY, worldHeight, dimensionName) and configuration that the light engine needs. These are correctly initialized with sensible defaults and will be updated when dimension changes are detected.
155-162: Well-implemented block update handler with light engine integration.The blockUpdate event handler now properly integrates with the light engine. It correctly:
- Extracts the stateId from the new block
- Conditionally calls updateBlockLight when clientSideLighting is enabled
- Handles affected chunks by triggering chunk reloads with the light update flag
This implementation ensures that lighting is updated properly when blocks change.
171-173: Good dimension change handling during login and respawn.Extracting the dimension change logic to a separate function (
possiblyDimensionChange) that's called during both login and respawn events is a clean approach. This ensures consistent handling of dimension-specific properties that affect lighting.Also applies to: 174-176
180-185: Well-implemented dimension change handler.The implementation correctly updates the minY, worldHeight, and dimensionName properties from the bot's game state, which are critical for proper light engine operation across different dimensions. The position update with forced recalculation ensures chunks are properly reloaded with the new dimension parameters.
297-299: Good light engine initialization in loadChunk.Calling
createLightEngineIfNeeded(this)at the start of the loadChunk method ensures the light engine is properly initialized before any lighting operations are performed. This is a good defensive programming practice.renderer/viewer/lib/lightEngine.ts (6)
1-5: Good imports for the light engine module.The imports correctly include the necessary components from 'minecraft-lighting' and 'prismarine-world' as well as the local dependencies needed for integration with the rest of the system.
6-13: Well-structured light engine singleton with appropriate accessor methods.The module correctly implements a singleton pattern for the light engine with two accessor methods:
getLightEngine()which throws an error if the engine isn't initialized (for required usages)getLightEngineSafe()which returns null if the engine isn't initialized (for optional usages)This design is appropriate for the various usage patterns in the codebase.
27-37: Good implementation of processLightChunk function.The function correctly:
- Gets the light engine safely, returning early if not available
- Calculates the chunk coordinates
- Calls the engine's receiveUpdateColumn method
- Returns the update result
This implementation provides a clean interface for the WorldDataEmitter to use when processing chunk lighting.
39-42: Well-implemented utility for dumping light data.The dumpLightData function provides a simple and clear interface for getting serialized light data for a chunk, which is useful for sending to workers or for debugging. The use of getLightEngineSafe ensures it won't throw errors if the engine isn't initialized.
44-50: Good debug utility for getting light values.The getDebugLightValues function provides a clean way to extract both block and sky light values at specific coordinates, with proper fallback to -1 if the engine isn't available. This will be useful for debugging and diagnostic UI components.
68-71: Good clean-up implementation in destroyLightEngine.The destroyLightEngine function correctly cleans up both the local module reference to the light engine and the global reference, which helps prevent memory leaks and ensures a clean state for future initialization.
todo do sky light update re-rendering when server: engine skip calc but stores 15 sky levels, it does recalc in worker and merge updated values
todo: worker, horizontal sky fix, FIX SKY LEVEL from time!, fix block light blocking. OTHER done
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
renderer/viewer/three/worldrendererThree.ts (2)
154-158: Document the reason for fixed lighting valuesSetting fixed lighting values instead of using reactive values is a significant change in behavior. It would help to document why this change was made.
if (this.worldRendererConfig.legacyLighting) { this.ambientLight.intensity = value } else { + // Fixed intensity as lighting is now handled by the light engine this.ambientLight.intensity = 1 }
162-166: Document the reason for fixed lighting valuesSimilar to the ambient light change, explain why directional light has a fixed intensity when legacy lighting is disabled.
if (this.worldRendererConfig.legacyLighting) { this.directionalLight.intensity = value } else { + // Fixed intensity as lighting is now handled by the light engine this.directionalLight.intensity = 0.4 }renderer/viewer/lib/worldDataEmitter.ts (2)
235-241:⚠️ Potential issueAdd null check for bot in destroy method
The destroy method attempts to use the global
botvariable directly. This could cause runtime errors ifbotis undefined. Consider adding a proper null check.destroy () { - if (bot) { + if (typeof bot !== 'undefined' && bot) { this.removeListenersFromBot(bot as any) } this.emitter.removeAllListeners() destroyLightEngine() }
155-164: 💡 Verification agent🧩 Analysis chain
Add safeguards for recursive light propagation
The implementation correctly processes light updates and propagates them to affected neighboring chunks. However, if chunks affect each other in a circular pattern, this could potentially lead to excessive recursion or infinite loops.
Consider adding a recursion depth counter or tracking processed chunks to prevent potential infinite loops:
-void this.loadChunk(new Vec3(affectedChunk.x * 16, 0, affectedChunk.z * 16), true) +void this.loadChunk(new Vec3(affectedChunk.x * 16, 0, affectedChunk.z * 16), true, 0, 'light-propagation') async loadChunk (pos: ChunkPos, isLightUpdate = false, recursionDepth = 0, reason = 'spiral') { createLightEngineIfNeeded(this) // existing code... + // Prevent excessive recursion + if (recursionDepth > 5) { + console.warn(`[LightEngine] Prevented excessive recursion at depth ${recursionDepth} for chunk ${pos.x},${pos.z}`) + return + } // rest of method... for (const affectedChunk of result) { if (affectedChunk.x === chunkX && affectedChunk.z === chunkZ) continue const loadedChunk = this.loadedChunks[`${affectedChunk.x},${affectedChunk.z}`] if (!loadedChunk) continue - void this.loadChunk(new Vec3(affectedChunk.x * 16, 0, affectedChunk.z * 16), true) + void this.loadChunk(new Vec3(affectedChunk.x * 16, 0, affectedChunk.z * 16), true, recursionDepth + 1, 'light-propagation') } }
🏁 Script executed:
#!/bin/bash # Check for existing recursion safeguards in light propagation and processing echo "Looking for recursion depth tracking in lightEngine.ts and worldDataEmitter.ts..." # Check for recursion depth tracking in light engine files rg -A 2 -B 2 "recursion|depth|recursionDepth" --glob "renderer/viewer/lib/lightEngine.ts" # Check for any visited/seen chunks tracking (another approach to prevent infinite loops) rg -A 2 -B 2 "seen|visited|processed" --glob "renderer/viewer/lib/lightEngine.ts" # Check if loadChunk ever tracks recursion rg "loadChunk.*recursion|recursion.*loadChunk" --glob "renderer/viewer/lib/worldDataEmitter.ts"Length of output: 440
Implement recursion depth and visited‐chunk tracking in
loadChunkto prevent infinite loopsThe current
loadChunkand light propagation routines lack safeguards against circular dependencies between chunks, which can lead to unbounded recursion when chunks trigger each other in a loop. Add both a recursion‐depth counter and a visited‐chunks set to break cycles gracefully.Locations to update:
renderer/viewer/lib/worldDataEmitter.ts
• Modify theloadChunksignature to acceptrecursionDepthandvisitedparameters.
• Introduce guards at the start of the method.
• Update recursive calls to pass incremented depth and the sharedvisitedset.- All existing
this.loadChunk(…, true, reason)invocations
• Supply a0forrecursionDepthsoreasonremains the fourth argument.Suggested diff:
--- a/renderer/viewer/lib/worldDataEmitter.ts +++ b/renderer/viewer/lib/worldDataEmitter.ts @@ - async loadChunk(pos: ChunkPos, isLightUpdate = false, reason = 'spiral') { + async loadChunk( + pos: ChunkPos, + isLightUpdate = false, + recursionDepth = 0, + visited = new Set<string>(), + reason = 'spiral' + ) { createLightEngineIfNeeded(this) + + // Prevent infinite loops / excessive recursion + const key = `${pos.x},${pos.z}` + if (visited.has(key) || recursionDepth > 5) { + console.warn(`[LightEngine] Skipping chunk ${key} at depth ${recursionDepth}`) + return + } + visited.add(key) // existing logic... for (const affectedChunk of result) { // skip the chunk itself if (affectedChunk.x === chunkX && affectedChunk.z === chunkZ) continue const loadedChunk = this.loadedChunks[`${affectedChunk.x},${affectedChunk.z}`] if (!loadedChunk) continue - void this.loadChunk(new Vec3(affectedChunk.x * 16, 0, affectedChunk.z * 16), true) + void this.loadChunk( + new Vec3(affectedChunk.x * 16, 0, affectedChunk.z * 16), + true, + recursionDepth + 1, + visited, + 'light-propagation' + ) } } // And update callers accordingly: @@ - void this.loadChunk(new Vec3(chunk.x * 16, 0, chunk.z * 16), true, 'setBlockStateId light update') + void this.loadChunk( + new Vec3(chunk.x * 16, 0, chunk.z * 16), + true, + 0, + undefined, + 'setBlockStateId light update' + )These changes will cap recursion at a safe depth and prevent revisiting the same chunks endlessly.
renderer/viewer/lib/lightEngine.ts (1)
16-23: 🛠️ Refactor suggestion
⚠️ Potential issueUndefined
loadedDataand incorrectheightcalculation break engine creation
loadedDatais referenced but never declared/imported, which will throw at runtime.
Additionally, theheightoption should be the vertical span of the world (e.g. 384), notminY + worldHeight– this silently short-cuts the top of the world wheneverminY≠ 0.-import { LightWorld, createLightEngineForSyncWorld, convertPrismarineBlockToWorldBlock } from 'minecraft-lighting' +import { LightWorld, createLightEngineForSyncWorld, convertPrismarineBlockToWorldBlock } from 'minecraft-lighting' +import mcDataRaw from 'minecraft-data' // ✅ supplies loadedData ... export const createLightEngineIfNeeded = (worldView: WorldDataEmitter) => { if (lightEngine) return - lightEngine = createLightEngineForSyncWorld(worldView.world as unknown as world.WorldSync, loadedData, { + const loadedData = mcDataRaw(worldView.version) // <-- pick the correct mc-data set + lightEngine = createLightEngineForSyncWorld( + worldView.world as unknown as world.WorldSync, + loadedData, + { minY: worldView.minY, - height: worldView.minY + worldView.worldHeight, + height: worldView.worldHeight, // <-- actual span // writeLightToOriginalWorld: true, // enableSkyLight: false, })Without this change the viewer crashes on the very first world load.
🧹 Nitpick comments (10)
README.MD (1)
15-16: Fix hyphenation in compound adjectiveThe feature description "Combined Lighting System - Server Parsing + Client Side Engine for block updates" uses "Client Side" as a compound adjective modifying "Engine", which should be hyphenated.
-Combined Lighting System - Server Parsing + Client Side Engine for block updates +Combined Lighting System - Server Parsing + Client-Side Engine for block updates🧰 Tools
🪛 LanguageTool
[uncategorized] ~16-~16: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ined Lighting System - Server Parsing + Client Side Engine for block updates - Official Min...(EN_COMPOUND_ADJECTIVE_INTERNAL)
renderer/viewer/three/worldrendererThree.ts (1)
247-259: Consider adding logging level or making the log message more informativeThe console log message provides basic information about skylight changes, but it could be improved to be more informative or use a logging level to avoid cluttering the console during normal operation.
skylightUpdated (): void { let updated = 0 for (const sectionKey of Object.keys(this.sectionObjects)) { if (this.sectionObjects[sectionKey].hasSkylight) { // set section to be updated const [x, y, z] = sectionKey.split(',').map(Number) this.setSectionDirty(new Vec3(x, y, z)) updated++ } } - console.log(`Skylight changed to ${this.skyLight}. Updated`, updated, 'sections') + console.debug(`[LightEngine] Skylight changed to ${this.skyLight}. Updated ${updated} sections`) }src/optionsStorage.ts (1)
178-181: Improve performance by avoiding the delete operatorUsing the
deleteoperator can impact performance. Consider usingundefinedassignment instead.if (options.dayCycleAndLighting) { - delete options.dayCycleAndLighting + options.dayCycleAndLighting = undefined options.dayCycle = options.dayCycleAndLighting }🧰 Tools
🪛 Biome (1.9.4)
[error] 179-179: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
renderer/viewer/lib/worldDataEmitter.ts (2)
309-310: Remove unnecessary ESLint disable commentThe ESLint disable comment for '@typescript-eslint/await-thenable' is never reported and can be safely removed.
if (dx <= this.viewDistance && dz <= this.viewDistance) { - // eslint-disable-next-line @typescript-eslint/await-thenable -- todo allow to use async world provider but not sure if needed const column = await this.world.getColumnAt(pos['y'] ? pos as Vec3 : new Vec3(pos.x, 0, pos.z))🧰 Tools
🪛 ESLint
[error] 309-309: '@typescript-eslint/await-thenable' rule is disabled but never reported.
(eslint-comments/no-unused-disable)
313-316: Consider adding a comment to explain lighting strategy optionsThe conditional check for
clientSideLighting === 'full'doesn't provide context about what other options are available and what they do.let result = [] as Array<{ x: number, z: number }> -if (!isLightUpdate && this.worldRendererConfig.clientSideLighting === 'full') { +// Only process chunk lighting if this isn't a light update itself and full client-side lighting is enabled +// clientSideLighting can be 'none', 'partial', or 'full' depending on lighting strategy +if (!isLightUpdate && this.worldRendererConfig.clientSideLighting === 'full') { result = await processLightChunk(pos.x, pos.z) ?? [] } if (!result) returnrenderer/viewer/lib/lightEngine.ts (2)
53-63:stateIdparameter is ignored – either use it or drop it
updateBlockLightacceptsstateIdbut never forwards it to the engine, so block-light re-calculation uses stale data. If the engine API supports passing a converted block, wire it; otherwise remove the dead parameter to avoid confusion.-export const updateBlockLight = async (x: number, y: number, z: number, stateId: number, distance: number) => { +export const updateBlockLight = async ( + x: number, + y: number, + z: number, + stateId: number, // TODO: remove or forward + distance: number, +) => {If forwarding is possible, call:
const mcBlock = loadedData.blocks[stateId] engine.setBlock(x, y, z, convertPrismarineBlockToWorldBlock(mcBlock, loadedData))
1-4: Remove unused import to keep bundle lean
convertPrismarineBlockToWorldBlockis not referenced in live code anymore. Delete the import (and any dead code) after wiringstateIdas discussed above to prevent tree-shaking misses and easier grepping.renderer/viewer/lib/worldrendererCommon.ts (3)
22-23: Drop unused symbols fromlightEngineimport
createLightEngineIfNeeded,getLightEngine, andgetLightEngineSafeare imported but never used in this file. Keeping them around triggers linter warnings and bloats the bundle.-import { createLightEngineIfNeeded, dumpLightData, getLightEngine, getLightEngineSafe } from './lightEngine' +import { dumpLightData } from './lightEngine'
539-553: Mid-night is as bright as mid-day – refine sky-light curveCurrent logic assigns
skyLight = 15fortimeOfDay >= 18 000, making midnight full-bright. Consider mapping 18 000–24 000 down to the night minimum (≈ 4) to mimic vanilla:-} else if (timeOfDay <= 6000 || timeOfDay >= 18_000) { - skyLight = 15 +} else if (timeOfDay <= 6_000) { // sunrise-to-midday + skyLight = 15 +} else if (timeOfDay >= 18_000) { // sunset-to-midnight + skyLight = 4Fine-tune values as desired; the key is avoiding full brightness at night.
640-642: Guard against undefinedlightDatato avoid excessive postMessage sizeIf the light engine is not yet initialised,
dumpLightDatareturnsundefined, yet the property is still included in every worker message. Short-circuit to keep IPC payload minimal:- lightData: dumpLightData(x, z) + ...(dumpLightData(x, z) && { lightData: dumpLightData(x, z) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
README.MD(1 hunks)package.json(1 hunks)renderer/viewer/lib/basePlayerState.ts(1 hunks)renderer/viewer/lib/lightEngine.ts(1 hunks)renderer/viewer/lib/mesher/models.ts(7 hunks)renderer/viewer/lib/mesher/shared.ts(2 hunks)renderer/viewer/lib/mesher/world.ts(3 hunks)renderer/viewer/lib/worldDataEmitter.ts(8 hunks)renderer/viewer/lib/worldrendererCommon.ts(10 hunks)renderer/viewer/three/worldrendererThree.ts(6 hunks)src/dayCycle.ts(2 hunks)src/mineflayer/playerState.ts(4 hunks)src/optionsGuiScheme.tsx(1 hunks)src/optionsStorage.ts(4 hunks)src/react/DebugOverlay.tsx(4 hunks)src/watchOptions.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- package.json
- renderer/viewer/lib/basePlayerState.ts
- src/dayCycle.ts
- src/react/DebugOverlay.tsx
- renderer/viewer/lib/mesher/shared.ts
- src/mineflayer/playerState.ts
- src/optionsGuiScheme.tsx
- renderer/viewer/lib/mesher/models.ts
- renderer/viewer/lib/mesher/world.ts
- src/watchOptions.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
renderer/viewer/lib/lightEngine.ts (1)
renderer/viewer/lib/worldDataEmitter.ts (1)
WorldDataEmitter(39-404)
renderer/viewer/lib/worldrendererCommon.ts (2)
renderer/viewer/lib/mesher/shared.ts (1)
MesherConfig(24-24)renderer/viewer/lib/lightEngine.ts (1)
dumpLightData(40-43)
renderer/viewer/lib/worldDataEmitter.ts (3)
renderer/viewer/lib/worldrendererCommon.ts (1)
WorldRendererConfig(59-59)renderer/viewer/lib/lightEngine.ts (5)
updateBlockLight(53-77)destroyLightEngine(85-88)createLightEngineIfNeeded(15-26)processLightChunk(28-38)lightRemoveColumn(79-83)renderer/viewer/lib/simpleUtils.ts (1)
chunkPos(24-28)
🪛 LanguageTool
README.MD
[uncategorized] ~16-~16: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ined Lighting System - Server Parsing + Client Side Engine for block updates - Official Min...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🪛 ESLint
renderer/viewer/lib/worldDataEmitter.ts
[error] 309-309: '@typescript-eslint/await-thenable' rule is disabled but never reported.
(eslint-comments/no-unused-disable)
🪛 Biome (1.9.4)
src/optionsStorage.ts
[error] 179-179: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
/deploy |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/optionsStorage.ts (4)
11-12: Clarify the TODO commentThe TODO comment on line 11 is empty and doesn't explain what needs to be done. Adding a description would help other developers understand the purpose of this comment and the associated constant
IS_BETA_TESTER.-// todo +// TODO: Implement comprehensive beta tester detection and features management
52-53: Document the experimental lighting optionThe
experimentalLightingV1option lacks documentation explaining its purpose and impact, unlike the well-documentedlightingStrategyoption.-// experimentalLighting: IS_BETA_TESTER, -experimentalLightingV1: location.hostname.startsWith('lighting.'), +// experimentalLighting: IS_BETA_TESTER, // Old lighting implementation +/** + * Enables version 1 of the experimental lighting engine + * Only active on lighting.* hostnames for controlled testing + */ +experimentalLightingV1: location.hostname.startsWith('lighting.'),
183-186: Consider performance implications of delete operatorUsing the
deleteoperator can impact performance. Consider using an undefined assignment instead, as suggested by static analysis.if (options.dayCycleAndLighting) { - delete options.dayCycleAndLighting + options.dayCycleAndLighting = undefined options.dayCycle = options.dayCycleAndLighting }🧰 Tools
🪛 Biome (1.9.4)
[error] 184-184: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
183-186: Enhance migration logic for lighting settingsThe current migration only handles transferring the
dayCycleAndLightingvalue todayCycle, but doesn't address how the lighting part of that setting should be migrated to the newlightingStrategyoption.if (options.dayCycleAndLighting) { delete options.dayCycleAndLighting options.dayCycle = options.dayCycleAndLighting + // Ensure lighting strategy is also properly migrated + if (options.dayCycleAndLighting && options.lightingStrategy === undefined) { + options.lightingStrategy = 'prefer-server' + } }🧰 Tools
🪛 Biome (1.9.4)
[error] 184-184: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/optionsStorage.ts(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/optionsStorage.ts
[error] 184-184: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (1)
src/optionsStorage.ts (1)
106-112: Well-documented lighting strategy optionsThe documentation for the lighting strategy options is clear and helpful, explaining each option's purpose. This is good practice for user-facing configuration options.
|
Deployed to Vercel Preview: https://prismarine-7bk41g0wf-zaro.vercel.app |
|
/deploy |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
renderer/viewer/lib/worldDataEmitter.ts (2)
235-241:⚠️ Potential issueFix undefined bot reference in destroy method
The
destroymethod checks for an undefinedbotvariable which is not defined in this scope.This will cause a ReferenceError when
destroy()is called. Fix this by using a proper null check or use an instance property:destroy () { - if (bot) { + if (typeof bot !== 'undefined' && bot) { this.removeListenersFromBot(bot as any) } this.emitter.removeAllListeners() destroyLightEngine() }
313-326:⚠️ Potential issueAdd safeguard against recursive light propagation
The current implementation could potentially lead to infinite recursion if chunks affect each other in a circular pattern.
Add a recursion depth counter to prevent excessive recursion:
-async loadChunk (pos: ChunkPos, isLightUpdate = false, reason = 'spiral') { +async loadChunk (pos: ChunkPos, isLightUpdate = false, reason = 'spiral', recursionDepth = 0) { createLightEngineIfNeededNew(this) // ... existing code ... if (!isLightUpdate) { const computeLighting = this.worldRendererConfig.clientSideLighting === 'full' const promise = processLightChunk(pos.x, pos.z, computeLighting) if (computeLighting) { result = (await promise) ?? [] } } if (!result) return + // Prevent excessive recursion + if (recursionDepth >= 5) return for (const affectedChunk of result) { if (affectedChunk.chunkX === chunkX && affectedChunk.chunkZ === chunkZ) continue const loadedChunk = this.loadedChunks[`${affectedChunk.chunkX * 16},${affectedChunk.chunkZ * 16}`] if (!loadedChunk) continue - void this.loadChunk(new Vec3(affectedChunk.chunkX * 16, 0, affectedChunk.chunkZ * 16), true) + void this.loadChunk(new Vec3(affectedChunk.chunkX * 16, 0, affectedChunk.chunkZ * 16), true, 'light update recursion', recursionDepth + 1) }renderer/viewer/lib/lightEngine.ts (2)
20-20:⚠️ Potential issueUndefined loadedData variable
The code uses an undefined
loadedDatavariable which will cause runtime errors.You need to define or import the
loadedDatavariable:import { LightWorld, createLightEngineForSyncWorld, convertPrismarineBlockToWorldBlock, createPrismarineLightEngineWorker } from 'minecraft-lighting' import { world } from 'prismarine-world' // import PrismarineWorker from 'minecraft-lighting/dist/prismarineWorker.worker.js' import { WorldDataEmitter } from './worldDataEmitter' +import { mcData } from './mcData' // Or wherever the appropriate data is defined // ... export const createLightEngineIfNeeded = (worldView: WorldDataEmitter) => { if (lightEngine) return + const loadedData = mcData // Or the appropriate data source lightEngine = createLightEngineForSyncWorld(worldView.world as unknown as world.WorldSync, loadedData, { // ... })
34-34:⚠️ Potential issueUndefined loadedData variable in createLightEngineIfNeededNew
Same as above, the
loadedDatavariable is used but not defined or imported.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
renderer/viewer/lib/lightEngine.ts(1 hunks)renderer/viewer/lib/worldDataEmitter.ts(8 hunks)renderer/viewer/lib/worldrendererCommon.ts(10 hunks)src/react/DebugOverlay.tsx(3 hunks)src/shims/minecraftData.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/react/DebugOverlay.tsx
- renderer/viewer/lib/worldrendererCommon.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/shims/minecraftData.ts (1)
src/utils.ts (1)
toMajorVersion(132-135)
renderer/viewer/lib/lightEngine.ts (1)
renderer/viewer/lib/worldDataEmitter.ts (1)
WorldDataEmitter(39-408)
renderer/viewer/lib/worldDataEmitter.ts (3)
renderer/viewer/lib/worldrendererCommon.ts (1)
WorldRendererConfig(57-57)renderer/viewer/lib/lightEngine.ts (5)
updateBlockLight(70-95)destroyLightEngine(103-106)createLightEngineIfNeededNew(31-43)processLightChunk(45-55)lightRemoveColumn(97-101)renderer/viewer/lib/simpleUtils.ts (1)
chunkPos(24-28)
🪛 GitHub Actions: CI
renderer/viewer/lib/lightEngine.ts
[error] 39-41: Module not found: Can't resolve 'minecraft-lighting/dist/prismarineWorker.worker.js'. The file or module is missing or the path is incorrect.
[error] 40-40: ESModulesLinkingError: export 'createPrismarineLightEngineWorker' was not found in 'minecraft-lighting'. Possible exports are: LightWorld, convertPrismarineBlockToWorldBlock, createLightEngineForSyncWorld, fillColumnWithZeroLight, getPrismarineSyncWorld.
🪛 ESLint
renderer/viewer/lib/worldDataEmitter.ts
[error] 309-309: '@typescript-eslint/await-thenable' rule is disabled but never reported.
(eslint-comments/no-unused-disable)
🔇 Additional comments (5)
src/shims/minecraftData.ts (3)
6-9: Inline implementation of previously imported utilityThis change inlines the
toMajorVersionutility function that was previously imported from another file. This maintains the exact functionality while reducing external dependencies.
26-27: Good addition for worker compatibilityThis change adds worker environment compatibility by ensuring
globalThis.windowis defined, allowing the code to run seamlessly in both browser and worker contexts.
76-76: Improved initialization using nullish coalescingChanged from unconditional assignment to nullish coalescing assignment (
??=), ensuring the cache is only initialized if not already defined. This prevents overwriting existing data and works better in worker contexts.renderer/viewer/lib/worldDataEmitter.ts (2)
155-164: Improved block update handling with lightingThe
blockUpdatemethod now properly handles lighting updates through the light engine. The distance check is a good optimization to avoid processing updates for distant blocks.
182-187: Good addition of dimension change trackingThe
possiblyDimensionChangemethod properly updates dimension properties from the bot's game data and triggers a position update.
| result = (await promise) ?? [] | ||
| } | ||
| } | ||
| if (!result) return |
There was a problem hiding this comment.
Potential logic issue with null/empty array check
The check if (!result) return may not work as expected since an empty array is truthy in JavaScript.
If you're trying to check for an empty array, use if (!result.length) return or for a null/undefined check, use if (result == null) return.
-if (!result) return
+if (!result || !result.length) return📝 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.
| if (!result) return | |
| // around line 320 in renderer/viewer/lib/worldDataEmitter.ts | |
| - if (!result) return | |
| + if (!result || !result.length) return |
| minY = -64 | ||
| worldHeight = 384 | ||
| dimensionName = '' | ||
|
|
||
| worldRendererConfig: WorldRendererConfig |
There was a problem hiding this comment.
Added dimension tracking properties and renderer config
The class now tracks dimension properties (minY, worldHeight, dimensionName) and includes a WorldRendererConfig property. However, the worldRendererConfig property is not initialized in the constructor.
The worldRendererConfig property needs to be initialized in the constructor or via another initialization method to avoid potential null reference errors when it's accessed in methods like blockUpdate and loadChunk.
constructor (public world: typeof __type_bot['world'], public viewDistance: number, position: Vec3 = new Vec3(0, 0, 0)) {
// eslint-disable-next-line constructor-super
super()
this.loadedChunks = {}
this.lastPos = new Vec3(0, 0, 0).update(position)
// todo
this.emitter = this
+ this.worldRendererConfig = { clientSideLighting: 'none' } // Add default configuration
}📝 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.
| minY = -64 | |
| worldHeight = 384 | |
| dimensionName = '' | |
| worldRendererConfig: WorldRendererConfig | |
| constructor ( | |
| public world: typeof __type_bot['world'], | |
| public viewDistance: number, | |
| position: Vec3 = new Vec3(0, 0, 0), | |
| ) { | |
| // eslint-disable-next-line constructor-super | |
| super() | |
| this.loadedChunks = {} | |
| this.lastPos = new Vec3(0, 0, 0).update(position) | |
| // todo | |
| this.emitter = this | |
| this.worldRendererConfig = { clientSideLighting: 'none' } // Add default configuration | |
| } |
renderer/viewer/lib/lightEngine.ts
Outdated
| export const destroyLightEngine = () => { | ||
| lightEngine = null | ||
| globalThis.lightEngine = null | ||
| } |
There was a problem hiding this comment.
Incomplete light engine cleanup
The destroyLightEngine function only destroys the synchronous engine and not the worker-based one.
Update the function to clean up both engine types:
export const destroyLightEngine = () => {
+ if (lightEngineNew) {
+ lightEngineNew.destroy?.()
+ lightEngineNew = null
+ }
lightEngine = null
globalThis.lightEngine = null
}📝 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.
| export const destroyLightEngine = () => { | |
| lightEngine = null | |
| globalThis.lightEngine = null | |
| } | |
| export const destroyLightEngine = () => { | |
| if (lightEngineNew) { | |
| lightEngineNew.destroy?.() | |
| lightEngineNew = null | |
| } | |
| lightEngine = null | |
| globalThis.lightEngine = null | |
| } |
| export const updateBlockLight = async (x: number, y: number, z: number, stateId: number, distance: number) => { | ||
| if (distance > 16) return [] | ||
| const chunkX = Math.floor(x / 16) * 16 | ||
| const chunkZ = Math.floor(z / 16) * 16 | ||
| const engine = getLightEngineSafe() | ||
| if (!engine) return | ||
| const start = performance.now() | ||
| const result = await engine.setBlock(x, y, z, stateId) | ||
| const end = performance.now() | ||
| console.log(`[light engine] updateBlockLight (${x}, ${y}, ${z}) took`, Math.round(end - start), 'ms', result.length, 'chunks') | ||
| return result |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for light engine operations
The updateBlockLight function logs performance data but doesn't handle errors.
Add try/catch block to handle potential errors from the light engine:
export const updateBlockLight = async (x: number, y: number, z: number, stateId: number, distance: number) => {
if (distance > 16) return []
const chunkX = Math.floor(x / 16) * 16
const chunkZ = Math.floor(z / 16) * 16
const engine = getLightEngineSafe()
if (!engine) return
const start = performance.now()
+ try {
const result = await engine.setBlock(x, y, z, stateId)
const end = performance.now()
console.log(`[light engine] updateBlockLight (${x}, ${y}, ${z}) took`, Math.round(end - start), 'ms', result.length, 'chunks')
return result
+ } catch (error) {
+ console.error(`[light engine] Error updating block light at (${x}, ${y}, ${z}):`, error)
+ return []
+ }
}📝 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.
| export const updateBlockLight = async (x: number, y: number, z: number, stateId: number, distance: number) => { | |
| if (distance > 16) return [] | |
| const chunkX = Math.floor(x / 16) * 16 | |
| const chunkZ = Math.floor(z / 16) * 16 | |
| const engine = getLightEngineSafe() | |
| if (!engine) return | |
| const start = performance.now() | |
| const result = await engine.setBlock(x, y, z, stateId) | |
| const end = performance.now() | |
| console.log(`[light engine] updateBlockLight (${x}, ${y}, ${z}) took`, Math.round(end - start), 'ms', result.length, 'chunks') | |
| return result | |
| export const updateBlockLight = async ( | |
| x: number, | |
| y: number, | |
| z: number, | |
| stateId: number, | |
| distance: number | |
| ) => { | |
| if (distance > 16) return [] | |
| const chunkX = Math.floor(x / 16) * 16 | |
| const chunkZ = Math.floor(z / 16) * 16 | |
| const engine = getLightEngineSafe() | |
| if (!engine) return | |
| const start = performance.now() | |
| try { | |
| const result = await engine.setBlock(x, y, z, stateId) | |
| const end = performance.now() | |
| console.log( | |
| `[light engine] updateBlockLight (${x}, ${y}, ${z}) took`, | |
| Math.round(end - start), | |
| 'ms', | |
| result.length, | |
| 'chunks' | |
| ) | |
| return result | |
| } catch (error) { | |
| console.error( | |
| `[light engine] Error updating block light at (${x}, ${y}, ${z}):`, | |
| error | |
| ) | |
| return [] | |
| } | |
| } |
renderer/viewer/lib/lightEngine.ts
Outdated
| import { LightWorld, createLightEngineForSyncWorld, convertPrismarineBlockToWorldBlock, createPrismarineLightEngineWorker } from 'minecraft-lighting' | ||
| import { world } from 'prismarine-world' | ||
| // import PrismarineWorker from 'minecraft-lighting/dist/prismarineWorker.worker.js' | ||
| import { WorldDataEmitter } from './worldDataEmitter' |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Missing dependency resources for light engine worker
The pipeline failures indicate issues with the worker implementation.
The pipeline detected the following errors:
- Module 'minecraft-lighting/dist/prismarineWorker.worker.js' cannot be resolved
- Export 'createPrismarineLightEngineWorker' was not found in 'minecraft-lighting'
Update the import statements to correctly reference the available APIs or ensure the proper version of 'minecraft-lighting' with worker support is installed.
-import { LightWorld, createLightEngineForSyncWorld, convertPrismarineBlockToWorldBlock, createPrismarineLightEngineWorker } from 'minecraft-lighting'
+import { LightWorld, createLightEngineForSyncWorld, convertPrismarineBlockToWorldBlock } from 'minecraft-lighting'
import { world } from 'prismarine-world'
-// import PrismarineWorker from 'minecraft-lighting/dist/prismarineWorker.worker.js'
+// Define a fallback implementation for the worker until proper support is available🌐 Web query:
Does the minecraft-lighting package support worker-based light engines and provide a createPrismarineLightEngineWorker export?
💡 Result:
Based on the search results provided, there is no specific information about a "minecraft-lighting" package that supports worker-based light engines or provides a "createPrismarineLightEngineWorker" export. The search results primarily discuss various Minecraft lighting implementations and mods, but none mention this specific package or export function.
The search results do cover several lighting-related topics in Minecraft:
-
Various lighting engine mods such as Starlight by PaperMC, which rewrites the light engine to fix lighting issues and improve performance[2].
-
Alfheim Lighting Engine, a fork/rewrite of previous lighting engines (Hesperus and Phosphor) that aims to improve performance and fix compatibility issues[3].
-
Custom implementations of lighting in Minecraft clones, including colored lighting and different propagation methods[1][9].
-
Discussions about dynamic lighting in Minecraft and potential methods to implement it[5].
However, none of these sources specifically mention a "minecraft-lighting" package with worker-based light engines or a "createPrismarineLightEngineWorker" export. If you're looking for this specific functionality, it might be part of a different package than those covered in the search results, or it might not exist at all.
Citations:
- 1: https://www.youtube.com/watch?v=NLbzPQoHkXI
- 2: https://github.com/PaperMC/Starlight
- 3: https://www.curseforge.com/minecraft/mc-mods/alfheim-lighting-engine
- 4: https://www.youtube.com/watch?v=GfTybJTb2j4
- 5: https://www.minecraftforum.net/forums/minecraft-java-edition/suggestions/3128591-a-potential-method-to-make-dynamic-lighting-work
- 6: https://www.youtube.com/watch?v=fEBdxNjY9YU
- 7: https://www.youtube.com/watch?v=aeb8exYb614
- 8: https://www.youtube.com/watch?v=wVj3-Ffs5gk
- 9: https://www.youtube.com/watch?v=s9KQRMtiTzE
- 10: https://www.youtube.com/watch?v=RGRTOUyeEy8
Remove unsupported worker imports from lightEngine.ts
The minecraft-lighting package does not expose a createPrismarineLightEngineWorker export nor bundle a prismarineWorker.worker.js, causing build failures:
- Error: Cannot resolve module
minecraft-lighting/dist/prismarineWorker.worker.js - Error: Export
createPrismarineLightEngineWorkernot found inminecraft-lighting
Please update the import and provide a temporary stub or fallback until upstream worker support is available.
Locations to update:
- renderer/viewer/lib/lightEngine.ts
Proposed diff:
--- a/renderer/viewer/lib/lightEngine.ts
+++ b/renderer/viewer/lib/lightEngine.ts
@@ -1,4 +1,3 @@
-import { LightWorld, createLightEngineForSyncWorld, convertPrismarineBlockToWorldBlock, createPrismarineLightEngineWorker } from 'minecraft-lighting'
+import { LightWorld, createLightEngineForSyncWorld, convertPrismarineBlockToWorldBlock } from 'minecraft-lighting'
import { world } from 'prismarine-world'
-// import PrismarineWorker from 'minecraft-lighting/dist/prismarineWorker.worker.js'
+// TODO: implement or stub out a worker-based light engine once supported upstream📝 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.
| import { LightWorld, createLightEngineForSyncWorld, convertPrismarineBlockToWorldBlock, createPrismarineLightEngineWorker } from 'minecraft-lighting' | |
| import { world } from 'prismarine-world' | |
| // import PrismarineWorker from 'minecraft-lighting/dist/prismarineWorker.worker.js' | |
| import { WorldDataEmitter } from './worldDataEmitter' | |
| --- a/renderer/viewer/lib/lightEngine.ts | |
| +++ b/renderer/viewer/lib/lightEngine.ts | |
| @@ -1,4 +1,4 @@ | |
| -import { LightWorld, createLightEngineForSyncWorld, convertPrismarineBlockToWorldBlock, createPrismarineLightEngineWorker } from 'minecraft-lighting' | |
| +import { LightWorld, createLightEngineForSyncWorld, convertPrismarineBlockToWorldBlock } from 'minecraft-lighting' | |
| import { world } from 'prismarine-world' | |
| -// import PrismarineWorker from 'minecraft-lighting/dist/prismarineWorker.worker.js' | |
| +// TODO: implement or stub out a worker-based light engine once supported upstream | |
| import { WorldDataEmitter } from './worldDataEmitter' |
|
Deployed to Vercel Preview: https://prismarine-mj9vc80kb-zaro.vercel.app |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/optionsStorage.ts (2)
11-12: Address TODO comment and clarify usage of IS_BETA_TESTERThe
IS_BETA_TESTERconstant is defined but appears to be unused in this file after being commented out fromexperimentalLighting. Consider either removing this constant if no longer needed, or completing the TODO to clarify its purpose.
183-186: Replace delete operator with undefined assignmentUsing the
deleteoperator can impact performance as flagged by static analysis. Consider using an undefined assignment instead.if (options.dayCycleAndLighting) { - delete options.dayCycleAndLighting + options.dayCycleAndLighting = undefined options.dayCycle = options.dayCycleAndLighting }🧰 Tools
🪛 Biome (1.9.4)
[error] 184-184: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
package.json(1 hunks)src/optionsGuiScheme.tsx(1 hunks)src/optionsStorage.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- src/optionsGuiScheme.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
src/optionsStorage.ts
[error] 184-184: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (1)
src/optionsStorage.ts (1)
106-112: Good documentation for lightingStrategy optionsThe documentation clearly explains the three lighting strategy options, fulfilling the previous review suggestion. This helps users understand the implications of each setting.
todo: fix chunks not receive skylight, recompute fix skylight values desync time
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
renderer/viewer/lib/lightEngine.ts (2)
57-68: 🛠️ Refactor suggestionAdd error handling for light engine operations.
The
updateBlockLightfunction doesn't handle errors from the light engine operations. Ifengine.setBlockthrows an error, it will crash the application.export const updateBlockLight = async (x: number, y: number, z: number, stateId: number, distance: number) => { if (distance > 16) return [] const chunkX = Math.floor(x / 16) * 16 const chunkZ = Math.floor(z / 16) * 16 const engine = getLightEngineSafe() if (!engine) return const start = performance.now() + try { const result = await engine.setBlock(x, y, z, stateId) const end = performance.now() console.log(`[light engine] updateBlockLight (${x}, ${y}, ${z}) took`, Math.round(end - start), 'ms', result.length, 'chunks') return result + } catch (error) { + console.error(`[light engine] Error updating block light at (${x}, ${y}, ${z}):`, error) + return [] + } }
90-93:⚠️ Potential issueIncomplete light engine cleanup.
The
destroyLightEnginefunction doesn't call any cleanup methods on the light engine before nulling it out. You should call the engine's destroy method to properly release resources.export const destroyLightEngine = () => { + if (lightEngineNew) { + lightEngineNew.destroy?.() + lightEngineNew = null + } - lightEngineNew = null globalThis.lightEngine = null }
🧹 Nitpick comments (4)
src/optionsStorage.ts (2)
11-12: Remove or complete the empty TODO comment.The TODO comment is empty without any details of what needs to be done. Either remove it or add a description of the intended task.
-// todo const IS_BETA_TESTER = location.hostname.startsWith('s.') || location.hostname.startsWith('beta.') +const IS_BETA_TESTER = location.hostname.startsWith('s.') || location.hostname.startsWith('beta.')
183-186: Avoid using delete operator for better performance.The delete operator can impact performance. Consider setting the property to undefined instead.
if (options.dayCycleAndLighting) { - delete options.dayCycleAndLighting + options.dayCycleAndLighting = undefined options.dayCycle = options.dayCycleAndLighting }🧰 Tools
🪛 Biome (1.9.4)
[error] 184-184: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
renderer/viewer/lib/lightEngine.ts (2)
1-5: Remove commented-out import that's not being used.The commented-out import should be removed if it's not needed. Based on past reviews, there were issues with this import.
import { createPrismarineLightEngineWorker } from 'minecraft-lighting' import { world } from 'prismarine-world' -// import PrismarineWorker from 'minecraft-lighting/dist/prismarineWorker.worker.js' import { WorldDataEmitter } from './worldDataEmitter' import { initMesherWorker, meshersSendMcData } from './worldrendererCommon'
69-82: Remove commented-out code block.This large block of commented-out code should be removed if it's no longer needed. It makes the file harder to read and maintain.
- // const engine = getLightEngineSafe() - // if (!engine) return - // const affected = engine['affectedChunksTimestamps'] as Map<string, number> - // const noAffected = affected.size === 0 - // engine.setBlock(x, y, z, convertPrismarineBlockToWorldBlock(stateId, loadedData)) - - // if (affected.size > 0) { - // const chunks = [...affected.keys()].map(key => { - // return key.split(',').map(Number) as [number, number] - // }) - // affected.clear() - - // return chunks - // }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
package.json(1 hunks)renderer/viewer/lib/lightEngine.ts(1 hunks)renderer/viewer/lib/mesher/mesher.ts(3 hunks)renderer/viewer/lib/worldDataEmitter.ts(8 hunks)renderer/viewer/lib/worldrendererCommon.ts(11 hunks)src/optionsStorage.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- package.json
- renderer/viewer/lib/mesher/mesher.ts
- renderer/viewer/lib/worldDataEmitter.ts
- renderer/viewer/lib/worldrendererCommon.ts
🧰 Additional context used
🪛 Biome (1.9.4)
src/optionsStorage.ts
[error] 184-184: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (1)
src/optionsStorage.ts (1)
106-112: Well-documented lighting strategy options.The documentation for the lightingStrategy option is clear and helpful, explaining each of the three modes and their use cases.
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-9dz52t6fy-zaro.vercel.app |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
renderer/viewer/lib/worldrendererCommon.ts (1)
546-558:⚠️ Potential issue
usingCustomLightHoldershould betruewhen chunk message carrieslightData.You now feed pre-computed light data to workers in the
addColumnmethod with thelightDataparameter. IfusingCustomLightHolderremainsfalse, the mesher will ignore those values and fall back to ambient/vanilla lighting.- usingCustomLightHolder: false, + usingCustomLightHolder: !!this.worldRendererConfig.clientSideLighting && this.worldRendererConfig.clientSideLighting !== 'none',
🧹 Nitpick comments (2)
src/optionsStorage.ts (1)
11-12: Consider removing the TODO comment.The TODO comment on line 11 doesn't provide any actionable information. Either add specific details about what needs to be done or remove it.
-// todo +// This constant identifies beta tester environments for feature flags const IS_BETA_TESTER = location.hostname.startsWith('s.') || location.hostname.startsWith('beta.')README.MD (1)
16-16: Fix compound adjective hyphenation in feature description.The term "Client Side Engine" should be hyphenated as "Client-Side Engine" since it's a compound adjective modifying "Engine".
-Combined Lighting System - Server Parsing + Client Side Engine for block updates +Combined Lighting System - Server Parsing + Client-Side Engine for block updates🧰 Tools
🪛 LanguageTool
[uncategorized] ~16-~16: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ined Lighting System - Server Parsing + Client Side Engine for block updates - Official Min...(EN_COMPOUND_ADJECTIVE_INTERNAL)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
README.MD(1 hunks)package.json(1 hunks)renderer/viewer/lib/mesher/mesher.ts(3 hunks)renderer/viewer/lib/mesher/models.ts(2 hunks)renderer/viewer/lib/mesher/shared.ts(2 hunks)renderer/viewer/lib/worldrendererCommon.ts(11 hunks)src/appViewer.ts(1 hunks)src/optionsGuiScheme.tsx(1 hunks)src/optionsStorage.ts(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- renderer/viewer/lib/mesher/models.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- package.json
- src/appViewer.ts
- renderer/viewer/lib/mesher/shared.ts
- renderer/viewer/lib/mesher/mesher.ts
- src/optionsGuiScheme.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
renderer/viewer/lib/worldrendererCommon.ts (3)
renderer/viewer/lib/mesher/shared.ts (1)
MesherConfig(24-24)renderer/viewer/lib/lightEngine.ts (1)
dumpLightData(44-47)src/utils.ts (1)
toMajorVersion(134-137)
🪛 LanguageTool
README.MD
[uncategorized] ~16-~16: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ined Lighting System - Server Parsing + Client Side Engine for block updates - Official Min...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🪛 Biome (1.9.4)
src/optionsStorage.ts
[error] 185-185: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (7)
src/optionsStorage.ts (1)
107-113: Great documentation for lighting strategy options!The JSDoc comments clearly explain the three lighting strategies, which will help developers understand how to configure the lighting system.
renderer/viewer/lib/worldrendererCommon.ts (6)
529-543: Well-implemented skylight calculation based on time of day.The skylight calculation logically maps time of day to light levels, creating a smooth day-night cycle effect.
794-800: Good handling of skylight updates.The code now properly calculates skylight changes based on the time of day and notifies the renderer to update lighting when changes are detected.
1004-1024: Good extraction of worker initialization logic.Extracting the worker creation logic into a separate function improves code organization and reusability. The implementation handles both single-file build and standard worker creation paths appropriately.
1026-1037: Well-structured mesher data sending mechanism.The extracted
meshersSendMcDatafunction is a good improvement that centralizes the logic for building and sending configuration data to workers. This makes the code more maintainable and reduces duplication.
42-44:✅ Verification successful
Verify all lighting configuration flags are properly handled.
The introduction of multiple lighting-related flags (
legacyLighting,clientSideLighting, andflyingSquidWorkarounds) requires thorough testing to ensure these flags work correctly in all combinations.
🏁 Script executed:
#!/bin/bash # Search for all places where these new flags are used to ensure consistent implementation echo "Checking legacyLighting usage:" rg -A 3 "legacyLighting" --glob "*.{ts,tsx,js,jsx}" echo -e "\nChecking clientSideLighting usage:" rg -A 3 "clientSideLighting" --glob "*.{ts,tsx,js,jsx}" echo -e "\nChecking flyingSquidWorkarounds usage:" rg -A 3 "flyingSquidWorkarounds" --glob "*.{ts,tsx,js,jsx}"Length of output: 7369
All lighting flags usage verified
I’ve confirmed that
legacyLighting,clientSideLighting, andflyingSquidWorkaroundsare consistently referenced and handled across the codebase; no missing implementations were detected.Key locations checked:
- src/watchOptions.ts – assignment based on UI options and server capabilities
- renderer/viewer/three/worldrendererThree.ts – branching for ambient and directional light intensities
- renderer/viewer/lib/worldDataEmitter.ts – conditional block light updates
- src/react/DebugOverlay.tsx – debug info for each lighting mode
- renderer/viewer/lib/mesher/ – legacy and Flying Squid workaround flags applied
With all flag combinations accounted for, this review comment can be resolved.
1000-1000:❓ Verification inconclusive
Ensure proper cleanup of resources.
It's good that you're calling
this.displayOptions.worldView.destroy()to clean up resources, but we should verify that the destroy method is implemented properly in the WorldDataEmitter class.
🏁 Script executed:
#!/bin/bash # Check if WorldDataEmitter has a destroy method echo "Checking for WorldDataEmitter.destroy implementation:" rg -A 10 "destroy\(\)" --glob "*worldDataEmitter.ts"Length of output: 171
#!/bin/bash # Locate files named WorldDataEmitter.ts (case-sensitive) echo "Locating WorldDataEmitter files:" fd WorldDataEmitter.ts # If found, inspect for a destroy method files=$(fd WorldDataEmitter.ts) if [ -n "$files" ]; then echo -e "\nInspecting for destroy method in file(s):" for f in $files; do echo -e "\n--- $f ---" rg -n "destroy(" -C 5 "$f" || echo "No destroy method found in $f" done else # Fallback: search by class name echo -e "\nNo WorldDataEmitter.ts file found. Searching for class definition:" rg -n "class WorldDataEmitter" -R || echo "No class WorldDataEmitter definition found." # Finally search for any destroy method in repository echo -e "\nListing all destroy method definitions repository-wide:" rg -n "destroy(" -R || echo "No destroy methods found in repo" fi
#!/bin/bash # 1. Locate any WorldDataEmitter definitions echo "Searching for WorldDataEmitter class definitions..." rg -n "class WorldDataEmitter" -R # 2. Search for any destroy method implementations echo -e "\nSearching for destroy method definitions across the repo..." rg -n "destroy\s*\(" -R # 3. Inspect worldView usage to determine its type/location echo -e "\nLocating worldView references and its type declaration:" rg -n "worldView" -R
Verify
worldView.destroy()Is Fully ImplementedPlease ensure that calling
this.displayOptions.worldView.destroy()truly cleans up all associated resources. In particular:
- Locate the class or module that defines
worldViewand confirm adestroy()method exists.- Inside that method, verify it unsubscribes from any events, stops timers/animations, and releases data or GPU resources.
- If no such method is found or it’s incomplete, add or extend a proper
destroy()implementation to avoid memory leaks.
| if (options.dayCycleAndLighting) { | ||
| delete options.dayCycleAndLighting | ||
| options.dayCycle = options.dayCycleAndLighting | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid the delete operator for better performance.
Using the delete operator can impact performance as it forces a deoptimization of the underlying object.
if (options.dayCycleAndLighting) {
- delete options.dayCycleAndLighting
+ options.dayCycleAndLighting = undefined
options.dayCycle = options.dayCycleAndLighting
}📝 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.
| if (options.dayCycleAndLighting) { | |
| delete options.dayCycleAndLighting | |
| options.dayCycle = options.dayCycleAndLighting | |
| } | |
| if (options.dayCycleAndLighting) { | |
| options.dayCycleAndLighting = undefined | |
| options.dayCycle = options.dayCycleAndLighting | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 185-185: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🤖 Prompt for AI Agents
In src/optionsStorage.ts around lines 184 to 187, avoid using the delete
operator on the options.dayCycleAndLighting property to improve performance.
Instead of deleting the property, assign undefined or null to
options.dayCycleAndLighting to effectively remove its value without deoptimizing
the object. Adjust the code to first assign options.dayCycleAndLighting to
options.dayCycle, then set options.dayCycleAndLighting to undefined or null.
| dayCycle: true, | ||
| // experimentalLighting: IS_BETA_TESTER, | ||
| experimentalLightingV1: location.hostname.startsWith('lighting.'), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve transition from legacy lighting system.
The code shows a transition from legacy options to the new lighting system, but the commented out code and the conditional flag for experimental lighting make the implementation confusing to follow.
Consider cleaning up this transition:
dayCycle: true,
-// experimentalLighting: IS_BETA_TESTER,
-experimentalLightingV1: location.hostname.startsWith('lighting.'),
+// V1 of the experimental lighting system is now available to beta testers
+experimentalLightingV1: IS_BETA_TESTER || location.hostname.startsWith('lighting.'),📝 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.
| dayCycle: true, | |
| // experimentalLighting: IS_BETA_TESTER, | |
| experimentalLightingV1: location.hostname.startsWith('lighting.'), | |
| dayCycle: true, | |
| // V1 of the experimental lighting system is now available to beta testers | |
| experimentalLightingV1: IS_BETA_TESTER || location.hostname.startsWith('lighting.'), |
🤖 Prompt for AI Agents
In src/optionsStorage.ts around lines 51 to 53, the presence of commented out
legacy code and a conditional flag for experimental lighting creates confusion
in the transition to the new lighting system. Remove the commented out legacy
option and simplify the flags by clearly defining only the new lighting system
options. Ensure the code is clean and straightforward to improve readability and
maintainability.
| @@ -632,7 +623,8 @@ export abstract class WorldRendererCommon<WorkerSend = any, WorkerReceive = any> | |||
| x, | |||
| z, | |||
| chunk, | |||
| customBlockModels: customBlockModels || undefined | |||
| customBlockModels: customBlockModels || undefined, | |||
| lightData: dumpLightData(x, z) | |||
There was a problem hiding this comment.
Fix commented-out light data dumping.
The dumpLightData function is currently not returning any data because its return statement is commented out, as seen in the relevant code snippet from lightEngine.ts. This will result in light data always being undefined.
#!/bin/bash
# Verify the implementation of dumpLightData function
echo "Current dumpLightData implementation:"
rg -A 5 "dumpLightData" --glob "*.{ts,tsx,js,jsx}"🤖 Prompt for AI Agents
In renderer/viewer/lib/worldrendererCommon.ts at line 627, the call to
dumpLightData(x, z) results in undefined because the dumpLightData function's
return statement is commented out in lightEngine.ts. To fix this, locate the
dumpLightData function implementation in lightEngine.ts, uncomment the return
statement so it properly returns the light data, and then verify that the
function returns the expected data structure to be used here.
| @@ -902,7 +892,7 @@ | |||
| this.reactiveState.world.mesherWork = true | |||
| const distance = this.getDistance(pos) | |||
| // todo shouldnt we check loadedChunks instead? | |||
| if (!this.workers.length || distance[0] > this.viewDistance || distance[1] > this.viewDistance) return | |||
| // if (!this.workers.length || distance[0] > this.viewDistance || distance[1] > this.viewDistance) return | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Performance concern: Commented out early return for distant chunks.
The commented-out early return would have prevented processing chunks outside the view distance. Without this check, the system may process sections that are not visible, potentially impacting performance.
🏁 Script executed:
#!/bin/bash
# Check for any alternative distance checks that might replace this one
echo "Looking for alternative distance checks for sections outside view distance:"
rg -C 3 "viewDistance" --glob "*setSectionDirty*"
rg -C 3 "distance" --glob "*setSectionDirty*"Length of output: 554
🏁 Script executed:
#!/bin/bash
# Show all instances of viewDistance in the file to see if there’s an alternative check
echo "Matches for viewDistance in renderer/viewer/lib/worldrendererCommon.ts:"
rg -n "viewDistance" renderer/viewer/lib/worldrendererCommon.ts
# Extract code context around the commented-out early return (lines ~850–950)
echo -e "\nContext around the commented-out return (lines 850–950):"
sed -n '850,950p' renderer/viewer/lib/worldrendererCommon.tsLength of output: 4533
Restore distance-based early return in setSectionDirty to avoid processing out-of-range sections
The commented-out guard in renderer/viewer/lib/worldrendererCommon.ts means every chunk outside the viewDistance still gets dispatched, which can severely impact performance. No other distance check replaces it.
• Location:
– File: renderer/viewer/lib/worldrendererCommon.ts
– Method: setSectionDirty (around line 895)
• Suggested fix:
Re-enable (or replace with equivalent) the early return using the computed distance and this.viewDistance, for example:
const distance = this.getDistance(pos)
- // if (!this.workers.length || distance[0] > this.viewDistance || distance[1] > this.viewDistance) return
+ if (!this.workers.length
+ || distance[0] > this.viewDistance
+ || distance[1] > this.viewDistance) {
+ return
+ }This will prevent dispatching “dirty” messages for chunks the user cannot see.
📝 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.
| // if (!this.workers.length || distance[0] > this.viewDistance || distance[1] > this.viewDistance) return | |
| const distance = this.getDistance(pos) | |
| if (!this.workers.length | |
| || distance[0] > this.viewDistance | |
| || distance[1] > this.viewDistance) { | |
| return | |
| } |
🤖 Prompt for AI Agents
In renderer/viewer/lib/worldrendererCommon.ts at line 895, the early return that
prevents processing chunks outside the viewDistance is commented out, causing
unnecessary processing and performance degradation. Restore this early return by
uncommenting the line that checks if there are no workers or if the chunk's
distance exceeds the viewDistance, and return early to skip processing those
chunks.
fixes #245
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation