From b1af1be3d27c1b5a7641f414774497c2b272de56 Mon Sep 17 00:00:00 2001 From: IntegratedQuantum <43880493+IntegratedQuantum@users.noreply.github.com> Date: Fri, 6 Jun 2025 19:26:21 +0200 Subject: [PATCH] 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 | 39 ++++++++++++++++++++++++++++++----- src/game.zig | 3 ++- src/gui/windows/workbench.zig | 1 - src/server/server.zig | 12 ++++++++--- src/server/world.zig | 7 +++++-- 5 files changed, 50 insertions(+), 12 deletions(-) diff --git a/src/Inventory.zig b/src/Inventory.zig index ec5c26d2..c0ce1a1a 100644 --- a/src/Inventory.zig +++ b/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, diff --git a/src/game.zig b/src/game.zig index d49ec23d..0b202363 100644 --- a/src/game.zig +++ b/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); diff --git a/src/gui/windows/workbench.zig b/src/gui/windows/workbench.zig index a677b65a..9fb252b4 100644 --- a/src/gui/windows/workbench.zig +++ b/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(); diff --git a/src/server/server.zig b/src/server/server.zig index 07aaaffb..63dab225 100644 --- a/src/server/server.zig +++ b/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); diff --git a/src/server/world.zig b/src/server/world.zig index 86ad4dad..501ae1e5 100644 --- a/src/server/world.zig +++ b/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;