WorldGen API v4#514
Open
Tides wants to merge 17 commits into
Open
Conversation
github-actions
Bot
added
api
Tides
added
epic
github-actions
Bot
added
commands
Tides
removed
commands
WalkthroughThis PR adds a level-centric world abstraction, introduces shared world/dimension creation through a factory, and migrates world-facing consumers to ChangesNbt Submodule Migration and Build Config
ILevel Abstraction and World/Dimension Refactor
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
github-actions
Bot
added
commands
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
Obsidian/Entities/Entity.cs (1)
361-372: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftCross-level teleport should move the existing entity, not spawn a detached copy. When
to.Level != Level, this removesthisfrom the old level, switchesLevel, then callsLevel.SpawnEntity(to.Position, Type)and drops the returned entity. That path creates and registers a new instance, sothisis never added to the target level and the spawned entity is lost.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Obsidian/Entities/Entity.cs` around lines 361 - 372, Cross-level teleport in Entity.TeleportAsync is spawning a new detached entity instead of moving the current one. In the branch where to.Level != Level, remove this from the old level, update the current entity’s Level to the target, and place/move this entity into the new level rather than calling Level.SpawnEntity and ignoring its return value. Use the existing Entity.TeleportAsync flow and Level.DestroyEntityAsync/SpawnEntity symbols to locate and replace the cross-level transfer logic so the same entity instance is preserved.Obsidian/Events/MainEventHandler.Inventory.cs (1)
319-355: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRegister the thrown item with the level before broadcasting it.
SpawnThrownItemonly sendsAddEntityPacket/SetEntityDataPacket; unlike the other item-spawn paths, it never callsplayer.Level.TryAddEntity(item). That leaves the item out of the level’s entity set, soItemEntity.TickAsyncwon’t run for merging or pickup timing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Obsidian/Events/MainEventHandler.Inventory.cs` around lines 319 - 355, SpawnThrownItem broadcasts the new ItemEntity but never registers it with the level, so it won’t participate in ticking, merging, or pickup timing. Update SpawnThrownItem in MainEventHandler.Inventory.cs to add the created item via player.Level.TryAddEntity(item) before queueing the AddEntityPacket and SetEntityDataPacket, matching the other item spawn paths.Obsidian/Entities/Player.cs (1)
349-368: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPre-existing
Debug.Assertrisk carried over into the Level migration.
Debug.Assert(codec is not null)(Line 359) is a no-op in Release builds; ifCodecRegistry.TryGetDimension(Level.DimensionName, ...)fails to resolve,codec.Idon Line 367 will throw an NRE in production. The// TODO Handle missing codeccomment already acknowledges this gap.This isn't introduced by the World→Level rename itself, but since this block is being touched now, want to flag it — happy to help wire up proper handling (e.g., fallback dimension type or explicit error) if useful.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Obsidian/Entities/Player.cs` around lines 349 - 368, RespawnAsync still relies on Debug.Assert(codec is not null), which won’t protect Release builds if CodecRegistry.TryGetDimension(Level.DimensionName, out var codec) fails. Update the RespawnAsync flow to handle a missing codec explicitly before building the RespawnPacket, using a clear fallback or failing fast with a logged error instead of letting codec.Id throw. Keep the fix localized to RespawnAsync and the codec lookup/packet construction path.Obsidian/Commands/Modules/MainCommandModule.cs (1)
163-171: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
/saveshould handle Dimension-backed levelsthis.Player?.Level is World worldskips the flush when the player is in aDimension, so the command no-ops without feedback. Resolve the owningWorldviaIDimension.ParentWorldhere too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Obsidian/Commands/Modules/MainCommandModule.cs` around lines 163 - 171, The /save command only flushes when this.Player?.Level is a World, so it silently does nothing for Dimension-backed levels. Update SaveAsync in MainCommandModule to resolve the owning World from IDimension.ParentWorld when the player level is a Dimension, then call FlushRegionsAsync on that world as well; keep the existing World path intact and use the current Player/Level checks to locate the level type.
🧹 Nitpick comments (7)
.github/workflows/dotnet.yml (1)
23-25: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueRecursive submodule checkout looks correct.
Matches the Obsidian.slnx reference to
Obsidian.Nbt/Nbt/Obsidian.Nbt.csproj.Static analysis flags that the checkout step doesn't set
persist-credentials: false, which leaves the GitHub token persisted in the local git config for the remainder of the job (artipacked pattern).🔒️ Optional hardening
- uses: actions/checkout@v5 with: submodules: recursive + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/dotnet.yml around lines 23 - 25, The checkout step in the dotnet workflow should be hardened by disabling credential persistence. Update the actions/checkout configuration used for recursive submodule checkout to explicitly set persist-credentials to false so the GitHub token is not left in the local git config. Locate the change in the checkout step that already uses submodules: recursive and add the missing security setting there.Source: Linters/SAST tools
Obsidian/Events/MainEventHandler.World.cs (1)
15-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale variable name after
Levelrename.
worldnow holdsplayer.Level(anILevel), not anIWorld. Consider renaming tolevelfor clarity given the rest of the codebase is migrating to this terminology.♻️ Proposed rename
- var world = player.Level; + var level = player.Level;(and update subsequent references to
worldin this method accordingly)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Obsidian/Events/MainEventHandler.World.cs` at line 15, Rename the local variable in MainEventHandler.World from world to level since it now stores player.Level (an ILevel rather than an IWorld), and update all subsequent references in the same method to match the new name so the terminology stays consistent with the Level migration.Obsidian/WorldData/Region.cs (1)
44-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
LogDebuginstead ofLogInformationfor region-load logging.
AbstractLevel.LoadRegionlogs the equivalent "region loaded/added" event atLogDebug, while this constructor logs atLogInformationfor every region file loaded — this can get noisy as regions load frequently during world traversal.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Obsidian/WorldData/Region.cs` around lines 44 - 45, The region-load message in Region’s constructor is too noisy at LogInformation; change the logger call in the Region file-loading path to LogDebug to match AbstractLevel.LoadRegion and avoid flooding normal logs. Update the logging in the constructor that records the region file and compression so it still includes the same context, just at debug level.Obsidian.API/World/DimensionSettings/DimensionGeneratorBiomeEntry.cs (1)
3-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNon-nullable properties lack initializers — possible CS8618 warnings.
BiomeandParametersare non-nullable reference types without defaults orrequired. Sibling typeDimensionGeneratorBiomeSource.Biomesuses a nullable annotation (List<DimensionGeneratorBiomeEntry>?), indicating nullable reference types are enabled for this project, which would flag these properties at compile time.♻️ Suggested fix
public sealed record class DimensionGeneratorBiomeEntry { - public string Biome { get; set; } + public required string Biome { get; set; } - public DimensionGeneratorBiomeParameters Parameters { get; set; } + public required DimensionGeneratorBiomeParameters Parameters { get; set; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Obsidian.API/World/DimensionSettings/DimensionGeneratorBiomeEntry.cs` around lines 3 - 8, The DimensionGeneratorBiomeEntry properties are non-nullable but have no initialization, which can trigger CS8618. Update Biome and Parameters in DimensionGeneratorBiomeEntry to either be marked required or initialized with safe defaults so the record class satisfies nullable reference type checks; keep the change aligned with the existing nullable annotations used by DimensionGeneratorBiomeSource.Biomes.Obsidian.API/World/DimensionSettings/DimensionGeneratorBiomeParameters.cs (1)
3-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame nullable-initialization concern as
DimensionGeneratorBiomeEntry.All seven
DimensionGeneratorParameterValue-typed properties are non-nullable without defaults/required, inconsistent with the nullable annotation style used elsewhere (e.g.DimensionGeneratorBiomeSource.Biomes).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Obsidian.API/World/DimensionSettings/DimensionGeneratorBiomeParameters.cs` around lines 3 - 18, The seven DimensionGeneratorBiomeParameters properties are declared non-nullable without initialization, which is inconsistent with the nullable style used elsewhere. Update the Offset, Continentalness, Weirdness, Erosion, Depth, Humidity, and Temperature properties in DimensionGeneratorBiomeParameters to be explicitly nullable or required/initialized, matching the pattern used by DimensionGeneratorBiomeEntry and DimensionGeneratorBiomeSource.Biomes. Make the choice consistent with the surrounding model types so callers can tell whether values must be provided or may be absent.Obsidian.API/World/ILevel.cs (2)
83-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNaming nit:
GetWorldSurfaceHeightAsyncstill uses "World" terminology.Given the rest of the interface has been renamed to Level-centric naming (
ILevel,LevelData, etc.), considerGetLevelSurfaceHeightAsyncfor consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Obsidian.API/World/ILevel.cs` at line 83, The interface method name still uses World terminology, so update ILevel’s GetWorldSurfaceHeightAsync to a Level-centric name like GetLevelSurfaceHeightAsync for consistency with the surrounding API naming. Make sure any implementation, call sites, and related references to GetWorldSurfaceHeightAsync are renamed together so the Level/LevelData naming stays uniform.
32-32: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider not exposing the raw
ConcurrentDictionaryforPlayers.Returning the live
ConcurrentDictionary<Guid, IPlayer>lets callers mutate it directly (add/remove/clear), bypassingTryAddPlayer/TryRemovePlayer(lines 69, 86), which per downstream usage (e.g.Player.TeleportAsynccallingLevel.TryRemovePlayer/w.TryAddPlayer) are the intended controlled mutation path. ExposingIReadOnlyDictionary<Guid, IPlayer>instead would enforce that invariant at the type level.♻️ Suggested change
- public ConcurrentDictionary<Guid, IPlayer> Players { get; } + public IReadOnlyDictionary<Guid, IPlayer> Players { get; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Obsidian.API/World/ILevel.cs` at line 32, The ILevel.Players property is exposing the mutable ConcurrentDictionary directly, allowing callers to bypass the controlled player management flow. Update the ILevel contract to return a read-only view such as IReadOnlyDictionary<Guid, IPlayer>, and adjust any implementations/usages of Players to preserve access while keeping TryAddPlayer and TryRemovePlayer as the only mutation paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.vscode/tasks.json:
- Around line 97-103: The restore task still points to the old solution file, so
update the dotnet restore target in the tasks configuration to match the
migrated solution name used by the other tasks. Check the restore task alongside
build-solution and clean in the tasks definition, and change the Obsidian.sln
reference to Obsidian.slnx so all solution-related tasks are consistent.
In `@Obsidian.API/World/DimensionSettings/DimensionGeneratorSettings.cs`:
- Around line 5-6: The DimensionGeneratorSettings DTO leaves Settings as an
interface type, so deserialization has no way to create a concrete instance.
Update DimensionGeneratorSettings to use a concrete mapping path between Type
and Settings, such as a custom converter or discriminator-based factory, and
ensure the deserialization logic resolves IDimensionSetting to the correct
implementation before materializing the object.
In `@Obsidian.API/World/WorldGenSetting.cs`:
- Line 12: The WorldGenSetting.Dimensions property is left null on new
instances, so initialize it to an empty dictionary by default in WorldGenSetting
to make it safe to enumerate without null checks. Update the property
declaration or constructor so callers always get a usable Dictionary<string,
DimensionSetting> when creating a new WorldGenSetting.
In `@Obsidian/Entities/Factories/EntitySpawner.cs`:
- Line 141: The EntitySpawner spawn path is casting the level to World even
though SpawnEntity(IEntity) is defined on AbstractLevel, which breaks non-World
levels. Update the return in EntitySpawner to call the shared spawn method on
the level’s AbstractLevel reference instead of using a World cast, and keep the
logic aligned with the Dimension/AbstractLevel hierarchy so all level types can
spawn entities correctly.
In `@Obsidian/Services/PacketBroadcaster.cs`:
- Around line 53-60: `BroadcastToLevelInRange` is ignoring its `excludedIds`
parameter, so players that should be skipped still receive packets. Update the
`PacketBroadcaster.BroadcastToLevelInRange` loop to filter the result of
`world.GetPlayersInRange(...)` the same way `BroadcastToLevel` and
`QueuePacketToLevelInRange` do, excluding any `Player` whose id is present in
`excludedIds` before calling `Client.SendPacket(packet)`.
In `@Obsidian/WorldData/AbstractLevel.cs`:
- Around line 385-393: The chunk unloading logic in ManageChunksAsync uses
LoadedChunks.Except(...).ForEach(async c => ...), which creates detached async
void work and can hide exceptions. Replace the ForEach lambda with an awaited
loop over the materialized set so each unload is awaited in sequence or with
controlled concurrency, and keep the LoadedChunks.TryRemove and
NumericsHelper.LongToInts/GetRegionForChunk flow inside that loop. Also add a
null check for the region returned by GetRegionForChunk before calling
UnloadChunk.
In `@Obsidian/WorldData/Dimension.cs`:
- Around line 28-34: Persist dimension LevelData in Dimension by implementing
the stubbed LoadAsync and SaveAsync methods so each dimension’s level.dat is
actually written and read back. Use the existing Dimension members like
FolderPath, LevelDataFilePath, and LevelData to serialize on SaveAsync and
restore on LoadAsync, and make sure the logic handles missing files gracefully.
Keep the behavior aligned with WorldManager.LoadWorldsAsync so existing worlds
can recreate or reload child dimensions on startup instead of losing
per-dimension state.
In `@Obsidian/WorldData/Region.cs`:
- Around line 152-157: The neighbor update loop in BeginTickAsync is using
List<T>.ForEach with an async lambda, which creates an unawaited async-void call
and can hide exceptions from BlockUpdateNeighborsAsync. Replace the
neighborUpdates.ForEach(async u => await u.Level.BlockUpdateNeighborsAsync(u))
pattern with an awaited asynchronous iteration so each neighbor update is
properly awaited before BeginTickAsync completes. Keep the existing
delayed/AddBlockUpdate flow intact, and make sure the fix preserves the use of
neighborUpdates and BlockUpdateNeighborsAsync as the key symbols.
In `@Obsidian/WorldData/RegionFile.cs`:
- Around line 79-82: GetChunkBytesAsync in RegionFile still uses a
partial-read-prone ReadAsync when filling the chunk buffer, which can leave
truncated data before BinaryPrimitives.ReadInt32BigEndian is applied. Update the
GetChunkBytesAsync read path to use ReadExactlyAsync on the regionFileStream for
the chunk buffer, matching the fix already applied earlier in RegionFile, so the
full requested byte count is guaranteed before parsing.
In `@Obsidian/WorldData/World.cs`:
- Around line 30-31: The World loading path in World and its NbtReader usage
leaves the FileStream from fi.OpenRead() undisposed, which can leak handles and
interfere with later file operations. Update the load logic to wrap both the
stream and the NbtReader in using statements (or equivalent disposal handling)
around the ReadNextTag call, keeping the existing levelCompound parsing intact.
- Around line 56-66: World.LoadAsync is still using a hardcoded spawn-chunk
radius instead of the configured value, which can overflow SpawnChunks or leave
default entries processed. Update the chunk range loops in World.LoadAsync to
use this.Configuration.SpawnChunkRadius, matching GenerateAsync and the
SpawnChunks sizing from Configuration.SpawnChunkRadius, and keep the SpawnChunks
fill and Parallel.ForEachAsync traversal aligned with that configured radius.
---
Outside diff comments:
In `@Obsidian/Commands/Modules/MainCommandModule.cs`:
- Around line 163-171: The /save command only flushes when this.Player?.Level is
a World, so it silently does nothing for Dimension-backed levels. Update
SaveAsync in MainCommandModule to resolve the owning World from
IDimension.ParentWorld when the player level is a Dimension, then call
FlushRegionsAsync on that world as well; keep the existing World path intact and
use the current Player/Level checks to locate the level type.
In `@Obsidian/Entities/Entity.cs`:
- Around line 361-372: Cross-level teleport in Entity.TeleportAsync is spawning
a new detached entity instead of moving the current one. In the branch where
to.Level != Level, remove this from the old level, update the current entity’s
Level to the target, and place/move this entity into the new level rather than
calling Level.SpawnEntity and ignoring its return value. Use the existing
Entity.TeleportAsync flow and Level.DestroyEntityAsync/SpawnEntity symbols to
locate and replace the cross-level transfer logic so the same entity instance is
preserved.
In `@Obsidian/Entities/Player.cs`:
- Around line 349-368: RespawnAsync still relies on Debug.Assert(codec is not
null), which won’t protect Release builds if
CodecRegistry.TryGetDimension(Level.DimensionName, out var codec) fails. Update
the RespawnAsync flow to handle a missing codec explicitly before building the
RespawnPacket, using a clear fallback or failing fast with a logged error
instead of letting codec.Id throw. Keep the fix localized to RespawnAsync and
the codec lookup/packet construction path.
In `@Obsidian/Events/MainEventHandler.Inventory.cs`:
- Around line 319-355: SpawnThrownItem broadcasts the new ItemEntity but never
registers it with the level, so it won’t participate in ticking, merging, or
pickup timing. Update SpawnThrownItem in MainEventHandler.Inventory.cs to add
the created item via player.Level.TryAddEntity(item) before queueing the
AddEntityPacket and SetEntityDataPacket, matching the other item spawn paths.
---
Nitpick comments:
In @.github/workflows/dotnet.yml:
- Around line 23-25: The checkout step in the dotnet workflow should be hardened
by disabling credential persistence. Update the actions/checkout configuration
used for recursive submodule checkout to explicitly set persist-credentials to
false so the GitHub token is not left in the local git config. Locate the change
in the checkout step that already uses submodules: recursive and add the missing
security setting there.
In `@Obsidian.API/World/DimensionSettings/DimensionGeneratorBiomeEntry.cs`:
- Around line 3-8: The DimensionGeneratorBiomeEntry properties are non-nullable
but have no initialization, which can trigger CS8618. Update Biome and
Parameters in DimensionGeneratorBiomeEntry to either be marked required or
initialized with safe defaults so the record class satisfies nullable reference
type checks; keep the change aligned with the existing nullable annotations used
by DimensionGeneratorBiomeSource.Biomes.
In `@Obsidian.API/World/DimensionSettings/DimensionGeneratorBiomeParameters.cs`:
- Around line 3-18: The seven DimensionGeneratorBiomeParameters properties are
declared non-nullable without initialization, which is inconsistent with the
nullable style used elsewhere. Update the Offset, Continentalness, Weirdness,
Erosion, Depth, Humidity, and Temperature properties in
DimensionGeneratorBiomeParameters to be explicitly nullable or
required/initialized, matching the pattern used by DimensionGeneratorBiomeEntry
and DimensionGeneratorBiomeSource.Biomes. Make the choice consistent with the
surrounding model types so callers can tell whether values must be provided or
may be absent.
In `@Obsidian.API/World/ILevel.cs`:
- Line 83: The interface method name still uses World terminology, so update
ILevel’s GetWorldSurfaceHeightAsync to a Level-centric name like
GetLevelSurfaceHeightAsync for consistency with the surrounding API naming. Make
sure any implementation, call sites, and related references to
GetWorldSurfaceHeightAsync are renamed together so the Level/LevelData naming
stays uniform.
- Line 32: The ILevel.Players property is exposing the mutable
ConcurrentDictionary directly, allowing callers to bypass the controlled player
management flow. Update the ILevel contract to return a read-only view such as
IReadOnlyDictionary<Guid, IPlayer>, and adjust any implementations/usages of
Players to preserve access while keeping TryAddPlayer and TryRemovePlayer as the
only mutation paths.
In `@Obsidian/Events/MainEventHandler.World.cs`:
- Line 15: Rename the local variable in MainEventHandler.World from world to
level since it now stores player.Level (an ILevel rather than an IWorld), and
update all subsequent references in the same method to match the new name so the
terminology stays consistent with the Level migration.
In `@Obsidian/WorldData/Region.cs`:
- Around line 44-45: The region-load message in Region’s constructor is too
noisy at LogInformation; change the logger call in the Region file-loading path
to LogDebug to match AbstractLevel.LoadRegion and avoid flooding normal logs.
Update the logging in the constructor that records the region file and
compression so it still includes the same context, just at debug level.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 60477044-6ce2-414e-8b37-c054c535b697
📒 Files selected for processing (96)
.github/workflows/dotnet.yml.gitmodules.vscode/launch.json.vscode/tasks.jsonObsidian.API/ChunkData/IBlockUpdate.csObsidian.API/Configuration/ServerConfiguration.csObsidian.API/Events/BlockBreakEventArgs.csObsidian.API/Events/BlockEventArgs.csObsidian.API/Obsidian.API.csprojObsidian.API/World/DimensionSettings/DimensionGeneratorBiomeEntry.csObsidian.API/World/DimensionSettings/DimensionGeneratorBiomeParameters.csObsidian.API/World/DimensionSettings/DimensionGeneratorBiomeSource.csObsidian.API/World/DimensionSettings/DimensionGeneratorSettings.csObsidian.API/World/DimensionSettings/DimensionGeneratorValueRange.csObsidian.API/World/DimensionSettings/DimensionSetting.csObsidian.API/World/DimensionSettings/IDimensionSetting.csObsidian.API/World/ILevel.csObsidian.API/World/ILevelFactory.csObsidian.API/World/ILevelGenerator.csObsidian.API/World/WorldGenSetting.csObsidian.API/_Interfaces/IDimension.csObsidian.API/_Interfaces/IEntity.csObsidian.API/_Interfaces/IPacketBroadcaster.csObsidian.API/_Interfaces/IServer.csObsidian.API/_Interfaces/IWorld.csObsidian.API/_Interfaces/IWorldManager.csObsidian.API/_Types/LevelData.csObsidian.NbtObsidian.Nbt/Exceptions/TagNotFoundException.csObsidian.Nbt/GlobalUsings.csObsidian.Nbt/INbtTag.csObsidian.Nbt/Interfaces/INbtWriter.csObsidian.Nbt/InternalsVisibleTo.csObsidian.Nbt/ModifiedUtf8.csObsidian.Nbt/NbtArray.csObsidian.Nbt/NbtCompound.csObsidian.Nbt/NbtCompression.csObsidian.Nbt/NbtList.csObsidian.Nbt/NbtReader.Primitives.csObsidian.Nbt/NbtReader.csObsidian.Nbt/NbtTag.csObsidian.Nbt/NbtTagType.csObsidian.Nbt/NbtWriterStream.Primitives.csObsidian.Nbt/NbtWriterStream.csObsidian.Nbt/Obsidian.Nbt.csprojObsidian.Nbt/RawNbtWriter.Primitives.csObsidian.Nbt/RawNbtWriter.csObsidian.Nbt/Utilities/NbtWriterState.csObsidian.Nbt/Utilities/ThrowHelper.csObsidian.Tests/Fakes/FakePlayer.csObsidian.Tests/Fakes/FakeServer.csObsidian.slnxObsidian/BlockUpdate.csObsidian/Commands/Modules/MainCommandModule.csObsidian/Entities/Animal.csObsidian/Entities/Entity.csObsidian/Entities/Factories/EntitySpawner.csObsidian/Entities/FallingBlock.csObsidian/Entities/ItemEntity.csObsidian/Entities/Living.csObsidian/Entities/Player.Helpers.csObsidian/Entities/Player.csObsidian/Events/MainEventHandler.Inventory.csObsidian/Events/MainEventHandler.World.csObsidian/Events/MainEventHandler.csObsidian/GlobalUsings.csObsidian/Hosting/DependencyInjection.csObsidian/Net/Packets/Configuration/Serverbound/FinishConfigurationPacket.csObsidian/Net/Packets/Play/Serverbound/PlayerActionPacket.csObsidian/Net/Packets/Play/Serverbound/PlayerCommandPacket.csObsidian/Net/Packets/Play/Serverbound/PlayerInputPacket.csObsidian/Net/Packets/Play/Serverbound/SetCarriedItemPacket.csObsidian/Net/Packets/Play/Serverbound/SetCreativeModeSlotPacket.csObsidian/Net/Packets/Play/Serverbound/UseItemOnPacket.csObsidian/Obsidian.csprojObsidian/Server.csObsidian/ServerConstants.csObsidian/Services/PacketBroadcaster.csObsidian/Utilities/Extensions.csObsidian/WorldData/AbstractLevel.csObsidian/WorldData/BlockUpdates.csObsidian/WorldData/Dimension.csObsidian/WorldData/Generators/EmptyWorldGenerator.csObsidian/WorldData/Generators/GenHelper.csObsidian/WorldData/Generators/IslandGenerator.csObsidian/WorldData/Generators/Mojang/ChunkBuilder.csObsidian/WorldData/Generators/MojangGenerator.csObsidian/WorldData/Generators/OverworldGenerator.csObsidian/WorldData/Generators/SuperflatGenerator.csObsidian/WorldData/LevelFactory.csObsidian/WorldData/Lighting.csObsidian/WorldData/Region.csObsidian/WorldData/RegionFile.csObsidian/WorldData/World.csObsidian/WorldData/WorldManager.csREADME.md
💤 Files with no reviewable changes (22)
- Obsidian.Nbt/Exceptions/TagNotFoundException.cs
- Obsidian.Nbt/Obsidian.Nbt.csproj
- Obsidian.Nbt/ModifiedUtf8.cs
- Obsidian.Nbt/InternalsVisibleTo.cs
- Obsidian.Nbt/NbtCompound.cs
- Obsidian.Nbt/NbtTagType.cs
- Obsidian.Nbt/Interfaces/INbtWriter.cs
- Obsidian.Nbt/NbtTag.cs
- Obsidian.API/_Interfaces/IWorldManager.cs
- Obsidian.Nbt/Utilities/NbtWriterState.cs
- Obsidian.Nbt/NbtReader.Primitives.cs
- Obsidian.Nbt/NbtReader.cs
- Obsidian.Nbt/RawNbtWriter.Primitives.cs
- Obsidian.Nbt/Utilities/ThrowHelper.cs
- Obsidian.Nbt/NbtList.cs
- Obsidian.Nbt/NbtCompression.cs
- Obsidian.Nbt/GlobalUsings.cs
- Obsidian.Nbt/NbtArray.cs
- Obsidian.Nbt/INbtTag.cs
- Obsidian.Nbt/NbtWriterStream.cs
- Obsidian.Nbt/NbtWriterStream.Primitives.cs
- Obsidian.Nbt/RawNbtWriter.cs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Obsidian/WorldData/AbstractLevel.cs (1)
248-268: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
zloop should iterate upward through the chunk range
ToChunkCoord()mapsXandZdirectly, sotopis the min chunk z andbottomis the max. As written,z >= bottomwithz -= ...skips every multi-row range here.🐛 Proposed fix
- for (int z = top; z >= bottom; z -= Region.CubicRegionSize) + for (int z = top; z <= bottom; z += Region.CubicRegionSize)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Obsidian/WorldData/AbstractLevel.cs` around lines 248 - 268, The chunk scan in AbstractLevel’s region iteration is using the wrong z-direction, causing multi-row ranges to be skipped. Update the loop in the entity search logic that uses GetRegionForChunk and Region.CubicRegionSize so that z advances upward from bottom to top instead of decrementing from top, and keep the bounds consistent with ToChunkCoord()’s mapping of top/min and bottom/max.
♻️ Duplicate comments (1)
Obsidian/WorldData/AbstractLevel.cs (1)
385-393: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winNull-check the region before calling
UnloadChunk.
GetRegionForChunk(cx, cz)returnsIRegion?, but line 391 dereferencesrunconditionally. If the region has already been unloaded/removed while the chunk was still tracked inLoadedChunks, this throws an NPE and abortsManageChunksAsync.🛡️ Proposed guard
if (LoadedChunks.TryRemove(chunk)) { NumericsHelper.LongToInts(chunk, out var cx, out var cz); var r = GetRegionForChunk(cx, cz); - await r.UnloadChunk(cx, cz); + if (r is not null) + await r.UnloadChunk(cx, cz); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Obsidian/WorldData/AbstractLevel.cs` around lines 385 - 393, The chunk cleanup loop in ManageChunksAsync can dereference a null region because GetRegionForChunk(cx, cz) returns IRegion? and r is used unconditionally before calling UnloadChunk. Update the LoadedChunks.TryRemove branch to guard the region lookup result from GetRegionForChunk, and only call r.UnloadChunk(cx, cz) when the region is non-null so the cleanup loop does not abort on a missing region.
🧹 Nitpick comments (1)
Obsidian/WorldData/AbstractLevel.cs (1)
478-483: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDrop the unnecessary
as Entitycast.
TryAddEntityalready acceptsIEntity, so casting withas Entityadds nothing except a silent-null hazard: ifentityisn't anEntity,entity as Entityyieldsnull, andTryAddEntitythen dereferencesentity.Position, throwing an NPE. Passentitydirectly.♻️ Proposed change
public IEntity SpawnEntity(IEntity entity) { entity.SpawnEntity(); - TryAddEntity(entity as Entity); + TryAddEntity(entity); return entity; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Obsidian/WorldData/AbstractLevel.cs` around lines 478 - 483, The SpawnEntity flow is using an unnecessary and unsafe cast when calling TryAddEntity; since TryAddEntity already accepts IEntity, remove the as Entity conversion and pass the entity reference directly. Update the SpawnEntity method in AbstractLevel so it spawns the entity and then adds the same IEntity instance without risking a null from the cast.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Obsidian/WorldData/AbstractLevel.cs`:
- Around line 248-268: The chunk scan in AbstractLevel’s region iteration is
using the wrong z-direction, causing multi-row ranges to be skipped. Update the
loop in the entity search logic that uses GetRegionForChunk and
Region.CubicRegionSize so that z advances upward from bottom to top instead of
decrementing from top, and keep the bounds consistent with ToChunkCoord()’s
mapping of top/min and bottom/max.
---
Duplicate comments:
In `@Obsidian/WorldData/AbstractLevel.cs`:
- Around line 385-393: The chunk cleanup loop in ManageChunksAsync can
dereference a null region because GetRegionForChunk(cx, cz) returns IRegion? and
r is used unconditionally before calling UnloadChunk. Update the
LoadedChunks.TryRemove branch to guard the region lookup result from
GetRegionForChunk, and only call r.UnloadChunk(cx, cz) when the region is
non-null so the cleanup loop does not abort on a missing region.
---
Nitpick comments:
In `@Obsidian/WorldData/AbstractLevel.cs`:
- Around line 478-483: The SpawnEntity flow is using an unnecessary and unsafe
cast when calling TryAddEntity; since TryAddEntity already accepts IEntity,
remove the as Entity conversion and pass the entity reference directly. Update
the SpawnEntity method in AbstractLevel so it spawns the entity and then adds
the same IEntity instance without risking a null from the cast.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d974a021-2a36-4a6a-a2d5-75537f5b4d3b
📒 Files selected for processing (5)
Obsidian.API/World/ILevel.csObsidian/Entities/Factories/EntitySpawner.csObsidian/Services/PacketBroadcaster.csObsidian/WorldData/AbstractLevel.csObsidian/WorldData/World.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- Obsidian/Entities/Factories/EntitySpawner.cs
- Obsidian/Services/PacketBroadcaster.cs
- Obsidian/WorldData/World.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Introduces a major refactor of the world and dimension system in the API, replacing the previous
IWorld-centric model with a newILevelabstraction.The most important changes are:
World/Level Abstraction Refactor
Introduced the new
ILevelinterface to represent a game level (world or dimension), replacing many usages ofIWorld. Updated related interfaces and classes to useILevelinstead ofIWOrld.Added
IDimensioninterface extendingILevel, with a reference to its parentIWorld.Updated
IPacketBroadcastermethods to useILevelinstead ofIWorldand renamed parameters accordingly.World and Dimension Management
Added
ILevelFactoryinterface for creating worlds and dimensions, registering generators, and initialization logic.Updated and renamed the world generator interface to
ILevelGenerator(fromIWorldGenerator), with methods now takingILevelinstead ofIWorld.Added
ILevelManagerproperty to theIServerinterface for managing worlds.Dimension and World Generation Settings
These changes lay the groundwork for more robust handling of multiple dimensions and advanced world generation, and should make future extensions and maintenance easier~
I'm not quite done with these changes yet but I should have something ready soon ^ ^
Summary by CodeRabbit