fix: new lighting algo#503
Conversation
…. Enable light spread across chunk boundaries
WalkthroughReplaced the monolithic WorldLight with a new async, neighbor-aware Lighting system; added IChunkSection.FillSkyLight and a cached full skylight in ChunkSection; updated generators to use Lighting with IWorld and await neighbor lighting; removed WorldLight; added VS Code launch/tasks for clean/build and world deletion. Changes
Sequence Diagram(s)sequenceDiagram
participant Gen as Chunk Generator
participant Lighting as Lighting
participant World as IWorld
participant Chunk as IChunk
participant Neigh as Neighbor Chunks
Gen->>Chunk: create terrain & sections
Gen->>Lighting: InitialFillSkyLight(chunk)
Lighting->>Chunk: fill skylight sections
Gen->>Lighting: await LightFromNeighbors(chunk, World)
Lighting->>World: request adjacent chunks
World->>Lighting: return neighbor chunks
Lighting->>Neigh: read neighbor edge data
Lighting->>Chunk: PropagateEdgeLightBetweenChunks / SpreadLight
Lighting-->>Gen: neighbor-lighting complete
Gen->>Lighting: await LightToNeighbors(chunk, World)
Lighting->>Neigh: push edge light changes outward
Gen->>Chunk: set chunk lighting status (light/full)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Obsidian/ChunkData/ChunkSection.cs (1)
74-81: Operator precedence bug in block-light branch confirmed—this is a critical issue.The code at line 81 exhibits an operator precedence problem. In C#, the right-shift operator (
>>) has higher precedence than bitwise AND (&). The block-light branch:(blockLight[index] & mask >> shift)is parsed as:
(blockLight[index] & (mask >> shift))This causes
mask >> shiftto execute first, resulting in the wrong nibble being extracted. The sky-light branch correctly uses explicit parentheses:(skyLight[index] & mask) >> shiftThe fix is to add parentheses to the block-light branch to match:
(blockLight[index] & mask) >> shift
🧹 Nitpick comments (4)
.vscode/tasks.json (1)
44-53: Consider cross-platform compatibility fordelete-worldtask.The
rm -rfcommand is Unix-specific and will fail on native Windows command prompt. If Windows support is needed, consider using a cross-platform approach.Option 1 - Use PowerShell (works on Windows/Linux/macOS):
{ "label": "delete-world", "type": "shell", "command": "pwsh", "args": [ "-Command", "Remove-Item -Recurse -Force -ErrorAction SilentlyContinue '${workspaceFolder}/Obsidian.ConsoleApp/bin/Debug/net10.0/worlds'" ], "problemMatcher": [] }Option 2 - Keep as-is if all developers use Unix-like shells (WSL, Git Bash, macOS, Linux).
Obsidian/WorldData/Generators/OverworldGenerator.cs (1)
9-10: Guard againstworldbeing null before lighting
Lighting.LightFromNeighbors(chunk, world)/Lighting.LightToNeighbors(chunk, world)assumeInithas been called andworldis non-null. Today onlyhelperis explicitly checked; ifGenerateChunkAsyncis ever used withoutInit, you’ll now get aNullReferenceExceptionfrom lighting instead of the clearer helper-null message.Consider adding an explicit guard (similar to the helper check), e.g.:
if (world is null) throw new InvalidOperationException("IWorld must not be null. Call Init(world) first.");to fail fast and consistently.
Also applies to: 56-62, 69-73
Obsidian/ChunkData/ChunkSection.cs (1)
21-27: Avoid hard-coded light array size and guard against mismatchedskyLightlengthThe static
FullSkyLightcache is a good optimization, butFillSkyLightcurrently assumesskyLight.Length == 2048. After aSetLightcall,skyLightmay reference an arbitrary array; if its length ever differs,Array.Copy(..., 2048)will throw.You can make this more robust and avoid the magic constant:
private static readonly byte[] FullSkyLight = new byte[2048]; public void FillSkyLight() { if (skyLight.Length != FullSkyLight.Length) skyLight = new byte[FullSkyLight.Length]; Array.Copy(FullSkyLight, skyLight, FullSkyLight.Length); HasSkyLight = true; }This keeps behavior the same for valid inputs but self-recovers from a bad-length
skyLight.Also applies to: 102-106
Obsidian/WorldData/Generators/MojangGenerator.cs (1)
11-15: EnforceInitcontract for_world/_builderto avoid hidden NREs
GenerateChunkAsyncnow depends on both_builderand_worldbeing initialized viaInit(IWorld world), but there’s no guard; misuse will surface asNullReferenceExceptionfrom either_builder.InitialShapeorLighting.LightFromNeighbors(..., _world).To make the usage contract explicit and failures clearer, consider:
public async ValueTask<IChunk> GenerateChunkAsync(...) { if (_builder is null || _world is null) throw new InvalidOperationException("Call Init(world) before generating chunks."); // existing logic... }This matches the pattern used in other generators that enforce their initialization step.
Also applies to: 18-21, 68-74, 79-83
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.vscode/launch.json(1 hunks).vscode/tasks.json(1 hunks)Obsidian.API/ChunkData/IChunkSection.cs(2 hunks)Obsidian/ChunkData/ChunkSection.cs(2 hunks)Obsidian/WorldData/Generators/IslandGenerator.cs(2 hunks)Obsidian/WorldData/Generators/MojangGenerator.cs(2 hunks)Obsidian/WorldData/Generators/Overworld/ChunkBuilder.cs(1 hunks)Obsidian/WorldData/Generators/OverworldGenerator.cs(3 hunks)Obsidian/WorldData/Lighting.cs(1 hunks)Obsidian/WorldData/WorldLight.cs(0 hunks)
💤 Files with no reviewable changes (1)
- Obsidian/WorldData/WorldLight.cs
🧰 Additional context used
🧬 Code graph analysis (4)
Obsidian/WorldData/Generators/MojangGenerator.cs (1)
Obsidian/WorldData/Lighting.cs (2)
Lighting(3-250)InitialFillSkyLight(5-63)
Obsidian/WorldData/Lighting.cs (3)
Obsidian.API/ChunkData/IChunkSection.cs (7)
FillSkyLight(40-40)GetLightLevel(35-35)GetLightLevel(36-36)SetLightLevel(32-32)SetLightLevel(33-33)IBlock(20-20)IBlock(21-21)Obsidian/ChunkData/ChunkSection.cs (7)
FillSkyLight(102-106)GetLightLevel(74-81)GetLightLevel(113-113)SetLightLevel(61-72)SetLightLevel(112-112)IBlock(50-50)IBlock(108-108)Obsidian.API/_Interfaces/IBlock.cs (1)
Is(21-21)
Obsidian/ChunkData/ChunkSection.cs (1)
Obsidian.API/ChunkData/IChunkSection.cs (1)
FillSkyLight(40-40)
Obsidian.API/ChunkData/IChunkSection.cs (2)
Obsidian/ChunkData/ChunkSection.cs (5)
IBlock(50-50)IBlock(108-108)BiomeCodec(52-52)BiomeCodec(109-109)FillSkyLight(102-106)Obsidian/WorldData/Chunk.cs (2)
IBlock(58-67)BiomeCodec(69-78)
🔇 Additional comments (4)
.vscode/launch.json (1)
4-27: LGTM!The updated and new launch configurations are well-structured. The "New World" variant provides a convenient way to test with fresh world data by leveraging the
clean-build-and-delete-worldtask.Obsidian/WorldData/Generators/Overworld/ChunkBuilder.cs (1)
57-67: LGTM!Formatting-only change to the
EmeraldBiomesarray. No functional modifications.Obsidian/WorldData/Lighting.cs (1)
65-82: I attempted to verify the null handling concern by examining howLightFromNeighborsis called throughout the codebase, but the repository clone failed. Without access to the codebase, I cannot definitively determine:
- Whether callers of
LightFromNeighborsvalidate theworldparameter before passing it- Whether null checks for
worldare a standard practice in similar methods- Whether this is an actual gap or if callers guarantee non-null values through implicit contract
Verify null handling for
worldparameter.The
worldparameter is used without null checks at line 69 (world.GetChunkAsync(...)). WhileGetChunkAsyncreturning null is handled, passing a nullworldwould throw aNullReferenceException. Consider verifying whether callers guarantee non-null values or adding a guard clause if null is a possible input.Obsidian.API/ChunkData/IChunkSection.cs (1)
39-41: NewFillSkyLightmethod is a breaking interface changeAdding this member will break any external implementations of
IChunkSection(mods/plugins) until they addFillSkyLight(). Make sure this is called out as a breaking API change (changelog / docs) and that all in-repo implementations are updated.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
Obsidian/WorldData/Lighting.cs (2)
174-205: RecursiveSpreadLightis acceptable given current bounds; iterative refactor remains optional.
SpreadLightis still implemented recursively (Lines 174‑205). With the current constraints (chunk height ~384 and light level ≤ 15), maximum recursion depth is effectively bounded (worst case ~384 for sky light, much less for block light), so stack overflow risk is low in practice.That said, this overlaps with the earlier review concern about recursion vs. an explicit queue/stack. If you expect much taller worlds or more complex propagation in the future, refactoring this to an iterative BFS/DFS would make the algorithm more robust and predictable, but it’s not strictly required for the current bounds.
223-227: Tag lookups in transparency helpers are still linear and sit in hot paths.
IsTransparent/IsSemitransparent/IsOpaque(Lines 223‑227) still useTagsRegistry.Block.*.Entries.Contains(b.RegistryId), whereEntriesis anint[]. As noted in the earlier review, this is an O(n) linear search and these helpers are called very frequently inside the skylight and spread loops.Echoing that prior comment: consider either:
- Changing
EntriesinTag/TagsRegistrytoHashSet<int>(or another O(1) lookup structure), or- Adding a per‑lighting‑pass cache keyed by
RegistryIdthat stores{ isTransparent, isSemitransparent }, so each block type incurs the tag checks only once per chunk lighting run.This would significantly reduce CPU cost during lighting without changing behavior.
🧹 Nitpick comments (2)
Obsidian/WorldData/Lighting.cs (2)
5-62: Verify Y-boundary behavior and consider guarding against out-of-range access.
startYand the uses ofstartY + 1,scanPos + Vector.Up, andHasSurfaceAbovecan produce Y values outside the nominal chunk range:
- If the topmost non-empty section is index 23,
startYis 319 andstartY + 1is 320 (Line 27).- If all sections are empty,
csends at-1, makingstartY-65(Line 20), so the first iteration of the Y loop usesy = -65(Line 28).HasSurfaceAbove(Line 231) usespos + Vector.Up, so forscanPos.Y == 319it will request Y = 320; similarlyHasSurfaceBelowcan hit Y = -65.If
IChunk.GetBlock/GetLightLevel/SetLightLeveldon’t explicitly handle Y outside[-64, 319]as air/empty, this can lead to index errors in the section arrays. If they do handle it, it would be good to document that contract here or add a small guard so the lighting logic doesn’t rely on that implicit behavior.One possible defensive tweak (if needed) is to early‑out when
cs < 0and clampstartYinto the valid range:- int cs; - for (cs = 23; cs >= 0; cs--) + int cs; + for (cs = 23; cs >= 0; cs--) { if (!chunk.Sections[cs].IsEmpty) { break; } chunk.Sections[cs].FillSkyLight(); } - - int startY = ((cs - 4) << 4) + 15; // 4 sections are negative + if (cs < 0) + { + // All sections were empty; they are already filled with skylight above. + return; + } + + int startY = ((cs - 4) << 4) + 15; // 4 sections are negative + if (startY > 319) startY = 319;Please double‑check the
IChunkimplementations and either rely on their clamping semantics intentionally or add explicit guards like above.Also applies to: 229-231
64-98: Avoid callingEnum.GetValues<LightType>()inside the inner edge loops.
PropagateEdgeLightBetweenChunkscallsEnum.GetValues<LightType>()(Line 127 and Line 159) inside the tightx/z/yloops, which will repeatedly allocate/iterate anArrayfor every boundary block. SinceLightTypehas a tiny, fixed set of values, it’s cheaper to cache the set once and reuse it.Consider something like:
internal static class Lighting { + private static readonly LightType[] AllLightTypes = + (LightType[])Enum.GetValues(typeof(LightType)); @@ - foreach (var lt in Enum.GetValues<LightType>()) + foreach (var lt in AllLightTypes) { @@ - foreach (var lt in Enum.GetValues<LightType>()) + foreach (var lt in AllLightTypes) {This keeps the hot edge‑lighting path allocation‑free while preserving behavior.
Also applies to: 100-171
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Obsidian/WorldData/Lighting.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Obsidian/WorldData/Lighting.cs (3)
Obsidian/ChunkData/ChunkSection.cs (7)
FillSkyLight(102-106)GetLightLevel(74-81)GetLightLevel(113-113)SetLightLevel(61-72)SetLightLevel(112-112)IBlock(50-50)IBlock(108-108)Obsidian.API/ChunkData/IChunkSection.cs (7)
FillSkyLight(40-40)GetLightLevel(35-35)GetLightLevel(36-36)SetLightLevel(32-32)SetLightLevel(33-33)IBlock(20-20)IBlock(21-21)Obsidian.API/_Interfaces/IBlock.cs (1)
Is(21-21)
2 participants
Summary by CodeRabbit
Refactor
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.