Fix illegal blastmining drops #5068
Conversation
Fixed so now spawners actually dont drop from blastmining, mining wont spawn illegal items anymore, before items like suspisous sand, infested_stone and similar illegal items were requiable with blastmining. Removed double drops from non-mining related blocks Added stone types back into the drops w/o double drops Added Javadoc
Added the new 1.21 potions to alchemy Added all new potion effect types
Fix Herbalism replanting to remove renamed crops from player inventory Previously, players could replant crops infinitely if the replant material was renamed, as it wasn't removed from the inventory.
| // Remove from main inventory | ||
| for (ItemStack itemStack : player.getInventory().getContents()) { | ||
| if (itemStack != null && itemStack.getType() == material) { | ||
| int stackAmount = itemStack.getAmount(); |
| @@ -147,18 +149,29 @@ public static boolean hasItemIncludingOffHand(Player player, Material material) | |||
| * @param amount Amount of the material to remove | |||
| */ | |||
| public static void removeItemIncludingOffHand(@NotNull Player player, @NotNull Material material, int amount) { | |||
| // Checks main inventory / item bar | |||
| if (player.getInventory().contains(material)) { | |||
| player.getInventory().removeItem(new ItemStack(material, amount)); | |||
There was a problem hiding this comment.
Could you please explain why you had to replace lines 150-153? It does not seem obvious why the code you've replaced it with would do anything differently.
| offHandItem.setAmount(newAmount); | ||
| // Remove from off-hand | ||
| ItemStack offHandItem = player.getInventory().getItemInOffHand(); | ||
| if (offHandItem != null && offHandItem.getType() == material) { |
There was a problem hiding this comment.
Was this to avoid a NPE or something else?
| @@ -104,6 +104,8 @@ private static String getMCName(PotionType type) { | |||
| case WEAKNESS : | |||
| case TURTLE_MASTER: | |||
| case SLOW_FALLING: | |||
| case INFESTED: | |||
There was a problem hiding this comment.
This won't be compatible with older versions, you'll have to change the switch case to operate on string value
There was a problem hiding this comment.
Side note: PotionConfigGenerator disgusts me 😆 , I appreciate that you went through the trouble to update it.
|
|
||
| return getSkillLevel() >= BlastMining.getBiggerBombsUnlockLevel() && Permissions.biggerBombs(getPlayer()); | ||
| return RankUtils.hasUnlockedSubskill(getPlayer(), SubSkillType.MINING_BIGGER_BOMBS) | ||
| && getSkillLevel() >= BlastMining.getBiggerBombsUnlockLevel() |
There was a problem hiding this comment.
Skill level check is unnecessary
| @@ -175,66 +210,56 @@ public void remoteDetonation() { | |||
| * @param event The {@link EntityExplodeEvent} | |||
| */ | |||
| public void blastMiningDropProcessing(float yield, EntityExplodeEvent event) { | |||
| if (yield == 0) | |||
| return; | |||
| if (yield == 0) return; | |||
There was a problem hiding this comment.
not a fan of this change, would like it changed back
|
|
||
| var increasedYieldFromBonuses = yield + (yield * getOreBonus()); | ||
| // Strip out only stuff that gives mining XP |
There was a problem hiding this comment.
Deleting comments is an unusual thing to do in a PR, try to keep your diff minimal
| return canUseBlastMining() && player.isSneaking() | ||
| && (isPickaxe(getPlayer().getInventory().getItemInMainHand()) || player.getInventory().getItemInMainHand().getType() == mcMMO.p.getGeneralConfig().getDetonatorItem()) | ||
| && (isPickaxe(player.getInventory().getItemInMainHand()) |
There was a problem hiding this comment.
I appreciate trying to clean up the mess that is the existing code, but try to keep the diff minimal for review
| for (Block targetBlock : event.blockList()) { | ||
| BlockState blockState = targetBlock.getState(); | ||
|
|
||
| if(mcMMO.getUserBlockTracker().isIneligible(targetBlock)) | ||
| continue; | ||
| if (mcMMO.getUserBlockTracker().isIneligible(targetBlock)) continue; |
There was a problem hiding this comment.
Please don't put executions on the same line as ifs, not a fan.
|
PR title seems a bit odd, if you are going to keep it as 3 commits and one PR (which I would prefer you did not), I would like the title of the PR changed to reflect the changes, please add a description of all changes in the PR as well. |
Fixed so now spawners actually dont drop from blastmining, mining wont spawn illegal items anymore, before items like suspisous sand, infested_stone and similar illegal items were requiable with blastmining.
Removed double drops from non-mining related blocks Added stone types back into the drops w/o double drops
Added Javadoc