Cleanup and fix the deposit on close behavior of the workbench. (#1602)

In the process I had to improve how the player inventory is handled,
which required adding an interface to externally load an inventory for
the inventory system.

This new interface is also relevant for block inventories, which is why
I would like you to review this @Argmaster

fixes #978

src/Inventory.zig

@@ -170,13 +170,17 @@ pub const Sync = struct { // MARK: Sync
 			inv: Inventory,
 			users: main.ListUnmanaged(*main.server.User),
 			source: Source,
+			managed: Managed,
 
-			fn init(len: usize, typ: Inventory.Type, source: Source) ServerInventory {
+			const Managed = enum {internallyManaged, externallyManaged};
+
+			fn init(len: usize, typ: Inventory.Type, source: Source, managed: Managed) ServerInventory {
 				main.utils.assertLocked(&mutex);
 				return .{
 					.inv = Inventory._init(main.globalAllocator, len, typ, .server),
 					.users = .{},
 					.source = source,
+					.managed = managed,
 				};
 			}
 
@@ -187,6 +191,7 @@ pub const Sync = struct { // MARK: Sync
 				self.inv._deinit(main.globalAllocator, .server);
 				self.inv._items.len = 0;
 				self.source = .alreadyFreed;
+				self.managed = .internallyManaged;
 			}
 
 			fn addUser(self: *ServerInventory, user: *main.server.User, clientId: u32) void {
@@ -199,7 +204,11 @@ pub const Sync = struct { // MARK: Sync
 				main.utils.assertLocked(&mutex);
 				_ = self.users.swapRemove(std.mem.indexOfScalar(*main.server.User, self.users.items, user).?);
 				std.debug.assert(user.inventoryClientToServerIdMap.fetchRemove(clientId).?.value == self.inv.id);
-				if(self.users.items.len == 0) {
+				if(self.users.items.len == 0 and self.managed == .internallyManaged) {
+					if(self.inv.type.shouldDepositToUserOnClose()) {
+						const playerInventory = getInventoryFromSource(.{.playerInventory = user.id}) orelse @panic("Could not find player inventory");
+						Sync.ServerSide.executeCommand(.{.depositOrDrop = .{.dest = playerInventory, .source = self.inv}}, null);
+					}
 					self.deinit();
 				}
 			}
@@ -304,10 +313,26 @@ pub const Sync = struct { // MARK: Sync
 			executeCommand(payload, source);
 		}
 
+		pub fn createExternallyManagedInventory(len: usize, typ: Inventory.Type, source: Source, zon: ZonElement) u32 {
+			mutex.lock();
+			defer mutex.unlock();
+			const inventory = ServerInventory.init(len, typ, source, .externallyManaged);
+			inventories.items[inventory.inv.id] = inventory;
+			inventory.inv.loadFromZon(zon);
+			return inventory.inv.id;
+		}
+
+		pub fn destroyExternallyManagedInventory(invId: u32) void {
+			mutex.lock();
+			defer mutex.unlock();
+			std.debug.assert(inventories.items[invId].managed == .externallyManaged);
+			inventories.items[invId].deinit();
+		}
+
 		fn createInventory(user: *main.server.User, clientId: u32, len: usize, typ: Inventory.Type, source: Source) void {
 			main.utils.assertLocked(&mutex);
 			switch(source) {
-				.sharedTestingInventory, .recipe, .blockInventory => {
+				.sharedTestingInventory, .recipe, .blockInventory, .playerInventory, .hand => {
 					for(inventories.items) |*inv| {
 						if(std.meta.eql(inv.source, source)) {
 							inv.addUser(user, clientId);
@@ -315,10 +340,10 @@ pub const Sync = struct { // MARK: Sync
 						}
 					}
 				},
-				.playerInventory, .hand, .other => {},
+				.other => {},
 				.alreadyFreed => unreachable,
 			}
-			const inventory = ServerInventory.init(len, typ, source);
+			const inventory = ServerInventory.init(len, typ, source, .internallyManaged);
 
 			inventories.items[inventory.inv.id] = inventory;
 			inventories.items[inventory.inv.id].addUser(user, clientId);
@@ -1823,6 +1848,10 @@ const Type = union(TypeEnum) {
 	creative: void,
 	crafting: void,
 	workbench: *const main.items.ToolType,
+
+	pub fn shouldDepositToUserOnClose(self: Type) bool {
+		return self == .workbench;
+	}
 };
 type: Type,
 id: u32,

src/game.zig

@@ -463,6 +463,7 @@ pub const Player = struct { // MARK: Player
 	pub var isGhost: Atomic(bool) = .init(false);
 	pub var hyperSpeed: Atomic(bool) = .init(false);
 	pub var mutex: std.Thread.Mutex = .{};
+	pub const inventorySize = 32;
 	pub var inventory: Inventory = undefined;
 	pub var selectedSlot: u32 = 0;
 
@@ -723,7 +724,7 @@ pub const World = struct { // MARK: World
 
 		try assets.loadWorldAssets("serverAssets", self.blockPalette, self.itemPalette, self.toolPalette, self.biomePalette);
 		Player.id = zon.get(u32, "player_id", std.math.maxInt(u32));
-		Player.inventory = Inventory.init(main.globalAllocator, 32, .normal, .{.playerInventory = Player.id});
+		Player.inventory = Inventory.init(main.globalAllocator, Player.inventorySize, .normal, .{.playerInventory = Player.id});
 		Player.loadFrom(zon.getChild("player"));
 		self.playerBiome = .init(main.server.terrain.biomes.getPlaceholderBiome());
 		main.audio.setMusic(self.playerBiome.raw.preferredMusic);

src/gui/windows/workbench.zig

@@ -90,7 +90,6 @@ fn openInventory() void {
 }
 
 fn closeInventory() void {
-	main.game.Player.inventory.depositOrDrop(inv);
 	inv.deinit(main.globalAllocator);
 	if(window.rootComponent) |*comp| {
 		comp.deinit();

src/server/server.zig

@@ -114,6 +114,8 @@ pub const User = struct { // MARK: User
 	lastSentBiomeId: u32 = 0xffffffff,
 
 	inventoryClientToServerIdMap: std.AutoHashMap(u32, u32) = undefined,
+	inventory: ?u32 = null,
+	handInventory: ?u32 = null,
 
 	connected: Atomic(bool) = .init(true),
 
@@ -137,16 +139,20 @@ pub const User = struct { // MARK: User
 	pub fn deinit(self: *User) void {
 		std.debug.assert(self.refCount.load(.monotonic) == 0);
 
+		main.items.Inventory.Sync.ServerSide.disconnectUser(self);
+		std.debug.assert(self.inventoryClientToServerIdMap.count() == 0); // leak
+		self.inventoryClientToServerIdMap.deinit();
+
 		world.?.savePlayer(self) catch |err| {
 			std.log.err("Failed to save player: {s}", .{@errorName(err)});
 			return;
 		};
 
+		if(self.inventory) |inv| main.items.Inventory.Sync.ServerSide.destroyExternallyManagedInventory(inv);
+		if(self.handInventory) |inv| main.items.Inventory.Sync.ServerSide.destroyExternallyManagedInventory(inv);
+
 		self.worldEditData.deinit();
 
-		main.items.Inventory.Sync.ServerSide.disconnectUser(self);
-		std.debug.assert(self.inventoryClientToServerIdMap.count() == 0); // leak
-		self.inventoryClientToServerIdMap.deinit();
 		self.unloadOldChunk(.{0, 0, 0}, 0);
 		self.conn.deinit();
 		main.globalAllocator.free(self.name);

src/server/world.zig

@@ -863,6 +863,9 @@ pub const ServerWorld = struct { // MARK: ServerWorld
 
 			main.items.Inventory.Sync.setGamemode(user, std.meta.stringToEnum(main.game.Gamemode, playerData.get([]const u8, "gamemode", @tagName(self.defaultGamemode))) orelse self.defaultGamemode);
 		}
+
+		user.inventory = main.items.Inventory.Sync.ServerSide.createExternallyManagedInventory(main.game.Player.inventorySize, .normal, .{.playerInventory = user.id}, playerData.getChild("playerInventory"));
+		user.handInventory = main.items.Inventory.Sync.ServerSide.createExternallyManagedInventory(1, .normal, .{.hand = user.id}, playerData.getChild("hand"));
 	}
 
 	pub fn savePlayer(self: *ServerWorld, user: *User) !void {
@@ -891,11 +894,11 @@ pub const ServerWorld = struct { // MARK: ServerWorld
 			defer main.items.Inventory.Sync.ServerSide.mutex.unlock();
 			if(main.items.Inventory.Sync.ServerSide.getInventoryFromSource(.{.playerInventory = user.id})) |inv| {
 				playerZon.put("playerInventory", inv.save(main.stackAllocator));
-			}
+			} else @panic("The player inventory wasn't found. Cannot save player data.");
 
 			if(main.items.Inventory.Sync.ServerSide.getInventoryFromSource(.{.hand = user.id})) |inv| {
 				playerZon.put("hand", inv.save(main.stackAllocator));
-			}
+			} else @panic("The player hand inventory wasn't found. Cannot save player data.");
 		}
 
 		const playerPath = std.fmt.allocPrint(main.stackAllocator.allocator, "saves/{s}/players", .{self.path}) catch unreachable;