Stop panic when blueprint doesn't exist for biome by BoySanic · Pull Request #2195 · PixelGuys/Cubyz

BoySanic · 2025-11-02T21:10:05Z

I just have it returning null if it can't find the blueprint and printing an error in the log.
It should work the same as if the id was not found.
Fixes #1898
Fixes #1932

…and print error message

src/server/terrain/biomes.zig

Argmaster · 2025-11-02T21:41:02Z

src/server/terrain/biomes.zig

 	};
 	const VTable = struct {
-		loadModel: *const fn(parameters: ZonElement) *anyopaque,
+		loadModel: *const fn(parameters: ZonElement) ?*anyopaque,


Although compiler seems to ignore it, I think it is unsafe to use ?*anyopaque here without changing the return type in all structure implementation. Current implementation can support it, but its definitely not the part of the contract. Even if it would be completely safe, for sake of consistency return type in every implementation of loadModel should be optional.

Could you then please also adjust the corresponding casting functions (utils.castFunctionReturnToAnyopaque and utils.castFunctionSelfToAnyopaque) so they do throw a compiler error on optional mismatch.

I'm not entirely sure what you mean by optional mismatch, but I'll give it my best shot.

I think I'm piecing it together.

You want a compiler error thrown if something is added that implements loadModel but it tries to return a non-optional anyopaque

On second thought there's more nuance to that.
It needs to know whether to expect an optional anyopaque or just an anyopaque. I'm not sure how it can know that, yet.

The simplest solution seems to be to create a new function for optionals.

I can't seem to figure out a cleaner way that keeps everything in one generalized function.

Okay I pushed up an attempt at this. It feels wrong, though.

… message is logged

IntegratedQuantum · 2025-11-04T17:55:45Z

src/server/terrain/simple_structures/SbbGen.zig

-pub fn loadModel(parameters: ZonElement) *SbbGen {
+pub fn loadModel(parameters: ZonElement) ?*SbbGen {
 	const structureId = parameters.get(?[]const u8, "structure", null) orelse {
 		main.utils.panicWithMessage("Error loading generator 'cubyz:sbb' structure field is mandatory.", .{});


Please also fix this while you are at it.

Done, this should now fix #1932 too.
I think my implementation might be a little ugly but I can clean that up later.

The root cause of the issue is that we resize structureList to a size larger than it would be if some sbbs are discarded.

Then when we loop over it, the last n items of the slice are invalid memory. (address aaaaaaaaa... and so on)

My fix is to bound the slice we loop over by how many items made it through registration. (0..stage1Count) Naming can be adjusted.

I was thinking about this some more, and an ideal approach would resize the list to the new size.

I couldn't think of a good way to do this, the typical resize function doesn't allow you to make the list smaller.

I guess we could supply a new, smaller list with the 0..stage1Count slice to reassign to structureList? Unless there's some obvious way I'm missing to reduce list size

Please read

Cubyz/src/utils/list.zig

Line 15 in 64b5813

pub fn List(comptime T: type) type {

Whole implementation
You will find solution to your problem there. A pretty one.

Just so it's clear, none cares if we use the whole allocation or if we waste few entries of memory, only correctness matter.
If solution doesn't occur to you after reading List implementation I will gladly suggest a stage1Couny-free solution without further riddles.

To be clear, it does not seem to add capacity to the list if we hit the catch on initFromZon. It cannot call the function to add capacity until the parameter is resolved.

Yes, there is a bug in current implementation, at least after reviewing the code on phone I think I see it. Uninitialized objects should have been removed.

so you guarantee that your list doesn't use 2N storage due to resizes, if someone allocate something else in meantime

I'm not following. It'll only add capacity to the list when it adds a sbb during world load, which is discarded by the worldArena once the world closes. Where's 2N coming from here?

Exactly here. World arena exists throughout whole gameplay, as long as the world is open. Therefore you will keep those unused allocations for most of the runtime. When you start appending, a list will start with small size, eg. N/8, then after reaching it's capacity (mind, not size, capacity!) it will reallocate (not resize!) to N/4 and repeat the cycle until it reaches N + possibly some overshoot due to X2 growth. Under certain circumstances allocation will not be resized (ie. won't be extended without changing address) but will be changed to completely different place, hence leaking the memory until world arena is reset.

Again, there is a difference between size and capacity, allocation size and array size.

Therefore you will keep those unused allocations for most of the runtime.

But my implementation avoids the unused allocations altogether?

It won't run .append if .initFromZon throws an error. It resolves the parameters before it ever calls the function.

Measured via the arenaAllocator.queryCapacity function:
My branch:

worldArena capacity before sbbs: 2950248 (usize)
worldArena capacity after sbbs: 2950248 (usize)

master:
worldArena capacity before sbbs: 2950492 (usize)
worldArena capacity after sbbs: 2950492 (usize)

Ok, remove line with preallocation of hashmap memory and try checking size of the arena again.

structureMap.ensureTotalCapacity(main.worldArena.allocator, structures.count()) catch unreachable;

I am not sure it will expose the problem, because I am not sure what queryCapacity actually returns, but maybe it will be visible.

Regardless of the outcome, it's not about current state of implementation, is about the semantics and potential future bugs.
When you call append on a list, it doesn't change it's allocation size by exactly one. It allocates continuous array of capacity of X elements but exposes size of 1. Then, you can freely append to it without reallocating until you try to append X+1 element. Then, list requests a resize (reallocation) of it's allocation to 2*X size. Allocator can decide to either grow that allocation, if it has unused space behind it, or has to create new memory region, copy old allocation into it and return a new pointer. Regardless of the path, list will expose size of X+1, but will have capacity of 2*X. This will be repeated until list can contain all N elements. In case of Arena allocator, second reallocation path will result in old allocation being unused, but still owned by the arena.

src/server/terrain/biomes.zig

src/server/terrain/structure_building_blocks.zig

IntegratedQuantum · 2025-11-08T12:30:21Z

src/utils.zig

 	typeInfo.@"fn".params = params[0..];
 	return @Type(typeInfo);
 }
+fn CastFunctionSelfToOptionalAnyopaqueType(Fn: type) type {


What would an optional self parameter mean, an how would it ever be useful?
Don't add functions that we don't need.

IntegratedQuantum · 2025-11-08T12:32:55Z

src/utils.zig

+
+fn CastFunctionReturnToOptionalAnyopaqueType(Fn: type) type {
+	var typeInfo = @typeInfo(Fn);
+	if(@sizeOf(typeInfo.@"fn".return_type.?) != @sizeOf(?*anyopaque) or @alignOf(typeInfo.@"fn".return_type.?) != @alignOf(?*anyopaque) or @typeInfo(typeInfo.@"fn".return_type.?) != .optional) {


Please also adjust the non-optional function to do the inverse check.

Argmaster · 2025-11-08T15:00:51Z

src/server/terrain/structure_building_blocks.zig

 	std.debug.assert(structureList.items.len == 0);
 	std.debug.assert(structureMap.capacity() == 0);
-
-	structureList.resize(main.worldArena, structures.count());


Please add ensureCapacity call with structures.count() to ensure structureList never has to reallocate it's storage.

src/server/terrain/structure_building_blocks.zig

src/utils.zig

src/server/terrain/structure_building_blocks.zig

I just have it returning null if it can't find the blueprint and printing an error in the log. It should work the same as if the id was not found. Fixes #1898 Fixes #1932

Stop panic when blueprint doesn't exist for biome, replace with null …

4b3c5c3

…and print error message

Argmaster suggested changes Nov 2, 2025

View reviewed changes

Change in all implementations of the function, adjust where the error…

be2377c

… message is logged

BoySanic added this to PRs to review Nov 3, 2025

BoySanic moved this to Easy to Review in PRs to review Nov 3, 2025

Argmaster approved these changes Nov 3, 2025

View reviewed changes

Argmaster requested a review from IntegratedQuantum November 3, 2025 19:52

codemob-dev moved this from Easy to Review to In review in PRs to review Nov 3, 2025

IntegratedQuantum requested changes Nov 4, 2025

View reviewed changes

Add fix for other panic

85163f0

Argmaster reviewed Nov 5, 2025

View reviewed changes

src/server/terrain/structure_building_blocks.zig Outdated Show resolved Hide resolved

rename var, eliminate one var

b130d9a

Argmaster reviewed Nov 5, 2025

View reviewed changes

src/server/terrain/structure_building_blocks.zig Show resolved Hide resolved

BoySanic added 4 commits November 5, 2025 09:41

refactor

1608bab

Put argmaster's line back

5b68768

fix format

0396526

Crude attempt at compile errors for optional mismatch

a84ac30

IntegratedQuantum requested changes Nov 8, 2025

View reviewed changes

Argmaster reviewed Nov 8, 2025

View reviewed changes

BoySanic added 3 commits November 8, 2025 07:29

Temp change

0e63b80

Remove unneeded functions, add inverse check

548d388

Add ensureCapacity call

b8b9006

IntegratedQuantum requested changes Nov 8, 2025

View reviewed changes

src/server/terrain/structure_building_blocks.zig Outdated Show resolved Hide resolved

src/utils.zig Show resolved Hide resolved

src/utils.zig Show resolved Hide resolved

BoySanic added 4 commits November 8, 2025 07:59

Put lines back, use appendAssumeCapacity

f42ed9a

Put newlines back

7a29b98

Put line back

35284b5

Fix format

527f775

IntegratedQuantum requested changes Nov 8, 2025

View reviewed changes

src/server/terrain/structure_building_blocks.zig Outdated Show resolved Hide resolved

Put var back in scope

3bf3285

IntegratedQuantum approved these changes Nov 8, 2025

View reviewed changes

IntegratedQuantum merged commit 57659b7 into PixelGuys:master Nov 8, 2025
1 check passed

Conversation

BoySanic commented Nov 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Argmaster Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Argmaster Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Argmaster Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Argmaster Nov 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

BoySanic commented Nov 2, 2025 •

edited

Loading

Argmaster Nov 5, 2025 •

edited

Loading

Argmaster Nov 5, 2025 •

edited

Loading

Argmaster Nov 5, 2025 •

edited

Loading

Argmaster Nov 8, 2025 •

edited

Loading