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
Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
🪧 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 |
Summary by CodeRabbit