Worldgen savings #497
Worldgen savings #497
Conversation
WalkthroughThread-safe, palette-driven containers were introduced for chunk data with new constructors, nullable DataArray, and serialization paths. Biome/block containers now delegate to a common base. Heightmap set reduced to MotionBlocking; networking writes only that heightmap. Palettes simplified with fixed global bit counts. Region (de)serialization initializes arrays explicitly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant DC as DataContainer<T>
participant Pal as Palette<T>
participant DA as DataArray?
rect rgba(230,240,255,0.5)
Note over DC: Thread-safe Set
Caller->>DC: Set(x,y,z, value)
activate DC
DC->>DC: lock(storageLock)
DC->>Pal: GetOrAddId(value)
alt DataArray is null and BitsPerEntry > 0
DC->>DC: InitializeDataArray()
DC->>DA: create storage
end
DC->>DA: Set(blockIndex, paletteId)
deactivate DC
end
sequenceDiagram
autonumber
participant Server as LevelChunkWithLightPacket
participant Chunk as Chunk
participant HM as Heightmaps
participant Writer as NetWriter
Server->>Chunk: Access Heightmaps[MotionBlocking]
Chunk-->>Server: MotionBlocking data (long[])
Server->>Writer: Write hash(MotionBlocking)
Server->>Writer: WriteLengthPrefixedArray(long[])
Note right of Writer: Only MotionBlocking heightmap serialized
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Obsidian/ChunkData/BiomeContainer.cs (1)
17-24: Missing null check for DataArray in serialization.
WriteToaccessesthis.DataArray.storagedirectly (line 23), butDataArrayis now nullable per the base class changes. With single-value palettes (whenBitsPerEntry == 0),DataArraywill benull, causing a NullReferenceException during serialization.Apply this diff to add a null check:
public override void WriteTo(INetStreamWriter writer) { writer.WriteByte(this.BitsPerEntry); this.Palette.WriteTo(writer); - writer.WriteLongArray(this.DataArray.storage); + if (this.DataArray != null) + writer.WriteLongArray(this.DataArray.storage); }Obsidian.API/ChunkData/DataContainer.cs (1)
33-39: Missing null check in GrowDataArray.
GrowDataArrayaccessesDataArray.BitsPerEntry(line 35) without checking ifDataArrayis null. SinceDataArrayis now nullable and can benullwhen using single-value palettes (BitsPerEntry == 0), this will cause aNullReferenceExceptionif called with a nullDataArray.Apply this diff to add a null check:
public void GrowDataArray() { - if (Palette.BitCount <= DataArray.BitsPerEntry) + if (DataArray == null || Palette.BitCount <= DataArray.BitsPerEntry) return; DataArray = DataArray.Grow(Palette.BitCount); }Obsidian/ChunkData/BlockStateContainer.cs (1)
31-90: Null reference exception when DataArray is null.
GetNonAirBlocks()accessesDataArray[i]on lines 45, 75, and 85 without checking ifDataArrayis null. WhenPalette.BitCount == 0(single-value palette),DataArrayis null, causing a null reference exception.Apply this diff to handle the null
DataArraycase:private short GetNonAirBlocks() { + // Single-value palette: all blocks are the same. + if (this.DataArray is null) + { + if (Palette.TryGetId(BlocksRegistry.Air, out _) || + Palette.TryGetId(BlocksRegistry.CaveAir, out _) || + Palette.TryGetId(BlocksRegistry.VoidAir, out _)) + { + return 0; // All blocks are air variants + } + return MaxEntryCount; // All blocks are non-air + } + int validBlocksCount = 0; if (!Palette.TryGetId(BlocksRegistry.Air, out var indexOne))
🧹 Nitpick comments (1)
Obsidian/ChunkData/BiomeContainer.cs (1)
11-15: Redundant property assignments in constructor.The private constructor delegates to
base(64, palette)which already sets thePaletteproperty in the base class, but then reassignsPalette = palettein the body. This is redundant.Apply this diff to remove the redundant assignment:
-private BiomeContainer(IPalette<Biome> palette, DataArray dataArray) : base(64, palette) -{ - Palette = palette; - DataArray = dataArray; -} +private BiomeContainer(IPalette<Biome> palette, DataArray dataArray) : base(64, palette) +{ + DataArray = dataArray; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
Obsidian.API/ChunkData/DataContainer.cs(2 hunks)Obsidian.API/ChunkData/Palettes/BaseIndirectPalette.cs(3 hunks)Obsidian.API/_Enums/HeightmapType.cs(1 hunks)Obsidian/ChunkData/BiomeContainer.cs(1 hunks)Obsidian/ChunkData/BlockStateContainer.cs(4 hunks)Obsidian/ChunkData/ChunkSection.cs(1 hunks)Obsidian/ChunkData/DataContainer.cs(0 hunks)Obsidian/ChunkData/GlobalBiomePalette.cs(1 hunks)Obsidian/ChunkData/GlobalBlockStatePalette.cs(1 hunks)Obsidian/ChunkData/Heightmap.cs(0 hunks)Obsidian/ChunkData/IndirectPalette.cs(1 hunks)Obsidian/ChunkData/InternalIndirectPalette.cs(1 hunks)Obsidian/ChunkData/Palette.cs(1 hunks)Obsidian/Net/Packets/Play/Clientbound/LevelChunkWithLightPacket.cs(2 hunks)Obsidian/WorldData/Chunk.cs(1 hunks)Obsidian/WorldData/Generators/Overworld/ChunkBuilder.cs(2 hunks)Obsidian/WorldData/Region.cs(5 hunks)Obsidian/WorldData/World.cs(1 hunks)
💤 Files with no reviewable changes (2)
- Obsidian/ChunkData/Heightmap.cs
- Obsidian/ChunkData/DataContainer.cs
🧰 Additional context used
🧬 Code graph analysis (12)
Obsidian/WorldData/World.cs (2)
Obsidian/WorldData/Generators/Overworld/ChunkBuilder.cs (1)
Heightmaps(278-337)Obsidian/Utilities/NumericsHelper.cs (1)
NumericsHelper(5-19)
Obsidian/WorldData/Region.cs (6)
Obsidian.Nbt/NbtCompound.cs (7)
NbtCompound(8-153)NbtCompound(22-28)NbtCompound(30-34)NbtCompound(36-40)GetInt(75-75)TryGetTag(46-46)TryGetTag(47-57)Obsidian/ChunkData/GlobalBiomePalette.cs (1)
GetOrAddId(16-16)Obsidian/Registries/BlocksRegistry.cs (2)
BlocksRegistry(6-165)BlocksRegistry(11-30)Obsidian/ChunkData/BlockStateContainer.cs (4)
BlockStateContainer(3-95)BlockStateContainer(10-10)BlockStateContainer(12-16)BlockStateContainer(92-92)Obsidian.API/ChunkData/DataContainer.cs (1)
InitializeDataArray(41-47)Obsidian/ChunkData/BiomeContainer.cs (4)
BiomeContainer(3-29)BiomeContainer(9-9)BiomeContainer(11-15)BiomeContainer(26-26)
Obsidian/ChunkData/IndirectPalette.cs (2)
Obsidian.API/ChunkData/Palettes/BaseIndirectPalette.cs (3)
BaseIndirectPalette(5-103)BaseIndirectPalette(11-15)BaseIndirectPalette(17-22)Obsidian/ChunkData/GlobalBlockStatePalette.cs (1)
IBlock(19-19)
Obsidian/Net/Packets/Play/Clientbound/LevelChunkWithLightPacket.cs (3)
Obsidian/WorldData/Chunk.cs (3)
Chunk(6-251)Chunk(22-43)Chunk(45-52)Obsidian/WorldData/Generators/Overworld/ChunkBuilder.cs (1)
Heightmaps(278-337)Obsidian.API/_Interfaces/INetStreamWriter.cs (1)
WriteLong(19-19)
Obsidian/ChunkData/GlobalBiomePalette.cs (1)
Obsidian.API/Registries/CodecRegistry.cs (1)
CodecRegistry(8-18)
Obsidian/ChunkData/GlobalBlockStatePalette.cs (2)
Obsidian/ChunkData/IndirectPalette.cs (2)
IPalette(24-29)IBlock(16-22)Obsidian/ChunkData/Palette.cs (2)
IPalette(5-14)IPalette(16-21)
Obsidian.API/ChunkData/DataContainer.cs (5)
Obsidian.API/ChunkData/Palettes/BaseIndirectPalette.cs (4)
T(24-24)IPalette(77-80)GetOrAddId(51-75)WriteTo(82-96)Obsidian/ChunkData/InternalIndirectPalette.cs (2)
T(16-25)IPalette(27-32)Obsidian/ChunkData/BlockStateContainer.cs (2)
GetIndex(94-94)WriteTo(18-29)Obsidian/ChunkData/BiomeContainer.cs (2)
GetIndex(28-28)WriteTo(17-24)Obsidian.API/_Interfaces/INetStreamWriter.cs (1)
WriteLongArray(28-28)
Obsidian/ChunkData/InternalIndirectPalette.cs (2)
Obsidian.API/ChunkData/DataContainer.cs (1)
T(68-80)Obsidian.API/ChunkData/Palettes/BaseIndirectPalette.cs (4)
T(24-24)BaseIndirectPalette(5-103)BaseIndirectPalette(11-15)BaseIndirectPalette(17-22)
Obsidian/ChunkData/BiomeContainer.cs (4)
Obsidian/WorldData/Chunk.cs (1)
Biome(65-74)Obsidian/ChunkData/ChunkSection.cs (2)
Biome(44-44)Biome(95-95)Obsidian/ChunkData/GlobalBiomePalette.cs (2)
Biome(18-18)IPalette(20-20)Obsidian/ChunkData/Palette.cs (2)
IPalette(5-14)IPalette(16-21)
Obsidian/ChunkData/Palette.cs (4)
Obsidian/ChunkData/GlobalBiomePalette.cs (3)
IPalette(20-20)Biome(18-18)GlobalBiomePalette(3-25)Obsidian/ChunkData/GlobalBlockStatePalette.cs (3)
IPalette(21-21)IBlock(19-19)GlobalBlockStatePalette(3-24)Obsidian/ChunkData/IndirectPalette.cs (5)
IPalette(24-29)IBlock(16-22)IndirectPalette(6-30)IndirectPalette(8-10)IndirectPalette(12-14)Obsidian/ChunkData/InternalIndirectPalette.cs (4)
IPalette(27-32)InternalIndirectPalette(6-33)InternalIndirectPalette(8-10)InternalIndirectPalette(12-14)
Obsidian/WorldData/Chunk.cs (1)
Obsidian/ChunkData/ChunkSection.cs (3)
ChunkSection(5-102)ChunkSection(24-33)ChunkSection(35-40)
Obsidian/ChunkData/BlockStateContainer.cs (5)
Obsidian.API/ChunkData/Palettes/BaseIndirectPalette.cs (2)
IPalette(77-80)WriteTo(82-96)Obsidian/ChunkData/GlobalBlockStatePalette.cs (3)
IPalette(21-21)IBlock(19-19)WriteTo(23-23)Obsidian/ChunkData/IndirectPalette.cs (2)
IPalette(24-29)IBlock(16-22)Obsidian/ChunkData/Palette.cs (3)
IPalette(5-14)IPalette(16-21)Palette(3-22)Obsidian.API/ChunkData/DataContainer.cs (1)
WriteTo(82-90)
🔇 Additional comments (61)
Obsidian/Net/Packets/Play/Clientbound/LevelChunkWithLightPacket.cs (3)
14-20: Good memory optimization by reducing heightmap data.The change to send only the MotionBlocking heightmap aligns well with the PR's memory savings objectives. The comment clearly documents this intentional reduction.
33-33: ConfirmedSizereturnsint; cast removal is safe
TheSizeproperty inNetworkBufferis defined aspublic int Size => size;, so passing it directly toWriteVarIntis correct.
16-17: No changes required forGetDataArray()return type or dictionary access
GetDataArray()returnslong[], and theHeightmapsdictionary is always seeded with theMotionBlockingkey, so using the indexer here is safe.Obsidian/ChunkData/GlobalBlockStatePalette.cs (4)
3-3: LGTM: Parameterless constructor simplifies instantiation.The addition of a parameterless constructor aligns with the PR's goal to simplify palette initialization and reduce configurability overhead.
6-6: LGTM: Fixed BitCount is correct for global palette.Initializing
BitCountto 15 is appropriate for a global block state palette, as it supports up to 32,768 unique block states, which covers the full range of Minecraft block states. This change eliminates runtime configurability and aligns with the memory savings objective.
3-3: LGTM: Parameterless constructor added.The primary constructor syntax is appropriate here and aligns with the simplified palette initialization pattern across the PR.
6-6: GlobalBlockStatePalette.BitCount remains hard-coded to 15 but is never referenced elsewhere in the codebase; please confirm against the Minecraft 1.21.x specification that 15 bits (32 768 entries) is the correct fixed size for the global block state palette.Obsidian/WorldData/World.cs (2)
181-181: LGTM: Simplified fluent call improves readability.The refactor to a single chained call is cleaner and maintains the same behavior. The use of
HeightmapType.WorldSurfacealigns with the reduced heightmap set introduced in this PR.
181-181: LGTM: Refactored to fluent call.The collapsed expression is more concise and maintains the same logic. The null-conditional operator correctly handles the case where the chunk is not available.
Obsidian.API/ChunkData/Palettes/BaseIndirectPalette.cs (2)
86-90: LGTM: Single-value palette optimization is a key memory savings.This special case for
BitCount == 0is central to the PR's memory savings goal. When a chunk section consists of a single block type, serialization now writes only that single value instead of a count and a loop, reducing both memory and network overhead. The logic is correct: for a single-value palette,values[0]holds the sole entry.
92-95: LGTM: Standard palette serialization path retained.The existing serialization path for multi-value palettes remains unchanged and correctly writes the count followed by all palette entries.
Obsidian/WorldData/Generators/Overworld/ChunkBuilder.cs (2)
287-287: LGTM: OceanFloor heightmap removed.Commenting out the
OceanFloorheightmap assignment aligns with the PR's goal to reduce memory usage by limiting the active heightmap set. This change is consistent with the removal ofOceanFloorfrom theHeightmapTypeenum.
326-327: LGTM: MotionBlockingNoLeaves heightmap removed.Commenting out the
MotionBlockingNoLeavesheightmap operation reduces memory overhead. The boolean flagmotionBlockingLeavesSetis still updated, preserving the early-exit optimization in the loop. This aligns with the reduced heightmap set in the PR.Obsidian/ChunkData/ChunkSection.cs (1)
24-24: LGTM: Default to single-value palettes is a major memory optimization.Changing the default
bitsPerBlockandbitsPerBiomefrom 4 and 2 to 0 is a key memory savings in this PR. New chunk sections will now start with single-value palettes, minimizing memory usage for uniform or empty sections. As blocks/biomes are added, the palettes will expand dynamically as needed. This aligns perfectly with the PR's goal to reduce memory usage in world generation.Obsidian/ChunkData/GlobalBiomePalette.cs (2)
6-6: LGTM: Direct initialization simplifies palette construction.The removal of the constructor parameter and direct initialization of
BitCountfromCodecRegistry.Biomes.GlobalBitsPerEntryeliminates runtime configurability in favor of a consistent, compile-time-known value. This aligns with the PR's memory optimization goals.
6-6: LGTM: BitCount initialized from registry.Using
CodecRegistry.Biomes.GlobalBitsPerEntryis a good approach as it centralizes the configuration and makes it easier to maintain compared to a hardcoded value. The removal of the constructor parameter aligns with the PR's goal to simplify palette initialization.Obsidian/ChunkData/InternalIndirectPalette.cs (3)
1-1: LGTM: Correct namespace import for base palette.The
using Obsidian.API.ChunkData.Palettesdirective is necessary for theBaseIndirectPalette<T>type reference.
6-6: LGTM: Redundant interface declaration removed.Removing explicit
IPalette<T>from the inheritance list is correct sinceBaseIndirectPalette<T>already implements this interface. This simplifies the declaration without changing behavior, mirroring the same refactoring applied toIndirectPalette.
1-6: LGTM!The inheritance change correctly delegates
IPalette<T>implementation toBaseIndirectPalette<T>, simplifying the class signature while maintaining functionality.Obsidian.API/_Enums/HeightmapType.cs (3)
11-11: Confirmed no active references to MotionBlockingNoLeaves
All occurrences are in commented-out code; removal is safe.
7-8: OceanFloor enum removal — only commented references remain.
No active usages; commented occurrences at Obsidian/WorldData/Region.cs:338, Obsidian/WorldData/Chunk.cs:30, Obsidian/WorldData/Generators/Overworld/ChunkBuilder.cs:287. Safe to remove from the public API. Remove or update those commented lines to avoid confusion.
7-11: No active references to the removed heightmap types remain. All found occurrences are commented-out, so there are no dangling enum references to fix.Obsidian/WorldData/Chunk.cs (4)
36-40: LGTM! Single-value palette optimization applied.The constructor now defaults to
bitsPerBlock=0andbitsPerBiome=0, enabling single-value palettes for chunk sections. This is a key memory optimization mentioned in the PR objectives, as sections with uniform blocks will use minimal storage.
27-34: No active dependencies on OceanFloor or MotionBlockingNoLeaves. Instantiation inChunk.csand generation logic inChunkBuilder.csare both commented out; no other code references these types.
27-34: LGTM! Heightmap reduction aligns with enum changes.The commented-out heightmap entries (OceanFloor, MotionBlockingNoLeaves) align with the updated
HeightmapTypeenum, reducing memory overhead by maintaining only the active heightmap types.
39-39: LGTM! Default zero-bit initialization supports memory savings.The updated constructor call
new ChunkSection(yBase: i - 4)defaults to 0 bits per block/biome, enabling single-value palettes for uniform sections. This aligns with the PR's memory-saving objectives.Obsidian/WorldData/Region.cs (10)
2-2: LGTM!The using directive is necessary for palette type references in the serialization/deserialization logic.
184-192: LGTM!The explicit cast to
NbtCompoundproperly handles the NBT list enumeration. The null-forgiving operator is safe given theTryGetTagcheck on line 184.
194-200: LGTM! Nullable DataArray pattern correctly implemented.The code properly initializes the DataArray only when needed. For single-value palettes (BitCount==0),
InitializeDataArray()returns early, leaving DataArray null. The subsequent check on line 194 ensures the data array is only assigned when present in NBT.
213-219: LGTM!Consistent nullable DataArray pattern for biome containers.
289-291: LGTM! Critical null check for single-value palettes.The null check is essential for single-value palettes where DataArray remains null to save memory. This prevents serialization errors when writing chunk sections with uniform block states.
2-2: LGTM!Adding the
Obsidian.API.ChunkData.Palettesnamespace aligns with the palette refactor across the codebase.
187-191: LGTM! Explicit cast clarifies NbtList element type.The explicit cast to
NbtCompoundensures type safety when iterating over the palette entries, making the deserialization logic more robust.
289-291: LGTM! Null check correctly guards DataArray write.The null check ensures that
DataArrayis only written when present, supporting the single-value palette optimization whereDataArrayis null for uniform sections.
310-311: LGTM! Null check correctly guards biome DataArray write.The null check ensures that biome
DataArrayis only written when present, aligning with the single-value palette optimization.
194-200: No guard needed: data tag always matches non-zero BitCount
The NBT writer only emits thedatatag whenDataArray != null(i.e.BitsPerEntry > 0), andInitializeDataArray()will allocateDataArrayin that case—so adatatag can never coincide withBitCount == 0.Obsidian/ChunkData/BlockStateContainer.cs (12)
5-5: LGTM!The named constant improves readability and maintainability.
10-16: LGTM! Constructors support palette optimization.The constructors properly support the single-value palette pattern:
- Default constructor uses
DetermineBlockPalette()to select appropriate palette type- Private constructor enables efficient cloning with shared palette semantics
27-29: LGTM! Required for single-value palette serialization.The null check prevents errors when serializing sections with single-value palettes (where DataArray is null to save memory).
43-49: LGTM!Loop bounds correctly use
MaxEntryCountconstant for all three air-type checking paths.
73-79: LGTM!Consistent use of
MaxEntryCountin the single-index path.
83-89: LGTM!Consistent use of
MaxEntryCountin the two-index path.
5-5: LGTM!Defining
MaxEntryCountas a constant improves code maintainability and clarity.
8-8: LGTM! IsEmpty logic aligns with palette-driven design.Checking
Palette.Count == 0correctly identifies empty containers, as an empty palette indicates no blocks have been added beyond the default Air block.
10-16: LGTM! Constructor refactor supports single-value palette optimization.The updated constructors align with the palette-driven container design, enabling memory savings through single-value palettes and nullable
DataArray.
27-28: LGTM! Null check correctly supports single-value palette optimization.The null check ensures
DataArrayis only written when present, saving memory for uniform sections withBitCount == 0.
43-43: LGTM! Loop bounds correctly use MaxEntryCount constant.The loop bounds correctly iterate over all 4096 entries in the chunk section.
Also applies to: 73-73, 83-83
92-94: LGTM! Clone method correctly uses private constructor.The clone implementation properly copies the palette and data array using the private constructor.
Obsidian.API/ChunkData/DataContainer.cs (7)
8-8: Good use of Lock for thread-safe palette operations.The introduction of
storageLockensures thread-safe access to palette and data array operations inSetandGetmethods. The use of the modernLocktype is appropriate here.
21-31: Constructors correctly implement lazy DataArray allocation.The new constructors properly support single-value palette optimization by only allocating a
DataArraywhenBitsPerEntry != 0(line 25). This aligns well with the PR's memory-saving objectives. ThedefaultValueconstructor variant correctly adds the default value to the palette.
41-47: InitializeDataArray correctly handles single-value palettes.The new
InitializeDataArraymethod appropriately checks ifBitCount == 0before allocating aDataArray, which is essential for the single-value palette optimization.
49-66: Set implementation correctly handles lazy initialization and single-value palettes.The
Setmethod properly:
- Uses locking for thread safety
- Returns early when
BitCount == 0(line 57-58), avoiding unnecessary array operations for single-value palettes- Lazily initializes
DataArraywhen needed (line 60)- Grows the array when the palette expands (line 62)
68-80: Get implementation correctly handles single-value palettes.The
Getmethod properly:
- Uses locking for thread safety
- Returns the single palette value when
BitCount == 0(lines 72-73), which is correct for single-value palettes- Safely accesses the
DataArrayonly after confirming it exists
82-90: WriteTo correctly handles nullable DataArray.The
WriteTomethod appropriately checks ifDataArray != nullbefore serializing it (line 88-89), which is essential for single-value palettes whereDataArraycan benull.
19-19: GetIndex is never called when BitsPerEntry == 0 Verified that DataContainer.Get/Set early-return for zero-entry palettes and all overrides use non-zero shifts; no additional guard required.Obsidian/ChunkData/BiomeContainer.cs (1)
3-29: Inherited Get/Set cover all callers; no changes required
DataContainer defines public virtual Get(int,int,int) and Set(int,int,int,T) that BiomeContainer inherits, and BiomeContainer’s override of GetIndex ensures correct indexing for ChunkSection.GetBiome/SetBiome.Obsidian/ChunkData/Palette.cs (3)
9-9: LGTM: Single-value palette optimization added.The new case for
bitsPerEntry == 0returningIndirectPalette(0)is a key memory optimization for chunk sections consisting of a single block type (as mentioned in the PR objectives). This avoids allocating a full data array for uniform sections.
12-12: LGTM: Consistent with GlobalBlockStatePalette changes.Using the parameterless constructor aligns with the removal of the
bitCountparameter inGlobalBlockStatePalette, centralizing the BitCount value within the palette class itself.
18-20: LGTM: Simplified biome palette selection.The ternary expression is more concise and moves the responsibility for BitCount configuration into the palette classes themselves, improving encapsulation.
Obsidian/ChunkData/IndirectPalette.cs (2)
1-1: LGTM: Added required using directive.The using directive is necessary for the
BaseIndirectPalette<IBlock>base class reference.
6-6: LGTM: Removed redundant interface declaration.Since
BaseIndirectPalette<IBlock>already implementsIPalette<IBlock>, explicitly declaring it again onIndirectPaletteis unnecessary. This simplifies the class declaration without changing behavior.
| public override IPalette<IBlock> Palette { get; internal set; } | ||
|
|
||
| public bool IsEmpty => DataArray.storage.Length == 0; | ||
| public override bool IsEmpty => this.Palette.Count == 0; |
There was a problem hiding this comment.
Verify IsEmpty logic for single-value air palettes.
The IsEmpty check now uses Palette.Count == 0, but a section filled with only air would have Palette.Count == 1 (containing just the air block entry). This means an empty section would incorrectly return IsEmpty = false.
Verify whether:
- The palette can ever have
Count == 0(since air is added by default in ChunkSection constructor line 27-28 of relevant snippet) - The intended behavior for sections containing only air blocks
- Whether
IsEmptyshould check if all entries are air rather than palette size
Consider this logic instead:
-public override bool IsEmpty => this.Palette.Count == 0;
+public override bool IsEmpty => this.Palette.Count == 1 && this.Palette.TryGetId(BlocksRegistry.Air, out _);📝 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.
| public override bool IsEmpty => this.Palette.Count == 0; | |
| public override bool IsEmpty => this.Palette.Count == 1 | |
| && this.Palette.TryGetId(BlocksRegistry.Air, out _); |
🤖 Prompt for AI Agents
In Obsidian/ChunkData/BlockStateContainer.cs around line 8, the IsEmpty getter
uses Palette.Count == 0 which misses the common case where the palette contains
a single air entry; change IsEmpty to return true when the palette is empty OR
when it contains exactly one entry that represents air (e.g. Palette.Count == 1
&& Palette[0].IsAir / Palette[0].Equals(BlockState.Air) or equivalent in your
codebase). Also confirm the constructor behavior that ensures air is added by
default and adjust the check accordingly so sections filled only with air return
IsEmpty = true.
This pr is for trying to get as much memory savings as possible. There were a lot of things with unused data arrays.
Added single value palettes for chunk sections that just consist of 1 block this makes for some more crazy savings. I believe this can be pushed a bit further but as of right now I'll put it on the backburner for when I finish the worldgen-api rework
Summary by CodeRabbit