Introduce new Sync command `CraftFrom` and make the crafting inventories client side only #2506

Wunka · 2026-01-19T19:18:01Z

fix the check for deserialize (As already mentioned in DepositToAny slice for the destination #2469 (comment) I am not that knowledgable on the reader code so if anybody has a better idea here, it would be very welcome)
find out if any mechanic was removed (what scenario made Drop call tryCrafting?)

This PR also replaces #2483 as its features are also done here.
CraftFrom accepts as source a slice to prepare for #2324 or/and making the user able to craft from items in a chest

Argmaster · 2026-01-20T09:27:42Z

Ok, so I am getting a bit confused by the description. What exactly is getting client sided here? Recipe resolution right? Not whole item transformation?
I admit my knowledge is a little bit outdated due to recent refactors I didn't have time to review, so please forgive my stupid questions.

Wunka · 2026-01-20T09:34:21Z

Ok, so I am getting a bit confused by the description. What exactly is getting client sided here? Recipe resolution right? Not whole item transformation? I admit my knowledge is a little bit outdated due to recent refactors I didn't have time to review, so please forgive my stupid questions.

For reference see: #2500 . There Quantum introduced the ClientInventory this can either be synced with a Normal inventory on the server or can be completly be handeld on client side. This PR introduces the CraftFrom sync so that the server doesn't have to have access to the crafting inventories (which are displayed in the crafting menu). Which makes it possible to make them also only client side as is the creative menu through #2500

Argmaster · 2026-01-20T09:45:56Z

Aight, since you are clearly keeping up with the topic, could you please answer for sake of documentation: Can the client invoke arbitrary item transformation that was not defined by server side recipe table? Will the server reject such transformation?

Wunka · 2026-01-20T09:49:51Z

src/sync.zig

+			// check if recipe is valid
+			const found = blk: {
+				outer: for(main.items.recipes()) |*recipe| {
+					if(recipe.resultItem != resultStack.item.baseItem) continue;
+					if(recipe.resultAmount != resultStack.amount) continue;
+					if(recipe.sourceItems.len != sourceStacksSize) continue;
+					for(recipe.sourceItems, recipe.sourceAmounts, sourceStacks) |recipeItem, recipeAmount, sourceStack| {
+						if(recipeItem != sourceStack.item.baseItem) continue :outer;
+						if(recipeAmount != sourceStack.amount) continue :outer;
+					}
+					break :blk true;
+				}
+				break :blk false;
+			};
+			if(!found) return error.Invalid;


This looks if the recipe actually exist, but I have not yet tested if there is a flaw in my logic here.

No worries, I am just trying to understand the intentions

Argmaster · 2026-01-20T10:44:30Z

src/sync.zig

+		}
+
+		fn deserialize(reader: *BinaryReader, side: Side, user: ?*main.server.User) !CraftFrom {
+			const destinationsSize = try reader.readVarInt(usize);


Oh yes, varInt. (This comment mainly to addressed @IntegratedQuantum) How often is this message send so this varInt makes sense here? It's saving 6? 7? Bytes. Out of how large impact over eg. Hour of average gameplay? Anyway, my skepticism aside, shouldn't we make the enums varInts too? Since we care about those few bytes, I am almost sure we are wasting double that when all the entries are summed up.

Btw this is destinationCount (possibly with plural destinations), size has to strong size-in-bytes connotations in this context.

Well, let me point you to the contributing guidelines, using varint is non-pessimization. Of course in this case it makes no difference, but if we save a few bytes in each packet, then suddenly we are looking at different dimensions.
And there is no reason not to use varint, it's just 3 characters more to write.

You are also correct about the enums here, since they (and all non-exhaustive enums) are indices, it would make sense to serialize them as varints as well, and this might actually make a meaningful enough difference for item drop/entitiy/projectile ids. So maybe we should think about automatically doing this in the background.

src/sync.zig

Argmaster · 2026-01-20T10:57:06Z

src/sync.zig

+			for(self.sources) |source| if(source.type != .normal) return;
+
+			// Can we even craft it?
+			outer: for(self.sourceStacks) |requiredStack| {


I suspect we have to wait with improvements for next PR?
This cries for a helper function. Abstraction level in here is too low and the function is too long overall.
This is clearly a separate function that would be better off with lower indentation level and a proper name, rather than "outer" label.

Ye without a proper abstraction level adjustment I am not reviewing that, but I know a person who will.

Argmaster · 2026-01-20T11:03:37Z

src/sync.zig

+			const sourceStacksSize = try reader.readVarInt(usize);
+			if(sourceStacksSize == 0) return error.Invalid;
+			std.log.info("sourceStacksSize: {d}, sourceStacksSize*@sizeOf(ItemStack): {d}, remaining: {d}\n", .{sourceStacksSize, sourceStacksSize*@sizeOf(ItemStack), reader.remaining.len});
+			// if(sourceStacksSize*@sizeOf(ItemStack) > reader.remaining.len) return error.Invalid; // this check sadly fails


When writting the code first I changed to > because this is the last read so if its maybe = then this would still be ok.

Anyway my suggestion is to read until it fails and hard cap max count we can process, as described In nearby comment.

Argmaster · 2026-01-20T11:11:20Z

src/sync.zig

+			}
+		}
+
+		fn deserialize(reader: *BinaryReader, side: Side, user: ?*main.server.User) !CraftFrom {


I think we could unit test this fairly easily. We would have to be naught and add mock entry to Side enums that result in getInventory being noop and reserve a sentinel InventoryId, but we could get serialization edge case coverage from that. Alternatively we can just implement declarative serialization C: nvm we can't easily make a dymmy CraftFrom object.

src/sync.zig

Argmaster · 2026-01-20T11:24:28Z

src/sync.zig

+		fn deserialize(reader: *BinaryReader, side: Side, user: ?*main.server.User) !CraftFrom {
+			const destinationsSize = try reader.readVarInt(usize);
+			if(destinationsSize == 0) return error.Invalid;
+			if(destinationsSize*@sizeOf(InventoryId) >= reader.remaining.len) return error.Invalid;


Do we have to check this ahead of time? We can just for loop for destinationCount times and try reading. Reader should report error when it runs out of buffer data I think? We then catch it and possibly transform to Invalid or whatever.

Comes from https://github.com/PixelGuys/Cubyz/pull/2469/changes#r2679023133

Well, then I guess it's bad I missed that one. Suggestion still remains valid IMO, unless proven otherwise.

Tho I admit that IMO a best solution would be an automagical serialization for structs, but Quantum disagrees.

Argmaster · 2026-01-20T11:32:24Z

src/sync.zig

+				sourceStack.* = try ItemStack.fromBytes(reader);
+			}
+
+			// check if recipe is valid


Very pretty comment. Now make it a function name.

This is extracted from: #2506 (comment) Initially this was only supposed to be a helper function, but as I wanted to improve the abstraction it grew into a struct to avoid namespace pollution. This will also be helpful if #2506 is not accepted, as I have at least one other slice refactor that currently copies this code (#2494), and we may extend additional functions to slices or make more operations use the same priority system.

Wunka · 2026-01-24T11:01:05Z

I have updated this to use the put_items_into.
I also looked what scenario made Drop call tryCraftingTo and I didn't find any scenario, so the remove seems to not remove any mechanic
I see for now no conclusion to the deserialize problem, so I didn't change anything there.
In my opinion the run function from CraftFrom has now engough abstraction but if there are different opinions I can move more code to helper functions (There is also this comment: PutItemsInto Helper #2515 (comment) so they may be changes to this order in the future in a follow up PR)

Wunka · 2026-01-25T22:39:24Z

this is now updated to use #2524.
I also want to mention that I had an idea. The last read where I get the problems for the check is about the recipe. But we know all recipes, so should I maybe just add a variable to recipe.zig for maxRecipeSourceCount wich while loading the recipes tracks the max amount then I can just compare it to the user given source amount. I think it probably needs a better solution in the future (at least for the inventory fromBytes) but that would at least solve for now the problem that I have no check there.

IntegratedQuantum · 2026-01-26T17:04:27Z

Hmm, I'm not sure. I think for now it would be better to have one clear pattern: Check if the reader has enough data left before allocating.

IntegratedQuantum

also add the check to fromBytes please

src/Inventory.zig

IntegratedQuantum · 2026-01-26T16:52:52Z

src/sync.zig

+
+			// Can we even craft it?
+			for (self.recipe.sourceItems, self.recipe.sourceAmounts) |requiredItem, requiredAmount| {
+				self.sources.removeItems(ctx, requiredAmount, requiredItem);


Please first check if we have the needed ingredients, before removing anything (you accidentally removed that code)

readded. It seems I removed it when introduced the remove_items namespace...

Co-authored-by: IntegratedQuantum <43880493+IntegratedQuantum@users.noreply.github.com>

Wunka · 2026-01-26T18:00:00Z

So now we go back to the problem I described at the beginning of the PR. The check for recipe fromBytes fails. So if you would run this currently the game crashes when crafting something.

IntegratedQuantum · 2026-01-27T16:39:18Z

src/items.zig

+		const resultAmount = try reader.readVarInt(u16);
+		const sourceCount = try reader.readVarInt(usize);
+		if (sourceCount == 0) return error.Invalid;
+		if (sourceCount*@sizeOf(Recipe) > reader.remaining.len) return error.Invalid; // this check fails


Should be @sizeOf(u16) + @sizeOf(BaseItemIndex), but even that doesn't account for the compression of the varint.

So instead, I'd suggest to just use a pair of Lists, that are maybe preallocated to @min(256, sourceCount).

If I understood correctly what you wanted. Its now there. (but please no merge It seems I removed a check so now if you click a sourceitem the game crashes)

IntegratedQuantum · 2026-01-27T16:41:48Z

src/sync.zig

+				// There might be duplicate entries:
+				for (self.recipe.sourceItems, self.recipe.sourceAmounts) |otherItem, otherAmount| {
+					if (std.meta.eql(requiredItem, otherItem))
+						amount += otherAmount;


#2506 (comment)

Co-authored-by: IntegratedQuantum <43880493+IntegratedQuantum@users.noreply.github.com>

src/items.zig

Co-authored-by: IntegratedQuantum <43880493+IntegratedQuantum@users.noreply.github.com>

Wunka · 2026-01-27T17:30:33Z

check is revived.

IntegratedQuantum · 2026-01-28T17:38:20Z

src/gui/gui.zig

-
-		if (itemSlot.inventory.type == .crafting and itemSlot.mode == .takeOnly and mainGuiButton.pressed and (recipeItem != .null or itemSlot.pressed)) {
+		if (itemSlot.inventory.type == .crafting and mainGuiButton.pressed and (recipeItem != .null or itemSlot.pressed)) {
+			if (itemSlot.mode != .takeOnly) return;


Could you explain this change?

I actual thought this was also a case where I needed to add the check again. But yeah... it also works without this change so I reverted that.

IntegratedQuantum · 2026-01-28T17:45:33Z

I'm also still wondering about the other change from that "check is revived" part, why did it need to change?

Wunka · 2026-01-28T17:55:34Z

I'm also still wondering about the other change from that "check is revived" part, why did it need to change?

When you click the left "source" inventories of the recipe menu you are in a recipe inventory but its not takeOnly. so we trigger a depositOrSwap with a non sharedInventory which triggers the assert. Before in depositOrSwap tryCrafting would be called.

src/gui/gui.zig

IntegratedQuantum

a few more things

IntegratedQuantum · 2026-01-28T18:21:30Z

src/items.zig

+		const resultItem = try reader.readEnum(BaseItemIndex);
+		const resultAmount = try reader.readVarInt(u16);
+		const sourceCount = try reader.readVarInt(usize);
+		if (sourceCount == 0) return error.Invalid;


I wouldn't dismiss this so quickly. The rest of the engine allows it, and there is no reason not to allow addon creators to define recipes like this. This might in fact make more sense once tool durability and special crafting stations are added to the mix.

IntegratedQuantum · 2026-01-28T18:25:13Z

src/items.zig

+		var sourceAmounts: main.List(u16) = .initCapacity(main.stackAllocator, @min(256, sourceCount));
+		defer sourceAmounts.deinit();
+
+		while (reader.remaining.len > 0) {


Should add most add sourceCount items. Otherwise we may cause problems if there is other stuff afterwards that expects to read things from the same reader.

src/sync.zig

Co-authored-by: IntegratedQuantum <43880493+IntegratedQuantum@users.noreply.github.com>

IntegratedQuantum

Thank you for working on this, I think it's a great improvement both in terms of readability and feature consistency.
If you are up to it, the next step in that direction would be #2529, it would be amazing if we could get rid of all the edge cases that came with the server-shared inventory type.

Wunka · 2026-01-28T19:00:27Z

If you are up to it, the next step in that direction would be #2529

maybe. I will have to read into the tool logic a little bit before to understand whats exactly going on there.
If I take it up I will again start first with the proposal for the structure of the Sync Operation like I did for this.

Wunka commented Jan 20, 2026

View reviewed changes

Argmaster reviewed Jan 20, 2026

View reviewed changes

src/sync.zig Outdated Show resolved Hide resolved

Argmaster reviewed Jan 20, 2026

View reviewed changes

Wunka commented Jan 20, 2026

View reviewed changes

src/sync.zig Outdated Show resolved Hide resolved

Argmaster reviewed Jan 20, 2026

View reviewed changes

Wunka mentioned this pull request Jan 21, 2026

PutItemsInto Helper #2515

Merged

bagggage added this to PRs to review Jan 23, 2026

bagggage moved this to In review in PRs to review Jan 23, 2026

Wunka added 13 commits January 24, 2026 12:16

try Crafting now accepts slice (sync.zig version)

08ac5ea

same priority system

0eaf344

part 1

a5e00db

kinda working craftFrom

79ca1df

its now ClientSide

b6e03b5

make priority right

e68e4a6

remove check for sync (as its now client side)

5248ecc

format

d5249fb

fix comments

e17b9d3

validRecipe helper function

92604f8

Size -> Count

5fa6137

update to use put_items_into

f577203

did not save the remove...

1133fe1

add errdefer and readd remove

40bee1e

IntegratedQuantum requested changes Jan 26, 2026

View reviewed changes

Wunka and others added 3 commits January 26, 2026 18:07

Update src/Inventory.zig

dd6a338

Co-authored-by: IntegratedQuantum <43880493+IntegratedQuantum@users.noreply.github.com>

readd removed code... why...

56edac5

fixed dumb thing and added the failing check

28b7e64

IntegratedQuantum requested changes Jan 27, 2026

View reviewed changes

Wunka and others added 2 commits January 27, 2026 17:46

Update src/sync.zig

db8891c

Co-authored-by: IntegratedQuantum <43880493+IntegratedQuantum@users.noreply.github.com>

list?

4ca7804

IntegratedQuantum requested changes Jan 27, 2026

View reviewed changes

src/items.zig Outdated Show resolved Hide resolved

Wunka and others added 2 commits January 27, 2026 18:29

check revived

f41dc25

Update src/items.zig

3cf6816

Co-authored-by: IntegratedQuantum <43880493+IntegratedQuantum@users.noreply.github.com>

IntegratedQuantum reviewed Jan 28, 2026

View reviewed changes

revert change

7b86c70

remove being able to take the a source item from recipe menu

a6079d2

IntegratedQuantum reviewed Jan 28, 2026

View reviewed changes

src/gui/gui.zig Show resolved Hide resolved

This was referenced Jan 28, 2026

Split item slots into separate Components #2528

Open

Change how the workbench is synchronized and remove the shared Inventory.Type #2529

Open

added immutable check

6258aed

IntegratedQuantum requested changes Jan 28, 2026

View reviewed changes

Wunka and others added 2 commits January 28, 2026 19:29

Update src/sync.zig

a5a2d6f

Co-authored-by: IntegratedQuantum <43880493+IntegratedQuantum@users.noreply.github.com>

allow recipes with 0 source tiems and use == instead of eql

bff8dfd

IntegratedQuantum approved these changes Jan 28, 2026

View reviewed changes

IntegratedQuantum merged commit 146ed64 into PixelGuys:master Jan 28, 2026
1 check passed

Introduce new Sync command CraftFrom and make the crafting inventories client side only #2506

Introduce new Sync command CraftFrom and make the crafting inventories client side only #2506

Conversation

Wunka commented Jan 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Argmaster commented Jan 20, 2026

Uh oh!

Wunka commented Jan 20, 2026

Uh oh!

Argmaster commented Jan 20, 2026

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!

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 Jan 20, 2026 • 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!

Argmaster Jan 20, 2026 • 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!

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!

Wunka commented Jan 24, 2026

Uh oh!

Wunka commented Jan 25, 2026

Uh oh!

IntegratedQuantum commented Jan 26, 2026

Uh oh!

IntegratedQuantum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Wunka commented Jan 26, 2026

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!

Uh oh!

Wunka commented Jan 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Wunka Jan 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Introduce new Sync command `CraftFrom` and make the crafting inventories client side only #2506

Introduce new Sync command `CraftFrom` and make the crafting inventories client side only #2506

Wunka commented Jan 19, 2026 •

edited

Loading

Argmaster Jan 20, 2026 •

edited

Loading

Argmaster Jan 20, 2026 •

edited

Loading

Wunka Jan 28, 2026 •

edited

Loading

Wunka commented Jan 28, 2026 •

edited

Loading