Skip to content

refactor: optimize cave sphere shaping #3382

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
BoringOrng wants to merge 2 commits into
base: master
Choose a base branch
from
Open

refactor: optimize cave sphere shaping #3382

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
71f9165
refactor: optimize cave sphere shaping
Jul 18, 2026
cee5fbe
chore: apply reviewer fixes
Jul 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 67 additions & 51 deletions src/server/terrain/cavegen/FractalCaveGenerator.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,72 +58,88 @@ pub fn generate(map: *CaveMapFragment, worldSeed: u64) void {
}
}

fn displaceByHalfExtentBounded(center: f32, extent: f32, max_extent: i32) struct {
min: i32,
max: i32,
} {

@IntegratedQuantum IntegratedQuantum Jul 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the other comment (also name convention is camel case)

Suggested change
fn displaceByHalfExtentBounded(center: f32, extent: f32, max_extent: i32) struct {
min: i32,
max: i32,
} {
fn getSphereBounds(center: Vec3f, radius: f32, maxExtent: i32) struct { Vec3i, Vec3i } {

@BoringOrng BoringOrng Jul 23, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rewrote that mostly as follows (completely missed the max_extent I've been writing too much rust as of late), I do have a question as to whether or not you want maxExtent to be a Vec3i or a Vec2i, it can't be an i32, because z has it's own scaling based off of the CaveFragment.height, whereas x and y use the width. Vec3i implies that x and y can use different maxExtents, whereas Vec2i cements in place that whatever x is scaled by, y must be scaled by.

@IntegratedQuantum IntegratedQuantum Jul 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then Vec3i would be better, Vec2i would be rather hard to read in my opinion. There is no reason to cement this property in a helper function that doesn't care about it.

return .{
.min = @max(@as(i32, @trunc(center - extent)) - 1, 0),
.max = @min(
@as(i32, @trunc(center + extent)) + 1,
max_extent,
),
};
}

fn generateSphere_(seed: *u64, map: *CaveMapFragment, relPos: Vec3f, radius: f32, comptime addTerrain: bool) void {
const lodScale = map.pos.voxelSize;

@IntegratedQuantum IntegratedQuantum Jul 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please stick to the existing naming convention

Suggested change
const lodScale = map.pos.voxelSize;
const voxelSize = map.pos.voxelSize;

const scaledWidth = CaveMapFragment.width*lodScale;
const scaledHeight = CaveMapFragment.height*lodScale;

// Makes walls rough by adding a 1-in-roughnessChance chance that blocks
// remain unchanged.
const roughnessChance = 6;
const terrainShaper = if (addTerrain) CaveMapFragment.addRange else CaveMapFragment.removeRange;

@IntegratedQuantum IntegratedQuantum Jul 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think function pointers are generally harder to read and argue about.
How about instead adding a helper function to CaveMapFragment called modifyTerrain that gets the same comptime parameter?

@BoringOrng BoringOrng Jul 23, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do like the idea of this, but I will challenge doing the same comptime-parameter, if it were up to me, I think a signature that looks more or less like this would be more explicit as to what is being done:

modifyTerrain(self: *CaveMapFragment, modifier: enum { add, remove }, ...)

If I do go this route, would you want me to make the _relX: i32, _relY: i32 something like _rel: Vec2i, and same with the _start: i32, _end: i32 parameters? Another thing is if you do wanna do the modifier-enum, should I make that the input to generateSphere_ too in place of addTerrain?

Also, slight sidenote, this function doesn't even benefit from using the comptime addTerrain, because the only time it's called is in generateSphere where the input to that parameter is essentially just radius < 0. Honestly, I could inline the generateSphere_ function into the generateSphere function and make the appropriate changes there.

@IntegratedQuantum IntegratedQuantum Jul 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, an enum is always better, and I'd prefer to use the same enum in both places.

If I do go this route, would you want me to make the _relX: i32, _relY: i32 something like _rel: Vec2i

I'd suggest not to do too many changes in this one PR. Also it is kind of annoying to convert a Vec3i to Vec2i currently due to lack of swizzling in the language.

could inline the generateSphere_ function into the generateSphere function and make the appropriate changes there.

I think when inlined you'd have trouble turning radius < 0 into a comptime value.


const relX = relPos[0];
const relY = relPos[1];
const relZ = relPos[2];
var xMin = @as(i32, @trunc(relX - radius)) - 1;
xMin = @max(xMin, 0);
var xMax = @as(i32, @trunc(relX + radius)) + 1;
xMax = @min(xMax, CaveMapFragment.width*map.pos.voxelSize);
var yMin = @as(i32, @trunc(relY - radius)) - 1;
yMin = @max(yMin, 0);
var yMax = @as(i32, @trunc(relY + radius)) + 1;
yMax = @min(yMax, CaveMapFragment.width*map.pos.voxelSize);
if (xMin >= xMax or yMin >= yMax or relZ - radius + 1 >= @as(f32, @floatFromInt(CaveMapFragment.height*map.pos.voxelSize)) or relZ + radius + 1 < 0) {

const xDist = displaceByHalfExtentBounded(relX, radius, scaledWidth);

@IntegratedQuantum IntegratedQuantum Jul 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you make displaceByHalfExtentBounded return just struct {i32, i32}, then you could use tuple destructuring here.
Furthermore nowadays we usually prefer vectors over seperate x/y/z variables, also I do think the name is a bit awkward.
Putting it all together

Suggested change
const xDist = displaceByHalfExtentBounded(relX, radius, scaledWidth);
const min, const max = getSphereBounds(.{relX, relY, relZ}, radius, scaledWidth);

@BoringOrng BoringOrng Jul 23, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not exactly a huge fan on doing just min or max, thoughts on suffixing some -Dist or Bound? I think that would clarify the intent a bit better.

@IntegratedQuantum IntegratedQuantum Jul 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

const yDist = displaceByHalfExtentBounded(relY, radius, scaledWidth);
const zDist = displaceByHalfExtentBounded(relZ, radius, scaledHeight);

if (xDist.min >= xDist.max or yDist.min >= yDist.max or zDist.min >= zDist.max) {
return;
}

const radiusSq = radius*radius;
const invRadiusSq = 1.0/(radiusSq);
const thresholdXY = 0.9*0.9*radiusSq;

// Go through all blocks within range of the sphere center and remove them.
var curX = xMin;
while (curX < xMax) : (curX += map.pos.voxelSize) {
const distToCenterX = (@as(f32, @floatFromInt(curX)) - relX)/radius;
var curY = yMin;
while (curY < yMax) : (curY += map.pos.voxelSize) {
const distToCenterY = (@as(f32, @floatFromInt(curY)) - relY)/radius;
const xyDistanceSquared = distToCenterX*distToCenterX + distToCenterY*distToCenterY;
var curX = xDist.min;
while (curX < xDist.max) : (curX += lodScale) {
const dx = @as(f32, @floatFromInt(curX)) - relX;
const dxSq = dx*dx;

var curY = yDist.min;
while (curY < yDist.max) : (curY += lodScale) {
const dy = @as(f32, @floatFromInt(curY)) - relY;
const dySq = dy*dy;
const xySumSq = dxSq + dySq;

@IntegratedQuantum IntegratedQuantum Jul 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be no performance difference to do the squaring here, avoiding the helper constants
Also I'd prefer the full name, as abbreviations can be confusing to outsiders.

Suggested change
const xySumSq = dxSq + dySq;
const xySumSquare = dx*dx + dy*dy;

@BoringOrng BoringOrng Jul 23, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll expand out the -Sq suffix but I do think dxSquare should be it's own helper-constant, not because I don't trust the compiler to inline it anyway, but because it makes more sense (at least to me) to read it being squared right where it's calculated, and not within another loop.

@IntegratedQuantum IntegratedQuantum Jul 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would disagree. I prefer reading arithmetic dx*dx instead of separate variable names like dxSquare.
As a reader/debugger dx*dx is unambiguous, whereas dxSquare is something where I'd need to check the definition.


var zMin: i32 = @trunc(relZ);
var zMax: i32 = @trunc(relZ);
if (xyDistanceSquared < 0.9*0.9) {
const zDistance = radius*@sqrt(0.9*0.9 - xyDistanceSquared);
if (xySumSq < thresholdXY) {
const zDistance = @sqrt(thresholdXY - xySumSq);
zMin = @trunc(relZ - zDistance);
zMax = @trunc(relZ + zDistance);
if (addTerrain) {
map.addRange(curX, curY, zMin, zMax); // Add the center range in a single call.
} else {
map.removeRange(curX, curY, zMin, zMax); // Remove the center range in a single call.
}
terrainShaper(map, curX, curY, zMin, zMax);
}
// Add some roughness at the upper cave walls:

// Add some roughness to the upper cave walls:
var curZ: i32 = zMax;
while (curZ <= CaveMapFragment.height*map.pos.voxelSize) : (curZ += map.pos.voxelSize) {
const distToCenterZ = (@as(f32, @floatFromInt(curZ)) - relZ)/radius;
const distToCenter = distToCenterZ*distToCenterZ + xyDistanceSquared;
if (distToCenter < 1) {
// Add a small roughness parameter to make walls look a bit rough by filling only 5/6 of the blocks at the walls with air:
if (random.nextIntBounded(u8, seed, 6) != 0) {
if (addTerrain) {
map.addRange(curX, curY, curZ, curZ + 1);
} else {
map.removeRange(curX, curY, curZ, curZ + 1);
}
}
} else break;
while (curZ <= scaledHeight) : (curZ += lodScale) {
const dz = @as(f32, @floatFromInt(curZ)) - relZ;
const distToCenter = (xySumSq + dz*dz)*invRadiusSq;
if (distToCenter >= 1) break;

if (random.nextIntBounded(u8, seed, roughnessChance) != 0) {
terrainShaper(map, curX, curY, curZ, curZ + 1);
}
}
// Add some roughness at the lower cave walls:

// Add some roughness to the lower cave walls:
curZ = zMin;
while (curZ >= 0) : (curZ -= map.pos.voxelSize) {
const distToCenterZ = (@as(f32, @floatFromInt(curZ)) - relZ)/radius;
const distToCenter = distToCenterZ*distToCenterZ + xyDistanceSquared;
if (distToCenter < 1) {
// Add a small roughness parameter to make walls look a bit rough by filling only 5/6 of the blocks at the walls with air:
if (random.nextIntBounded(u8, seed, 6) != 0) {
if (addTerrain) {
map.addRange(curX, curY, curZ, curZ + 1);
} else {
map.removeRange(curX, curY, curZ, curZ + 1);
}
}
} else break;
while (curZ >= 0) : (curZ -= lodScale) {
const dz = @as(f32, @floatFromInt(curZ)) - relZ;
const distToCenter = (xySumSq + dz*dz)*invRadiusSq;
if (distToCenter >= 1) break;

if (random.nextIntBounded(u8, seed, roughnessChance) != 0) {
terrainShaper(map, curX, curY, curZ, curZ + 1);
}
}
}
}
Expand Down
Loading