Skip to content

Fix siege cubes remaining in player inventory on death via keepinventory#1913

Open
Mansarde wants to merge 1 commit intomainfrom
fix/siege-cube-keepinventory-death-trick
Open

Fix siege cubes remaining in player inventory on death via keepinventory#1913
Mansarde wants to merge 1 commit intomainfrom
fix/siege-cube-keepinventory-death-trick

Conversation

@Mansarde
Copy link
Member

@Mansarde Mansarde commented Sep 26, 2025 •
edited
Loading

About the PR

Prevents siege cubes from staying in a player's inventory on death when the gamerule keepinventory is true.

Originally reported by amokdev on Discord here: https://discord.com/channels/1213989169878274068/1421125485362020525/1421134182800818206

Why / Balance

So that a player, who set their spawn point inside a TARDIS, cannot take the siege cube inside of that TARDIS by dying with the siege cube in their inventory (while keepinventory is true).

Technical details

I hooked into the onDeath() method of the ServerPlayer and then check for siege cube items.
If one is found, I remove it and place the TARDIS exterior at the position of the player.

That code is basically just copied over from how it is done when a player is disconnecting from the server with a siege cube in their inventory (see ServerPlayConnectionEvents.DISCONNECT event of the SiegeHandler)

Requirements

Changelog
🆑

  • fix: Siege cubes remained in a player's inventory on death when the gamerule "keepinventory" was true

@Mansarde
fix siege cube remaining on death via keepinventory
ec22571 
This will prevent a siege cube from remaining in the player inventory 
on death when the gamerule keepinventory is true.
This is to prevent a player from taking a siege cube into its TARDIS by 
setting their spawn point inside that TARDIS and dying with the siege cube 
in their inventory while outside of the TARDIS.
@Mansarde Mansarde requested a review from a team as a code owner September 26, 2025 15:05
@github-actions github-actions bot added S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. S: Needs Review Status: Requires additional reviews before being fully accepted. size/S Denotes a PR that changes 10-99 lines. labels Sep 26, 2025
@Mansarde Mansarde added T: Bugfix Type: Bugs and/or bugfixes. P3: Standard Priority: Default priority for repository items. A: General Interactions Area: General in-game interactions that don't relate to another area. and removed S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. labels Sep 26, 2025
drtheodor
drtheodor requested changes Sep 26, 2025
View reviewed changes
src/main/java/dev/amble/ait/mixin/server/ServerPlayerMixin.java
public void ait$onDeath(DamageSource damageSource, CallbackInfo ci) {
ServerPlayerEntity player = (ServerPlayerEntity) (Object) this;

ServerTardisManager.getInstance().forEach(tardis -> {
Copy link
Member

@drtheodor drtheodor Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ow. this is real bad.

Copy link
Member Author

@Mansarde Mansarde Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a copy of your code. 😆
See here: https://github.com/amblelabs/ait/blame/bb56218359c19bebf28dd57c59a954fb00227022/src/main/java/dev/amble/ait/core/tardis/handler/SiegeHandler.java#L64

Copy link
Member

@drtheodor drtheodor Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not.

Copy link
Member

@drtheodor drtheodor Sep 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was a part of my refactor, however, it wasn't my code. It was taken from here: cd7fa6b#diff-6d014cd6ae1f468da6e5be563db99c534c44490e608d092fbb092162b9269a6cL341-L351 and moved to the SiegeHandler so git blame thinks that it was me who put it there.

Copy link
Member

@drtheodor drtheodor Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

29456f4 that's the commit that introduced it

Copy link
Member Author

@Mansarde Mansarde Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you did the same thing as I did then. Copy it without changing it. :P

But jokes aside, I guess the point is moot anyways, now that this is not going to be used at all.

src/main/java/dev/amble/ait/mixin/server/ServerPlayerMixin.java
if (!Objects.equals(tardis.siege().getHeldPlayerUUID(), player.getUuid()))
return;

for (ItemStack itemStack : player.getInventory().main) {
Copy link
Member

@drtheodor drtheodor Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thats horrible

@github-actions github-actions bot added S: Awaiting Changes Status: Changes are required before another review can happen. and removed S: Needs Review Status: Requires additional reviews before being fully accepted. labels Sep 26, 2025
@drtheodor
Copy link
Member

drtheodor commented Sep 26, 2025

just prevent dropping siege cubes inside the tardis

@Mansarde
Copy link
Member Author

Mansarde commented Sep 26, 2025

just prevent dropping siege cubes inside the tardis

I thought about this first, but I assumed that if someone's game crashes while they have a cube in their inventory then there would be no exterior in the world anymore (not even in intem form).

But I just tested in ingame and it seems the disconnect event ( with the real bad code :P ) is actually preventing that from happening as well.

So I'll just prevent the dropping of the item in the same TARDIS then as you suggested. 🫡

@Mansarde
Copy link
Member Author

Mansarde commented Sep 26, 2025

Hmm, this problem is a bit more extensive than I initially thought.
For example, I have to also consider what happens if a player dies inside the TARDIS they have a siege cube of in their inventory.
Or that a player could put the item inside a container that is inside the sieged TARDIS.
Or the various ways in which an item can be dropped from the inventory.
Etc.

Will have to analyze this a bit more to cover all the bases.

@drtheodor
Copy link
Member

drtheodor commented Sep 27, 2025

https://discord.com/channels/1213989169878274068/1213989170964340883/1421197555156389981

@github-actions github-actions bot added the S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted. label Sep 27, 2025
@github-actions
Copy link

github-actions bot commented Sep 27, 2025

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted. label Oct 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@drtheodor drtheodor drtheodor requested changes

@Loqor Loqor Awaiting requested review from Loqor

@MaketendoDev MaketendoDev Awaiting requested review from MaketendoDev

@duzos duzos Awaiting requested review from duzos

@rapbattlegod32 rapbattlegod32 Awaiting requested review from rapbattlegod32

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

A: General Interactions Area: General in-game interactions that don't relate to another area. P3: Standard Priority: Default priority for repository items. S: Awaiting Changes Status: Changes are required before another review can happen. size/S Denotes a PR that changes 10-99 lines. T: Bugfix Type: Bugs and/or bugfixes.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Mansarde @drtheodor