Skip to content

Commit

Permalink
Fix Open & Close inventory events
Browse files Browse the repository at this point in the history
Before this change, inventory events were complete mess.
They could fire multiple times, have no right order
or leave the player and container in a broken state
when cancelled.

Previously we used transactions for them which
would try to rollback the state afterwards
but after trying multiple different aproaches,
this never works and just introduces even more
subtle bugs and hard to diagnose problems.

This change makes the events more of pre
where they used to be more of post type of
event. While this does change the behaviour
of where we no longer can capture the slot changes
for close but instead fire a different event,
this is better than just completly leaving them
at the state where they were.
  • Loading branch information
aromaa committed Jan 12, 2025
1 parent 0062222 commit 491e5c4
Show file tree
Hide file tree
Showing 17 changed files with 376 additions and 144 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/*
* This file is part of Sponge, licensed under the MIT License (MIT).
*
* Copyright (c) SpongePowered <https://www.spongepowered.org>
* Copyright (c) contributors
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in
* all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
* THE SOFTWARE.
*/
package org.spongepowered.neoforge.mixin.inventory.event.entity;

import net.minecraft.world.entity.player.Player;
import net.minecraft.world.inventory.AbstractContainerMenu;
import net.minecraft.world.inventory.InventoryMenu;
import org.spongepowered.asm.mixin.Final;
import org.spongepowered.asm.mixin.Mixin;
import org.spongepowered.asm.mixin.Shadow;

@Mixin(Player.class)
public abstract class PlayerMixin_Inventory_Neo {

// @formatter:off
@Shadow public AbstractContainerMenu containerMenu;
@Shadow @Final public InventoryMenu inventoryMenu;
// @formatter:on
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@
*/
package org.spongepowered.neoforge.mixin.inventory.event.server.level;

import com.llamalad7.mixinextras.injector.wrapoperation.Operation;
import com.llamalad7.mixinextras.injector.wrapoperation.WrapOperation;
import com.llamalad7.mixinextras.sugar.Cancellable;
import net.minecraft.network.RegistryFriendlyByteBuf;
import net.minecraft.server.level.ServerPlayer;
import net.minecraft.world.MenuProvider;
Expand All @@ -34,17 +37,23 @@
import org.spongepowered.asm.mixin.Mixin;
import org.spongepowered.asm.mixin.injection.At;
import org.spongepowered.asm.mixin.injection.Inject;
import org.spongepowered.asm.mixin.injection.Redirect;
import org.spongepowered.asm.mixin.injection.callback.CallbackInfoReturnable;
import org.spongepowered.common.bridge.world.inventory.container.ContainerBridge;
import org.spongepowered.common.event.inventory.InventoryEventFactory;
import org.spongepowered.common.event.tracking.PhaseContext;
import org.spongepowered.common.event.tracking.PhaseTracker;
import org.spongepowered.common.event.tracking.context.transaction.EffectTransactor;
import org.spongepowered.neoforge.mixin.inventory.event.entity.PlayerMixin_Inventory_Neo;

import java.util.OptionalInt;
import java.util.function.Consumer;

@Mixin(ServerPlayer.class)
public class ServerPlayerMixin_Inventory_Neo {
public abstract class ServerPlayerMixin_Inventory_Neo extends PlayerMixin_Inventory_Neo {

// @formatter:off
@Nullable private Object inventory$menuProvider;
// @formatter:on

@Inject(
method = "openMenu(Lnet/minecraft/world/MenuProvider;Ljava/util/function/Consumer;)Ljava/util/OptionalInt;",
Expand All @@ -64,7 +73,14 @@ public class ServerPlayerMixin_Inventory_Neo {
this.inventory$menuProvider = menuProvider;
}

@Redirect(
@Inject(method = "openMenu(Lnet/minecraft/world/MenuProvider;Ljava/util/function/Consumer;)Ljava/util/OptionalInt;", at = @At(value = "INVOKE", target = "Lnet/minecraft/server/level/ServerPlayer;closeContainer()V", shift = At.Shift.AFTER), cancellable = true)
private void impl$openMenuCloseCancelled(final CallbackInfoReturnable<OptionalInt> cir) {
if (this.containerMenu != this.inventoryMenu) {
cir.setReturnValue(OptionalInt.empty());
}
}

@WrapOperation(
method = "openMenu(Lnet/minecraft/world/MenuProvider;Ljava/util/function/Consumer;)Ljava/util/OptionalInt;",
at = @At(
value = "INVOKE",
Expand All @@ -73,13 +89,16 @@ public class ServerPlayerMixin_Inventory_Neo {
)
private AbstractContainerMenu impl$transactMenuCreationWithEffect(
final MenuProvider menuProvider, final int containerCounter, final net.minecraft.world.entity.player.Inventory inventory,
final Player player
final Player player, final Operation<AbstractContainerMenu> original, final @Cancellable CallbackInfoReturnable<OptionalInt> cir
) {
try (final EffectTransactor ignored = PhaseTracker.SERVER.getPhaseContext()
.getTransactor()
.logOpenInventory((ServerPlayer) (Object) this)
) {
return menuProvider.createMenu(containerCounter, inventory, player);
final PhaseContext<?> context = PhaseTracker.SERVER.getPhaseContext();
try (final EffectTransactor ignored = context.getTransactor().logOpenInventory((ServerPlayer) (Object) this)) {
final AbstractContainerMenu menu = original.call(menuProvider, containerCounter, inventory, player);
context.containerLocation().ifPresent(((ContainerBridge) menu)::bridge$setOpenLocation);
if (!InventoryEventFactory.callInteractContainerOpenEvent((ServerPlayer) (Object) this, menu)) {
cir.setReturnValue(OptionalInt.empty());
}
return menu;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
"target": "@env(DEFAULT)",
"priority": 1300,
"mixins": [
"event.entity.PlayerMixin_Inventory_Neo",
"event.server.level.ServerPlayerMixin_Inventory_Neo",
"event.world.level.block.entity.HopperBlockEntityMixin_Inventory_Neo"
]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,4 +47,5 @@ public interface ServerPlayerAccessor {

@Invoker("triggerDimensionChangeTriggers") void invoker$triggerDimensionChangeTriggers(final ServerLevel world);

@Accessor("containerCounter") int accessor$containerCounter();
}
Original file line number Diff line number Diff line change
Expand Up @@ -223,15 +223,14 @@ private static List<SlotTransaction> generateTransactions(final @Nullable Invent
return trans;
}

public static boolean callInteractContainerOpenEvent(final ServerPlayer player) {
final ItemStackSnapshot newCursor = ItemStackUtil.snapshotOf(player.containerMenu.getCarried());
public static boolean callInteractContainerOpenEvent(final ServerPlayer player, final AbstractContainerMenu menu) {
final ItemStackSnapshot newCursor = ItemStackUtil.snapshotOf(menu.getCarried());
final Transaction<ItemStackSnapshot> cursorTransaction = new Transaction<>(ItemStackSnapshot.empty(), newCursor);
final InteractContainerEvent.Open event =
SpongeEventFactory.createInteractContainerEventOpen(PhaseTracker.getCauseStackManager().currentCause(),
(org.spongepowered.api.item.inventory.Container) player.containerMenu, cursorTransaction);
(org.spongepowered.api.item.inventory.Container) menu, cursorTransaction);
SpongeCommon.post(event);
if (event.isCancelled()) {
player.closeContainer();
return false;
}
// Custom cursor
Expand Down Expand Up @@ -282,10 +281,6 @@ public static boolean callInteractContainerOpenEvent(final ServerPlayer player)
return null;
}

if (!InventoryEventFactory.callInteractContainerOpenEvent(player)) {
return null;
}

if (container instanceof ContainerBridge) {
// This overwrites the normal container behaviour and allows viewing
// inventories that are more than 8 blocks away
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import org.spongepowered.api.event.CauseStackManager;
import org.spongepowered.api.event.Event;
import org.spongepowered.api.event.EventContextKeys;
import org.spongepowered.api.world.server.ServerLocation;
import org.spongepowered.common.applaunch.config.core.SpongeConfigs;
import org.spongepowered.common.event.tracking.context.transaction.TransactionalCaptureSupplier;
import org.spongepowered.common.util.MemoizedSupplier;
Expand Down Expand Up @@ -373,6 +374,10 @@ public boolean isClientSide() {
return false;
}

public Optional<ServerLocation> containerLocation() {
return Optional.empty();
}

protected boolean isRunaway(final PhaseContext<?> phaseContext) {
return phaseContext.getClass() == this.getClass();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,10 @@ public Optional<InteractContainerEvent> generateEvent(
final PhaseContext<@NonNull ?> context, @Nullable final GameTransaction<@NonNull ?> parent,
final ImmutableList<GameTransaction<InteractContainerEvent>> gameTransactions, final Cause currentCause
) {
// TODO: API-14 post events?
if (true) {
return Optional.empty();
}
final ItemStackSnapshot resultingCursor = ItemStackUtil.snapshotOf(this.player.containerMenu.getCarried());
final Transaction<ItemStackSnapshot> cursorTransaction = new Transaction<>(this.cursor, resultingCursor);
final InteractContainerEvent.Close event = SpongeEventFactory.createInteractContainerEventClose(currentCause, (Container) this.menu,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,10 @@ public Optional<InteractContainerEvent> generateEvent(
final PhaseContext<@NonNull ?> context, @Nullable final GameTransaction<@NonNull ?> parent,
final ImmutableList<GameTransaction<InteractContainerEvent>> gameTransactions, final Cause currentCause
) {
// TODO: API-14 post events?
if (true) {
return Optional.empty();
}
final ItemStackSnapshot resultingCursor = ItemStackUtil.snapshotOf(this.player.containerMenu.getCarried());
final Transaction<ItemStackSnapshot> cursorTransaction = new Transaction<>(this.cursor, resultingCursor);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@
import org.spongepowered.common.event.tracking.phase.packet.drag.PrimaryDragInventoryStopState;
import org.spongepowered.common.event.tracking.phase.packet.drag.SecondaryDragInventoryStopState;
import org.spongepowered.common.event.tracking.phase.packet.inventory.BasicInventoryPacketState;
import org.spongepowered.common.event.tracking.phase.packet.inventory.CloseWindowContext;
import org.spongepowered.common.event.tracking.phase.packet.inventory.CloseWindowState;
import org.spongepowered.common.event.tracking.phase.packet.inventory.CreativeInventoryPacketState;
import org.spongepowered.common.event.tracking.phase.packet.inventory.DoubleClickInventoryState;
Expand Down Expand Up @@ -128,7 +127,7 @@ public static final class General {
static final IPhaseState<BasicPacketContext> STOP_SPRINTING = new BasicPacketState();
static final IPhaseState<BasicPacketContext> STOP_SLEEPING = new StopSleepingPacketState();
static final IPhaseState<BasicPacketContext> TAB_COMPLETE = new BasicPacketState();
public static final IPhaseState<CloseWindowContext> CLOSE_WINDOW = new CloseWindowState();
static final CloseWindowState CLOSE_WINDOW = new CloseWindowState();
public static final IPhaseState<BasicPacketContext> UPDATE_SIGN = new BasicPacketState();
static final IPhaseState<BasicPacketContext> STOP_RIDING_JUMP = new BasicPacketState();
static final IPhaseState<BasicPacketContext> HANDLED_EXTERNALLY = new UnknownPacketState();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/*
* This file is part of Sponge, licensed under the MIT License (MIT).
*
* Copyright (c) SpongePowered <https://www.spongepowered.org>
* Copyright (c) contributors
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in
* all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
* THE SOFTWARE.
*/
package org.spongepowered.common.event.tracking.phase.player;

import org.checkerframework.checker.nullness.qual.Nullable;
import org.spongepowered.api.world.server.ServerLocation;
import org.spongepowered.common.event.tracking.IPhaseState;
import org.spongepowered.common.event.tracking.PhaseContext;
import org.spongepowered.common.event.tracking.PhaseTracker;

import java.util.Optional;

public final class PlayerInteractContext extends PhaseContext<PlayerInteractContext> {

private @Nullable ServerLocation containerLocation;

PlayerInteractContext(final IPhaseState<PlayerInteractContext> state, final PhaseTracker tracker) {
super(state, tracker);
}

public PlayerInteractContext containerLocation(final ServerLocation location) {
this.containerLocation = location;
return this;
}

@Override
public Optional<ServerLocation> containerLocation() {
return Optional.ofNullable(this.containerLocation);
}

@Override
protected void reset() {
super.reset();
this.containerLocation = null;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/*
* This file is part of Sponge, licensed under the MIT License (MIT).
*
* Copyright (c) SpongePowered <https://www.spongepowered.org>
* Copyright (c) contributors
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in
* all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
* THE SOFTWARE.
*/
package org.spongepowered.common.event.tracking.phase.player;

import org.spongepowered.common.event.tracking.PhaseTracker;
import org.spongepowered.common.event.tracking.PooledPhaseState;
import org.spongepowered.common.event.tracking.TrackingUtil;

public final class PlayerInteractPhase extends PooledPhaseState<PlayerInteractContext> {

@Override
protected PlayerInteractContext createNewContext(final PhaseTracker tracker) {
return new PlayerInteractContext(this, tracker);
}

@Override
public void unwind(final PlayerInteractContext context) {
TrackingUtil.processBlockCaptures(context);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ public final class PlayerPhase {

public static final class State {

public static final PlayerInteractPhase PLAYER_INTERACT = new PlayerInteractPhase();

public static final IPhaseState<GeneralizedContext> PLAYER_LOGOUT = new PlayerLogoutPhaseState();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,18 +25,13 @@
package org.spongepowered.common.mixin.inventory.api.server.level;

import net.kyori.adventure.text.Component;
import org.checkerframework.checker.nullness.qual.NonNull;
import org.spongepowered.api.entity.living.player.server.ServerPlayer;
import org.spongepowered.api.item.inventory.Container;
import org.spongepowered.api.item.inventory.Inventory;
import org.spongepowered.asm.mixin.Mixin;
import org.spongepowered.asm.mixin.Shadow;
import org.spongepowered.common.bridge.world.inventory.container.ContainerBridge;
import org.spongepowered.common.event.inventory.InventoryEventFactory;
import org.spongepowered.common.event.tracking.PhaseContext;
import org.spongepowered.common.event.tracking.PhaseTracker;
import org.spongepowered.common.event.tracking.TrackingUtil;
import org.spongepowered.common.event.tracking.phase.packet.PacketPhase;
import org.spongepowered.common.mixin.inventory.api.world.entity.player.PlayerMixin_Inventory_API;

import java.util.Optional;
Expand Down Expand Up @@ -74,19 +69,9 @@ public boolean closeInventory() throws IllegalArgumentException {
}
// Create Close_Window to capture item drops
final net.minecraft.server.level.ServerPlayer player = (net.minecraft.server.level.ServerPlayer) (Object) this;
try (final PhaseContext<@NonNull ?> ctx = PacketPhase.General.CLOSE_WINDOW.createPhaseContext(PhaseTracker.SERVER)
.source(this)
.packetPlayer(player)
.isClientSide(false)
) {
ctx.buildAndSwitch();
this.shadow$doCloseContainer();

if (!TrackingUtil.processBlockCaptures(ctx)) {
return false;
}
}
return true;
final net.minecraft.world.inventory.AbstractContainerMenu openMenu = player.containerMenu;
this.shadow$doCloseContainer();
return openMenu != player.containerMenu;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,11 @@
@Mixin(value = Player.class)
public abstract class PlayerMixin_Inventory extends LivingEntityMixin_Inventory {

// @formatter:off
@Final @Shadow public net.minecraft.world.entity.player.Inventory inventory;
@Shadow public AbstractContainerMenu containerMenu;
@Shadow @Final public InventoryMenu inventoryMenu;
// @formatter:on

protected PlayerMixin_Inventory(final EntityType<?> param0, final Level param1) {
super(param0, param1);
Expand Down
Loading

0 comments on commit 491e5c4

Please sign in to comment.