Fixed Super Sponge unable to absorb shimmer

[sgkoishi]

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

(holding sponge, not picksaw)

[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))

[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. https://github.com/Pryaxis/TShock/blob/536c4d25459a61971af07d03453e6d46ce7f4b41/TShockAPI/Bouncer.cs#L1815-L1817

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) https://github.com/Pryaxis/TShock/blob/536c4d25459a61971af07d03453e6d46ce7f4b41/TShockAPI/Bouncer.cs#L1738-L1771