Fixed Super Sponge unable to absorb shimmer

[sgkoishi]

Fixes super sponge unable to handle shimmer, reported by Asanay.

(holding sponge, not picksaw)

[sgkoishi]

I'm not sure if the banned item check should be here, I keep it because it used to be here and the IsBeingDisabled check does not check item-ban disabled

TShock/TShockAPI/Bouncer.cs

Line 1656 in 5d84192

if (args.Player.IsBeingDisabled())

[sgkoishi]

There is also a behaviour change here:
Previously ban lava (or other liquid) bucket => ban lava bucket, bottomless lava bucket, lava sponge, ultra sponge (if absorbing lava)
Now ban lava bucket just means lava bucket.

[hakusaro]

Now ban lava bucket just means lava bucket.

Why?

This PR is genuinely daunting to review because the implication is that the intent is to "Fixed Super Sponge unable to absorb shimmer" but it also "Now ban lava bucket just means lava bucket." and in-so-doing reorders all of the code related to this and makes it hard to review. At minimum can you separate changes like this into multiple commits?

For example:

Commit A (contents) could be the diff just for absorbing shimmer
Commit B could change the behavior for the lava bucket
Commit C could reorder everything in a different way

I've looked at this PR and other PRs in TShock multiple times but it's not easy to parse or understand the change deltas with very small descriptions of changes.

[sgkoishi]

Now this adds back to the original behavior, banning the lava bucket will disable all lava-related actions, and so do water/honey/shimmer (via bottomless shimmer bucket).

[sgkoishi]

The commit 49921cb seems to be confusing - view it with "patience diff" algorithm (git diff --patience ac7ee7e..49921cb) would be more clear:

diff --git a/TShockAPI/Bouncer.cs b/TShockAPI/Bouncer.cs
index cfa10b50..4b1ff297 100644
--- a/TShockAPI/Bouncer.cs
+++ b/TShockAPI/Bouncer.cs
@@ -1729,54 +1729,70 @@ namespace TShockAPI
 					args.Handled = true;
 				}
 
+				if (TShock.ItemBans.DataModel.ItemIsBanned(EnglishLanguage.GetItemNameById(selectedItemType), args.Player))
+				{
+					Reject(GetString("Using banned {0} to manipulate liquid", Lang.GetItemNameValue(selectedItemType)));
+					return;
+				}
+
+				switch (type)
+				{
+					case LiquidType.Water:
+						if (TShock.ItemBans.DataModel.ItemIsBanned(EnglishLanguage.GetItemNameById(ItemID.WaterBucket), args.Player))
+						{
+							Reject(GetString("Using banned water bucket without permissions"));
+							return;
+						}
+						break;
+					case LiquidType.Lava:
+						if (TShock.ItemBans.DataModel.ItemIsBanned(EnglishLanguage.GetItemNameById(ItemID.LavaBucket), args.Player))
+						{
+							Reject(GetString("Using banned lava bucket without permissions"));
+							return;
+						}
+						break;
+					case LiquidType.Honey:
+						if (TShock.ItemBans.DataModel.ItemIsBanned(EnglishLanguage.GetItemNameById(ItemID.HoneyBucket), args.Player))
+						{
+							Reject(GetString("Using banned honey bucket without permissions"));
+							return;
+						}
+						break;
+					case LiquidType.Shimmer:
+						if (TShock.ItemBans.DataModel.ItemIsBanned(EnglishLanguage.GetItemNameById(ItemID.BottomlessShimmerBucket), args.Player))
+						{
+							Reject(GetString("Using banned shimmering water bucket without permissions"));
+							return;
+						}
+						break;
+					default:
+						Reject(GetString("Manipulating unknown liquid type"));
+						return;
+				}
+
 				if (type == LiquidType.Lava && !(selectedItemType == ItemID.LavaBucket || selectedItemType == ItemID.EmptyBucket || selectedItemType == ItemID.LavaAbsorbantSponge || selectedItemType == ItemID.BottomlessLavaBucket || selectedItemType == ItemID.UltraAbsorbantSponge))
 				{
 					Reject(GetString("Spreading lava without holding a lava bucket"));
 					return;
 				}
 
-				if (type == LiquidType.Lava && TShock.ItemBans.DataModel.ItemIsBanned("Lava Bucket", args.Player))
-				{
-					Reject(GetString("Using banned lava bucket without permissions"));
-					return;
-				}
-
 				if (type == LiquidType.Water && !(selectedItemType == ItemID.WaterBucket || selectedItemType == ItemID.EmptyBucket || selectedItemType == ItemID.BottomlessBucket || selectedItemType == ItemID.UltraAbsorbantSponge || selectedItemType == ItemID.SuperAbsorbantSponge))
 				{
 					Reject(GetString("Spreading water without holding a water bucket"));
 					return;
 				}
 
-				if (type == LiquidType.Water && TShock.ItemBans.DataModel.ItemIsBanned("Water Bucket", args.Player))
-				{
-					Reject(GetString("Using banned water bucket without permissions"));
-					return;
-				}
-
 				if (type == LiquidType.Honey && !(selectedItemType == ItemID.HoneyBucket || selectedItemType == ItemID.EmptyBucket || selectedItemType == ItemID.BottomlessHoneyBucket || selectedItemType == ItemID.HoneyAbsorbantSponge || selectedItemType == ItemID.UltraAbsorbantSponge))
 				{
 					Reject(GetString("Spreading honey without holding a honey bucket"));
 					return;
 				}
 
-				if (type == LiquidType.Honey && TShock.ItemBans.DataModel.ItemIsBanned("Honey Bucket", args.Player))
-				{
-					Reject(GetString("Using banned honey bucket without permissions"));
-					return;
-				}
-
 				if (type == LiquidType.Shimmer && !(selectedItemType == ItemID.BottomlessShimmerBucket || selectedItemType == ItemID.UltraAbsorbantSponge || selectedItemType == ItemID.SuperAbsorbantSponge))
 				{
 					Reject(GetString("Spreading shimmer without holding a shimmer bucket"));
 					return;
 				}
-
-				if (type == LiquidType.Shimmer &&
-					TShock.ItemBans.DataModel.ItemIsBanned("Bottomless Shimmer Bucket", args.Player))
-				{
-					Reject(GetString("Using banned bottomless shimmer bucket without permissions"));
-					return;
-				}
 			}
 
 			if (!args.Player.HasBuildPermission(tileX, tileY))

[drunderscore]

I'm not sure if this is what @hakusaro was exactly looking for when mentioning "separate commits", but I find these commits to be super weird:

• The first commit changes the magic numbers to include allow the Super Sponge
• ...but then the second commit removes those magic numbers completely
• ...but then the sixth commit totally removes all of that and replaces it with a switch statement on the items?

I do not recall how this PR previously looked, and that's certainly my own fault, but it's hard to imagine this is better?

[drunderscore]

I find this entire commit to be weird... why group the check of liquids based on item instead? It just feels so backwards when looking at the code.

[drunderscore]

And what's the point of this commit, if not to make all of this code a switch case later?

[sgkoishi]

The first commit resolves the issue, SuperAbsorbantSponge and BottomlessBucket behave differently and we need to split them.

However, it turns out that the bucket number is meaningless because basically, each item maps to a bucket, e.g. bucket == 0 is abbr of selectedItemType == ItemID.EmptyBucket. With ItemID it is more readable to have ItemID.EmptyBucket than 0. This is the second commit.

After the second commit, the lines are long and there are still many duplications, e.g. we have four EmptyBucket and four UltraAbsorbantSponge checks, group them by item type helps readability: if they share common behaviour, they use the same code instead of duplicating selectedItemType == ItemID.EmptyBucket || selectedItemType == ItemID.UltraAbsorbantSponge or bucket == X four times.

TShock/TShockAPI/Bouncer.cs

Lines 1815 to 1817 in 536c4d2

	case ItemID.EmptyBucket:
	case ItemID.UltraAbsorbantSponge:
	break;

The original commit 3e61e68 is exactly the same but in one commit and without this, (restrict all lava actions if lava bucket banned, etc. to keep the same behaviour as old implementation)

TShock/TShockAPI/Bouncer.cs

Lines 1738 to 1771 in 536c4d2

	switch (type)
	{
	case LiquidType.Water:
	if (TShock.ItemBans.DataModel.ItemIsBanned(EnglishLanguage.GetItemNameById(ItemID.WaterBucket), args.Player))
	{
	Reject(GetString("Using banned water bucket without permissions"));
	return;
	}
	break;
	case LiquidType.Lava:
	if (TShock.ItemBans.DataModel.ItemIsBanned(EnglishLanguage.GetItemNameById(ItemID.LavaBucket), args.Player))
	{
	Reject(GetString("Using banned lava bucket without permissions"));
	return;
	}
	break;
	case LiquidType.Honey:
	if (TShock.ItemBans.DataModel.ItemIsBanned(EnglishLanguage.GetItemNameById(ItemID.HoneyBucket), args.Player))
	{
	Reject(GetString("Using banned honey bucket without permissions"));
	return;
	}
	break;
	case LiquidType.Shimmer:
	if (TShock.ItemBans.DataModel.ItemIsBanned(EnglishLanguage.GetItemNameById(ItemID.BottomlessShimmerBucket), args.Player))
	{
	Reject(GetString("Using banned shimmering water bucket without permissions"));
	return;
	}
	break;
	default:
	Reject(GetString("Manipulating unknown liquid type"));
	return;
	}

[drunderscore]

I've tested that these changes fix the titled issue.

In the end, the commits will be squashed and merged into general-devel, so I'm alright with them staying how they are, as I was able to review the final code alright.

The switching on the selected item to check against the liquid type actually seems fine. I was a bit over-the-top with my previous dismissal of it, but the code is clean with it.

[drunderscore]

It is a bit weird the basic bucket type being banned means the liquid is entirely banned, but that is what the original code did, so that's alright I think.