feat: Finally a light engine! #304
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
WalkthroughThis update adds a new dependency ( Changes
Sequence Diagram(s)sequenceDiagram
participant WDE as WorldDataEmitter
participant LE as LightEngine
Note over WDE,LE: Initialization
WDE->>LE: createLightEngine()
LE-->>WDE: Light engine instance
Note over WDE,LE: Chunk Loading Flow
WDE->>LE: processLightChunk(x, z)
LE-->>WDE: Chunk light update results
Note over WDE,LE: Block Update Flow
WDE->>LE: updateBlockLight(x, y, z, stateId)
LE-->>WDE: Light state updated
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
/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.
| export const updateBlockLight = (x: number, y: number, z: number, stateId: number) => { | ||
| const engine = getLightEngine() | ||
| engine.setBlock(x, y, z, convertPrismarineBlockToWorldBlock(mcData.blocks[stateId], loadedData)) | ||
| } |
There was a problem hiding this comment.
Missing loadedData variable in updateBlockLight
The loadedData variable is used but not defined or imported in this file.
export const updateBlockLight = (x: number, y: number, z: number, stateId: number) => {
const engine = getLightEngine()
+ // Get the loadedData from the appropriate source
+ const loadedData = globalThis.viewer?.displayOptions?.loadedData; // Adjust this to the actual source
engine.setBlock(x, y, z, convertPrismarineBlockToWorldBlock(mcData.blocks[stateId], loadedData))
}📝 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 = (x: number, y: number, z: number, stateId: number) => { | |
| const engine = getLightEngine() | |
| engine.setBlock(x, y, z, convertPrismarineBlockToWorldBlock(mcData.blocks[stateId], loadedData)) | |
| } | |
| export const updateBlockLight = (x: number, y: number, z: number, stateId: number) => { | |
| const engine = getLightEngine() | |
| // Get the loadedData from the appropriate source | |
| const loadedData = globalThis.viewer?.displayOptions?.loadedData; // Adjust this to the actual source | |
| engine.setBlock(x, y, z, convertPrismarineBlockToWorldBlock(mcData.blocks[stateId], loadedData)) | |
| } |
| export const createLightEngine = (world: WorldRendererCommon) => { | ||
| lightEngine = createLightEngineForSyncWorld(world.displayOptions.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 | ||
| } |
There was a problem hiding this comment.
Light engine creation function needs proper world parameter handling
The implementation of createLightEngine is missing some important context. The function takes a "world" parameter of type WorldRendererCommon, but then tries to access world.displayOptions.worldView.world which might not match the expected structure.
Also, the loadedData variable is used but not defined.
export const createLightEngine = (world: WorldRendererCommon) => {
- lightEngine = createLightEngineForSyncWorld(world.displayOptions.worldView.world as unknown as world.WorldSync, loadedData, {
+ // Get world and loaded data from the appropriate sources
+ const worldView = world.displayOptions.worldView;
+ const loadedData = world.displayOptions.loadedData; // Adjust this to the actual source of loaded data
+ 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
}📝 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 createLightEngine = (world: WorldRendererCommon) => { | |
| lightEngine = createLightEngineForSyncWorld(world.displayOptions.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 | |
| } | |
| export const createLightEngine = (world: WorldRendererCommon) => { | |
| // Get world and loaded data from the appropriate sources | |
| const worldView = world.displayOptions.worldView; | |
| const loadedData = world.displayOptions.loadedData; // Adjust this to the actual source of loaded data | |
| 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 | |
| } |
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.
Summary by CodeRabbit