Conversation
WalkthroughUpdates include dependency version bumps, new density-function classes and extensive worldgen JSON assets, velocity type and packing refactor, enum and packet layout changes, several packet/serialization updates, and small utilities/constants additions. No runtime algorithm implementations were completed for the new density functions. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas to focus on:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
Obsidian.Tests/Obsidian.Tests.csproj (1)
20-29: Do not upgrade System.Drawing.Common to 10.0.0 without addressing platform compatibility.System.Drawing.Common 10.0.0 is Windows-only on all platforms including .NET 10. Your CI/CD pipeline uses
ubuntu-latest(Docker workflow), which means tests will fail withPlatformNotSupportedExceptionat runtime. Since the main code uses System.Drawing in HexColor.cs and ChatColor.cs (in Obsidian.API), and tests verify these classes, this dependency cannot be trivially removed.Required action: Either:
- Migrate color handling in Obsidian.API to a cross-platform library (e.g., ImageSharp, SkiaSharp), or
- Keep System.Drawing.Common at 9.0.4 and ensure tests run only on Windows, or
- Mark test execution as Windows-only and skip tests on non-Windows CI runners.
Obsidian/Net/Packets/Common/ClientInformationPacket.cs (1)
51-59: Based on my verification, I have confirmed the issue. The code has a real vulnerability:Key findings:
- ServerConfiguration.ViewDistance is of type
byte(range 0-255)- ClientInformationPacket.ViewDistance is of type
sbyte(range -128 to 127)- The cast
(sbyte)server.Configuration.ViewDistanceis unchecked and will overflow if ViewDistance > 127- No upper bound validation exists on ServerConfiguration.ViewDistance setter
- When byte value > 127 is cast to sbyte, it wraps to a negative value, breaking the Min() comparison logic
For example: if
server.Configuration.ViewDistance = 200, the cast producessbyte -56, which would cause incorrect clamping behavior.
Guard against potential overflow when casting
Configuration.ViewDistancetosbyteThe code assumes
server.Configuration.ViewDistance(which is of typebyte, range 0-255) always fits in thesbyterange (-128 to 127). If that config is set above 127, the unchecked cast will wrap to a negative value, breaking the clamping logic.ViewDistance = sbyte.Min(ViewDistance, (sbyte)server.Configuration.ViewDistance),Consider clamping the configuration value before casting:
var configViewDistance = (sbyte)Math.Clamp(server.Configuration.ViewDistance, sbyte.MinValue, sbyte.MaxValue); ViewDistance = sbyte.Min(ViewDistance, configViewDistance),Obsidian.API/_Types/Velocity.cs (2)
35-38: Remove obsolete(short)casts that cause precision loss.The constructor now accepts
double, but these factory methods still cast toshort, truncating values to integers and risking overflow for values outside -32768 to 32767.Apply this diff:
public static Velocity FromBlockPerSecond(float x, float y, float z) { - return new Velocity((short)(400f * x), (short)(400f * y), (short)(400f * z)); + return new Velocity(400.0 * x, 400.0 * y, 400.0 * z); }
46-49: Same incomplete refactor issue—remove(short)casts.The
8000fmultiplier makes overflow even more likely here.Apply this diff:
public static Velocity FromBlockPerTick(float x, float y, float z) { - return new Velocity((short)(8000f * x), (short)(8000f * y), (short)(8000f * z)); + return new Velocity(8000.0 * x, 8000.0 * y, 8000.0 * z); }
🧹 Nitpick comments (13)
Obsidian/Utilities/NumericsHelper.cs (1)
20-21: AbsMax implementation looks correct; consider explicitSystemusage for clarityThe method correctly returns the greater absolute value and matches the style of existing helpers. The only nit is that this file doesn’t declare
using System;, soMathrelies on implicit/global usings. For robustness/clarity you might prefer explicit qualification:- public static double AbsMax(double a, double b) => Math.Max(Math.Abs(a), Math.Abs(b)); + public static double AbsMax(double a, double b) => System.Math.Max(System.Math.Abs(a), System.Math.Abs(b));Obsidian/Assets/worldgen/density_function/overworld/caves/spaghetti_2d_thickness_modulator.json (1)
1-17: Normalize floating-point precision in arithmetic constants.Line 8 contains a floating-point precision artifact (
-0.35000000000000003) where the intended value is likely-0.35. While Minecraft's density function evaluation handles this transparently, these precision artifacts indicate the asset was generated programmatically without post-processing normalization. Standardizing these values improves data consistency and readability.Apply this diff to clean up the constant:
"argument2": { "type": "minecraft:mul", - "argument1": -0.35000000000000003, + "argument1": -0.35, "argument2": {Obsidian/Assets/worldgen/density_function/overworld/caves/spaghetti_2d.json (1)
1-61: Add newline at end of file.Density function JSON files should follow standard formatting conventions. The file is missing a trailing newline character at the end.
Add a newline after the closing brace on line 61:
"max": 1.0, "min": -1.0 } +Obsidian/Assets/worldgen/density_function/overworld/caves/entrances.json (1)
1-83: Normalize floating-point precision in arithmetic constants.Line 68 contains a floating-point precision artifact (
-0.011499999999999996) where the intended value is likely-0.0115or similar normalized constant. This is consistent with the precision artifact in spaghetti_2d_thickness_modulator.json (line 8). Standardizing these values improves data consistency across the density function asset collection.Apply this diff to clean up the constant:
"type": "minecraft:add", "argument1": -0.0765, "argument2": { "type": "minecraft:mul", - "argument1": -0.011499999999999996, + "argument1": -0.0115, "argument2": {Obsidian.API/_Enums/EntityMetadataType.cs (1)
20-38: Enum value reindexing: verify protocol and persistence impactRemoving the previous entry here and shifting
ParticlethroughQuaternionso they now occupy16..33(withResolvableProfile = 34) changes the underlying ints for all of these metadata types. That’s fine if it’s intentionally tracking the 1.21.10 protocol, but it’s a breaking change for any code that relied on the old numeric values (wire format, saved data, or plugin-facing APIs that persisted/cast the enum).Please double‑check that:
- All metadata (de)serializers are updated to use this new mapping for 1.21.10.
- There’s no persisted state or third‑party API contract that still assumes the previous ints, or if there is, that there’s a migration/compat layer in place.
- The new
ResolvableProfilevalue is fully supported wherever metadata values are encoded/decoded.Obsidian/Net/Packets/Play/Clientbound/SetDefaultSpawnPositionPacket.cs (1)
10-14: Inconsistent property accessors betweenYawandPitch.
Yawhas a setter ({ get; set; }) whilePitchis read-only ({ get; }). If both should be immutable after construction, consider makingYawread-only as well for consistency.[Field(1), DataFormat(typeof(float))] -public Angle Yaw { get; set; } = yaw; +public Angle Yaw { get; } = yaw; [Field(2), DataFormat(typeof(float))] public Angle Pitch { get; } = pitch;Obsidian.API/Utilities/Weighted.cs (1)
1-8: Weighted API is fine; consider tightening Weight initializationThe immutable
Weighted<T>record withrequiredValuelooks good. If a weight of0is not meaningful in your selection logic, consider also markingWeightasrequired(or assigning a sensible default like1) to avoid accidentally constructing unusable entries.Obsidian/Assets/worldgen/density_function/overworld/jaggedness.json (1)
1-303: Jaggedness spline graph is structurally consistent but very brittle to manual editsThe nested
flat_cache→cache_2d→add/mul/splinestructure, coordinates (minecraft:overworld/continents,erosion,ridges_folded,ridges), and point objects (location,value,derivative) all follow the expected schema. Given the amount of duplicated numeric/control‑point data, consider keeping this file generated from upstream 1.21.10 data or adding a small validation tool so future hand edits don’t accidentally desync locations/values between similar point blocks.Obsidian/Assets/worldgen/density_function/overworld/offset.json (1)
1-1523: Offset density function wiring appears valid; consider tooling for reuse/validationThe top‑level
flat_cache+cache_2dwrapper, use ofblend_offset,blend_alpha,cache_once, and the deeply nested continent/erosion/ridges_folded splines all match Minecraft’s density function schema, and the coordinate/resource IDs are consistently"minecraft:overworld/...". The only concern here is maintainability: there is a lot of repeated spline-and-points structure that would be hard to correct by hand if Mojang tweaks values; generating this from upstream data or adding a validator would reduce future errors.Obsidian/Assets/worldgen/density_function/overworld/factor.json (1)
1-890: Factor density function is well-formed and consistent with related assetsThe
flat_cache/cache_2dwrapper, arithmetic (add/mul), and spline structure overminecraft:overworld/continents→erosion→ridges/ridges_foldedlook schema‑correct, and the repeated ridge value patterns align with how factor/jaggedness/offset are usually coordinated. No structural issues stand out; just the usual caveat that large hand‑maintained numeric tables are easy to desync without a generator.Obsidian/Assets/worldgen/density_function/end/base_3d_noise.json (1)
1-8: LGTM! End dimension base noise configuration added.The base 3D noise configuration for the End dimension is properly structured with appropriate parameters. This asset is correctly referenced by
end/sloped_cheese.jsonand follows the pattern established for other dimension noise configurations in the PR.Consider adding a newline at the end of the file for consistency with standard JSON formatting practices, though this does not affect functionality.
Obsidian.API/_Enums/ParticleType.cs (1)
5-119: Consider decoupling enum semantics from wire IDsGiven this enum is tightly coupled to protocol IDs and must be renumbered each MC update, consider introducing a separate mapping (e.g., a lookup table from
ParticleTypeto protocol ID) and letting the enum have stable, implicit values. That would reduce churn and the risk of subtle off‑by‑one errors on future updates.Obsidian/Net/Packets/Play/Clientbound/ExplodePacket.cs (1)
32-38: ExplosionRecord structure is reasonable; consider aligning defaults with specRepresenting a per‑entry block particle as
ParticleData+Scaling+Speedmatches the documentedblock_particlesentry format (particle, scaling, speed) and pairs well withWeighted<ExplosionRecord>for theweightfield.If this struct is also reused for the enchantment
block_particlesJSON, consider initializing the floats to the documented defaults (1.0) instead of 0.0 to mirror game semantics and avoid accidental “no‑movement” values when constructing records manually, for example:public readonly struct ExplosionRecord { - public required ParticleData Particle { get; init; } - - public float Scaling { get; init; } - - public float Speed { get; init; } + public required ParticleData Particle { get; init; } + + public float Scaling { get; init; } = 1.0f; + + public float Speed { get; init; } = 1.0f; }Please confirm whether the runtime expects omitted scaling/speed to behave as 1.0 in all cases; if not, keeping the current defaults may be intentional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (75)
Obsidian.API/Obsidian.API.csproj(1 hunks)Obsidian.API/Utilities/Weighted.cs(1 hunks)Obsidian.API/World/Generator/DensityFunctions/FindTopSurface.cs(1 hunks)Obsidian.API/World/Generator/DensityFunctions/InvertDensityFunction.cs(1 hunks)Obsidian.API/World/Generator/Noise/NoiseRouter.cs(1 hunks)Obsidian.API/_Enums/EntityMetadataType.cs(1 hunks)Obsidian.API/_Enums/EntityType.cs(2 hunks)Obsidian.API/_Enums/ParticleType.cs(1 hunks)Obsidian.API/_Enums/PositionFlags.cs(0 hunks)Obsidian.API/_Enums/ProtocolVersion.cs(1 hunks)Obsidian.API/_Types/Velocity.cs(1 hunks)Obsidian.ConsoleApp/Obsidian.ConsoleApp.csproj(1 hunks)Obsidian.SourceGenerators/Obsidian.SourceGenerators.csproj(1 hunks)Obsidian.Tests/Obsidian.Tests.csproj(1 hunks)Obsidian/Assets/Codecs/biomes.json(0 hunks)Obsidian/Assets/Codecs/dimensions.json(1 hunks)Obsidian/Assets/Codecs/trim_material.json(1 hunks)Obsidian/Assets/enums.json(1 hunks)Obsidian/Assets/packets.json(6 hunks)Obsidian/Assets/recipes.json(50 hunks)Obsidian/Assets/tags.json(35 hunks)Obsidian/Assets/worldgen/density_function/end/base_3d_noise.json(1 hunks)Obsidian/Assets/worldgen/density_function/end/sloped_cheese.json(1 hunks)Obsidian/Assets/worldgen/density_function/nether/base_3d_noise.json(1 hunks)Obsidian/Assets/worldgen/density_function/overworld/base_3d_noise.json(1 hunks)Obsidian/Assets/worldgen/density_function/overworld/caves/entrances.json(1 hunks)Obsidian/Assets/worldgen/density_function/overworld/caves/noodle.json(1 hunks)Obsidian/Assets/worldgen/density_function/overworld/caves/pillars.json(1 hunks)Obsidian/Assets/worldgen/density_function/overworld/caves/spaghetti_2d.json(1 hunks)Obsidian/Assets/worldgen/density_function/overworld/caves/spaghetti_2d_thickness_modulator.json(1 hunks)Obsidian/Assets/worldgen/density_function/overworld/caves/spaghetti_roughness_function.json(1 hunks)Obsidian/Assets/worldgen/density_function/overworld/continents.json(1 hunks)Obsidian/Assets/worldgen/density_function/overworld/depth.json(1 hunks)Obsidian/Assets/worldgen/density_function/overworld/erosion.json(1 hunks)Obsidian/Assets/worldgen/density_function/overworld/factor.json(1 hunks)Obsidian/Assets/worldgen/density_function/overworld/jaggedness.json(1 hunks)Obsidian/Assets/worldgen/density_function/overworld/offset.json(1 hunks)Obsidian/Assets/worldgen/density_function/overworld/ridges.json(1 hunks)Obsidian/Assets/worldgen/density_function/overworld/ridges_folded.json(1 hunks)Obsidian/Assets/worldgen/density_function/overworld/sloped_cheese.json(1 hunks)Obsidian/Assets/worldgen/density_function/overworld_amplified/depth.json(1 hunks)Obsidian/Assets/worldgen/density_function/overworld_amplified/factor.json(1 hunks)Obsidian/Assets/worldgen/density_function/overworld_amplified/jaggedness.json(1 hunks)Obsidian/Assets/worldgen/density_function/overworld_amplified/offset.json(1 hunks)Obsidian/Assets/worldgen/density_function/overworld_amplified/sloped_cheese.json(1 hunks)Obsidian/Assets/worldgen/density_function/overworld_large_biomes/continents.json(1 hunks)Obsidian/Assets/worldgen/density_function/overworld_large_biomes/depth.json(1 hunks)Obsidian/Assets/worldgen/density_function/overworld_large_biomes/erosion.json(1 hunks)Obsidian/Assets/worldgen/density_function/overworld_large_biomes/factor.json(1 hunks)Obsidian/Assets/worldgen/density_function/overworld_large_biomes/jaggedness.json(1 hunks)Obsidian/Assets/worldgen/density_function/overworld_large_biomes/offset.json(1 hunks)Obsidian/Assets/worldgen/density_function/overworld_large_biomes/sloped_cheese.json(1 hunks)Obsidian/Assets/worldgen/density_function/shift_x.json(1 hunks)Obsidian/Assets/worldgen/density_function/shift_z.json(1 hunks)Obsidian/Assets/worldgen/density_function/y.json(1 hunks)Obsidian/Assets/worldgen/density_function/zero.json(1 hunks)Obsidian/Assets/worldgen/noise_settings/amplified.json(1 hunks)Obsidian/Assets/worldgen/noise_settings/caves.json(1 hunks)Obsidian/Assets/worldgen/noise_settings/end.json(1 hunks)Obsidian/Assets/worldgen/noise_settings/floating_islands.json(1 hunks)Obsidian/Assets/worldgen/noise_settings/large_biomes.json(1 hunks)Obsidian/Assets/worldgen/noise_settings/nether.json(1 hunks)Obsidian/Assets/worldgen/noise_settings/overworld.json(1 hunks)Obsidian/Entities/Player.cs(1 hunks)Obsidian/Net/NetworkBuffer.Reading.cs(4 hunks)Obsidian/Net/NetworkBuffer.Writing.cs(3 hunks)Obsidian/Net/Packets/Common/ClientInformationPacket.cs(1 hunks)Obsidian/Net/Packets/Configuration/Serverbound/FinishConfigurationPacket.cs(1 hunks)Obsidian/Net/Packets/Play/Clientbound/AddEntityPacket.cs(1 hunks)Obsidian/Net/Packets/Play/Clientbound/BlockEntityDataPacket.cs(1 hunks)Obsidian/Net/Packets/Play/Clientbound/ExplodePacket.cs(1 hunks)Obsidian/Net/Packets/Play/Clientbound/SetDefaultSpawnPositionPacket.cs(1 hunks)Obsidian/Obsidian.csproj(1 hunks)Obsidian/ServerConstants.cs(1 hunks)Obsidian/Utilities/NumericsHelper.cs(1 hunks)
💤 Files with no reviewable changes (2)
- Obsidian.API/_Enums/PositionFlags.cs
- Obsidian/Assets/Codecs/biomes.json
🧰 Additional context used
🧬 Code graph analysis (9)
Obsidian/Entities/Player.cs (4)
Obsidian.Nbt/RawNbtWriter.Primitives.cs (2)
WriteByte(24-28)WriteByte(30-37)Obsidian/Net/NetworkBuffer.cs (1)
WriteByte(103-108)Obsidian/Net/EncryptedNetworkBuffer.cs (1)
WriteByte(17-17)Obsidian.API/Utilities/Extensions.Nbt.cs (1)
WriteNbtCompound(189-199)
Obsidian.API/World/Generator/DensityFunctions/FindTopSurface.cs (2)
Obsidian.API/World/Generator/DensityFunctions/InvertDensityFunction.cs (2)
DensityFunction(3-15)GetValue(14-14)Obsidian.API/World/Generator/ChunkBuilder.cs (1)
IDensityFunction(42-52)
Obsidian/Net/NetworkBuffer.Writing.cs (6)
Obsidian.API/_Interfaces/INetStreamWriter.cs (4)
WriteVelocity(51-51)WriteByte(10-10)WriteByte(11-11)WriteByte(12-12)Obsidian.API/_Types/Velocity.cs (6)
Velocity(35-38)Velocity(46-49)Velocity(55-58)Velocity(64-67)Velocity(74-77)Velocity(84-87)Obsidian.API/_Interfaces/INetStreamReader.cs (1)
Velocity(68-68)Obsidian/Utilities/NumericsHelper.cs (5)
NumericsHelper(5-22)MethodImpl(7-12)MethodImpl(14-15)MethodImpl(17-18)MethodImpl(20-21)Obsidian.Nbt/RawNbtWriter.Primitives.cs (2)
WriteByte(24-28)WriteByte(30-37)Obsidian/Net/NetworkBuffer.cs (1)
WriteByte(103-108)
Obsidian.API/World/Generator/DensityFunctions/InvertDensityFunction.cs (1)
Obsidian.API/World/Generator/DensityFunctions/FindTopSurface.cs (2)
DensityFunction(3-21)GetValue(20-20)
Obsidian.API/World/Generator/Noise/NoiseRouter.cs (1)
Obsidian.API/World/Generator/ChunkBuilder.cs (1)
IDensityFunction(42-52)
Obsidian/Net/NetworkBuffer.Reading.cs (2)
Obsidian.API/_Types/Velocity.cs (6)
Velocity(35-38)Velocity(46-49)Velocity(55-58)Velocity(64-67)Velocity(74-77)Velocity(84-87)Obsidian.API/_Interfaces/INetStreamReader.cs (1)
Velocity(68-68)
Obsidian/Net/Packets/Configuration/Serverbound/FinishConfigurationPacket.cs (2)
Obsidian/Net/Packets/Play/Clientbound/SetDefaultSpawnPositionPacket.cs (1)
SetDefaultSpawnPositionPacket(5-22)Obsidian/WorldData/World.cs (2)
World(16-898)World(89-96)
Obsidian/Net/Packets/Play/Clientbound/SetDefaultSpawnPositionPacket.cs (2)
Obsidian/Net/Packets/Play/Clientbound/TeleportEntityPacket.cs (1)
Serialize(25-37)Obsidian.API/_Interfaces/INetStreamWriter.cs (1)
WriteSingle(21-21)
Obsidian/Net/Packets/Play/Clientbound/ExplodePacket.cs (1)
Obsidian.API/_Types/ParticleData.cs (1)
ParticleData(3-8)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Obsidian/Net/NetworkBuffer.Reading.cs (1)
478-480: Consider documenting the unpacking formula.The
Unpackmethod's normalization formula—clamping to [0, 32766], scaling by2.0 / 32766, then subtracting 1—correctly maps a 15-bit unsigned integer to the range [-1.0, 1.0]. Adding a brief comment explaining this transformation would aid future maintainers.Example:
+// Unpacks a 15-bit unsigned value to a normalized double in [-1.0, 1.0] private static double Unpack(long value) => Math.Min((value & short.MaxValue), short.MaxValue - 1) * 2.0 / (short.MaxValue - 1) - 1.0;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Obsidian/Net/NetworkBuffer.Reading.cs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Obsidian/Net/NetworkBuffer.Reading.cs (2)
Obsidian.API/_Types/Velocity.cs (6)
Velocity(35-38)Velocity(46-49)Velocity(55-58)Velocity(64-67)Velocity(74-77)Velocity(84-87)Obsidian.API/_Interfaces/INetStreamReader.cs (4)
Velocity(68-68)ReadByte(9-9)ReadInt(20-20)ReadVarInt(27-27)
🔇 Additional comments (1)
Obsidian/Net/NetworkBuffer.Reading.cs (1)
80-102: Perfect! I have completed the verification. Let me now generate the rewritten review comment:
Continuation bit fix verified; read/write implementation is symmetric and correct.
The past review correctly identified that checking
HasContinuationBit(scaleFactor)would always return false sincescaleFactoronly contains bits 0-1 offirstByte. The current implementation at line 94 correctly checksHasContinuationBit(firstByte), properly testing bit 2.Verification confirms the packed data layout and symmetry with
WriteVelocity:
- 48-bit layout: 3 bits (scale + continuation) + 15 bits each (x, y, z)
- Scale factor encoding/decoding is symmetric across read/write
- Helper methods
PackVelocityandUnpackare mathematically inverse functions- Continuation bit logic correctly handles extended scale factors via VarInt
The implementation is sound and ready.
Summary by CodeRabbit
New Features
World Generation
✏️ Tip: You can customize this high-level summary in your review settings.