AncientWillContainer Interface Compatibility issue

[megadoxs]

Mod Loader

Forge

Minecraft Version

1.20.1

Botania version

latest

Issue description

Hi, while working on a botania addon, I found a few methods inside the TerrasteelHelmItem class that I believe should probably be moved to the AncientWillContainer Interface. While the way it is currently set works for botania, when making an addon, having to copy these methods to a new helmet is redundant and requires to make a mixin only to reimplement what onEntityAttacked already does. Extending TerrasteelHelmItem not being an option since it has a set ArmorMaterial that can't be modified.

The methods that should be moved to AncientWillContainer:

• public static boolean hasAnyWill_
• public static float getCritDamageMult
• public static DamageSource onEntityAttacked
• private static boolean hasAncientWill

hasAnyWill()

hasAnyWill is used once in the TerrasteelHelmetLayer's method named render. Making hasAnyWill be a static method inside AncientWillContainer would still allow TerrasteelHelmetLayer's method named render to work after having changed the method
from:

if (!helm.isEmpty() && helm.getItem() instanceof TerrasteelHelmItem terraHelm) {
	if (TerrasteelHelmItem.hasAnyWill(helm) && !terraHelm.hasPhantomInk(helm)) {
               ...irrelevant code...
	}
}

to:

if (!helm.isEmpty() && helm.getItem() instanceof AncientWillContainer awc) {
	if (awc.hasAnyWill(helm) && !PhantomInkable.hasPhantomInk(helm)) {
                ...irrelevant code...
	}
}

The new hasAnyWill method already takes into account that hasAncientWill_ has been moved to AncientWillContainer and renamed to hasAncientWill (we will talk about that at the end). Because this change would have to be made to hasAnyWill()

from:

public static float getCritDamageMult(Player player) {
	if (hasTerraArmorSet(player)) {
		ItemStack stack = player.getItemBySlot(EquipmentSlot.HEAD);
		if (!stack.isEmpty() && stack.getItem() instanceof TerrasteelHelmItem && hasAncientWill_(stack, AncientWillType.DHAROK)) {
			return 1F + (1F - player.getHealth() / player.getMaxHealth()) * 0.5F;
		}
	}
	return 1.0F;
}

to:

public static float getCritDamageMult(Player player) {
	ItemStack stack = player.getItemBySlot(EquipmentSlot.HEAD);
	if ((stack.getItem() instanceof AncientWillContainer awc && awc.hasArmorSet(player)) {
		if (!stack.isEmpty() && stack.getItem() instanceof TerrasteelHelmItem && hasAncientWill_(stack, AncientWillType.DHAROK)) {
			return 1F + (1F - player.getHealth() / player.getMaxHealth()) * 0.5F;
		}
	}
	return 1.0F;
}

For this change to work, as you can see, you will need to add a new abstract method to AncientWillContainer interface that would be overwritten by the HelmItem class. this method inside of TerrasteelHelmItem could look like the following;

public boolean hasArmorSet(Player player) {
        return hasArmorSet(player);
}

(btw this would also add the possibility for an addon to reuse the same code for two diffrent set of armor. Usefull for addons that could add a male and female variation of an armour set)

Keep this method in mind as it will also be used when I talk about moving getCritDamageMult and onEntityAttacked.

This change would also require hasPhantomInk to be removed from ManasteelArmorItem and BaubleItem and be added as a static method inside PhantomInkable. This third change is not a requisite, but it would allow for TerrasteelHelmetLayer to be extended by any helmet added that can be upgraded with wills without having to override the method. The method would probably still be overwritten in addon to adjust the location of the will texture tho, so yeah the second change is not a necessity. As an alternative instead of out right replacing
helm.getItem() instanceof TerrasteelHelmItem terraHelm
with
helm.getItem() instanceof AncientWillContainer awc,
you could simply have both. Checking if the item has any will from the AncientWillContainer Interface and still loocking if it has phantom ink from the TerrasteelHelmItem class.

getCritDamageMult()

getCritDamageMult is used only once inside ForgeCommonInitializer's registerEvents method. To remove this method from TerrasteelHelmItem would require some changes to the method. here is my suggestion;
from:

public static float getCritDamageMult(Player player) {
       if (hasTerraArmorSet(player)) {
            ItemStack stack = player.getItemBySlot(EquipmentSlot.HEAD);
            if (!stack.isEmpty() && stack.getItem() instanceof TerrasteelHelmItem && hasAncientWill_(stack, AncientWillType.DHAROK)) {
                return 1.0F + (1.0F - player.getHealth() / player.getMaxHealth()) * 0.5F;
            }
        }

        return 1.0F;
    }

to:

static float getCritDamageMult(Player player) {
        ItemStack stack = player.getItemBySlot(EquipmentSlot.HEAD);
        if (stack.getItem() instanceof AncientWillContainer awc && awc.hasArmortSet(player)) {
            if (!stack.isEmpty() && AncientWillContainer.hasAncientWill_(stack, AncientWillType.DHAROK)) {
                return 1.0F + (1.0F - player.getHealth() / player.getMaxHealth()) * 0.5F;
            }
        }

        return 1.0F;
    }

This change works if and only if the abstract hasArmorSet method is added to AncientWillContainer and hasAncientWill is also moved to AncientWillContainer. lastly, since TerrasteelHelmItem.getCritDamageMult is used inside ForgeCommonInitializer's registerEvents method, it would need to have this line
e.setDamageModifier(e.getDamageModifier() * TerrasteelHelmItem.getCritDamageMult(e.getEntity()));
modified to be
e.setDamageModifier(e.getDamageModifier() * AncientWillContainer.getCritDamageMult(e.getEntity()));

onEntityAttacked()

onEntityAttacked is used once inside PlayerMixin's onDamageTarget method. If the following changed are implemented, this line
DamageSource newSource = TerrasteelHelmItem.onEntityAttacked(source, amount, (Player) (Object) this, terraWillCritTarget);
would also need to be changed to
DamageSource newSource = AncientWillContainer.onEntityAttacked(source, amount, (Player) (Object) this, terraWillCritTarget);.
To remove this method from TerrasteelHelmItem would require some changes to the method. here is my suggestion;

from:

public static DamageSource onEntityAttacked(DamageSource source, float amount, Player player, LivingEntity entity) {
        if (hasTerraArmorSet(player)) {
            ItemStack stack = player.getItemBySlot(EquipmentSlot.HEAD);
            if (!stack.isEmpty() && stack.getItem() instanceof TerrasteelHelmItem) {
                if (hasAncientWill_(stack, AncientWillType.AHRIM)) {
                    entity.addEffect(new MobEffectInstance(MobEffects.WEAKNESS, 20, 1));
                }

                if (hasAncientWill_(stack, AncientWillType.GUTHAN)) {
                    player.heal(amount * 0.25F);
                }

                if (hasAncientWill_(stack, AncientWillType.TORAG)) {
                    entity.addEffect(new MobEffectInstance(MobEffects.MOVEMENT_SLOWDOWN, 60, 1));
                }

                if (hasAncientWill_(stack, AncientWillType.VERAC)) {
                    source = Sources.playerAttackArmorPiercing(player.level().registryAccess(), player);
                }

                if (hasAncientWill_(stack, AncientWillType.KARIL)) {
                    entity.addEffect(new MobEffectInstance(MobEffects.WITHER, 60, 1));
                }
            }
        }

        return source;
    }

to:

static DamageSource onEntityAttacked(DamageSource source, float amount, Player player, LivingEntity entity) {
        ItemStack stack = player.getItemBySlot(EquipmentSlot.HEAD);
        if (stack.getItem() instanceof AncientWillContainer awc && awc.hasArmorSet()) {
            if (AncientWillContainer.hasAncientWill(stack, AncientWillType.AHRIM)) {
                entity.addEffect(new MobEffectInstance(MobEffects.WEAKNESS, 20, 1));
            }

            if (AncientWillContainer.hasAncientWill(stack, AncientWillType.GUTHAN)) {
                player.heal(amount * 0.25F);
            }

            if (AncientWillContainer.hasAncientWill(stack, AncientWillType.TORAG)) {
                entity.addEffect(new MobEffectInstance(MobEffects.MOVEMENT_SLOWDOWN, 60, 1));
            }

            if (AncientWillContainer.hasAncientWill(stack, AncientWillType.VERAC)) {
                source = BotaniaDamageTypes.Sources.playerAttackArmorPiercing(player.level().registryAccess(), player);
            }

            if (AncientWillContainer.hasAncientWill(stack, AncientWillType.KARIL)) {
                entity.addEffect(new MobEffectInstance(MobEffects.WITHER, 60, 1));
            }
        }

        return source;
    }

Again, this change works if and only if the abstract hasArmorSet method is added to AncientWillContainer and hasAncientWill is also moved to AncientWillContainer.

hasAncientWill_() (static)

Currently, the static hasAncientWill is used 8 times inside TerrasteelHelmItem across the following methods:

• 1 time in hasAnyWill
• 1 time in getCritDamageMult
• 5 times in onEntityAttacked
• 1 time in hasAncientWill (non-static)

As you can see, in hasAnyWill, getCritDamageMult and onEntityAttacked I assumed that hasAncientWill has already been moved to AncientWillContainer as a static method. The only method in the list that use hasAncientWill_ (static) that hasn't been addressed is hasAncientWill (non-static). And to make it short, this method can be removed. It is only used inside addArmorSetDescription and could already be replaced with its static version. As for moving it to AncientWillContainer, it should be fairly straight foward, I would personally name it hasAncientWill removing the _ as there would no longer be two of them anyway.

The content of the following methods could be also moved to a new static method inside AncientWillContainer to minimise code duplication:

• public void inventoryTick
• public void addArmorSetDescription

inventoryTick()

The current content of inventoryTick is going to be used by every armour item that implements AncientWillContainer, so having a method that you just give all the arguments of inventoryTick would make for a simpler implementation and less code duplication across diffrent armour class. The only thing to keep in mind if this is done, is that when the content of the method is copy-pasted to a new static AncientWilltick method (or some other name) the line
if (player.getInventory().armor.contains(stack) && this.hasArmorSet(player)) {
should be replace with
if (player.getInventory().armor.contains(stack) && hasArmorSet(player)) {,
making sure to use the newly added hasArmorSet of AncientWillContainer.

addArmorSetDescription()

The same thing can be said about addArmorSetDescription, if you add ancientWill to you armour set, you 99% want to have the description that they are added. So again a static method named addAncientWillDescription (or smt) that takes all of addArmorSetDescription parameters as arguments, would be great I believe. Keeping in mind again that if done,
if (hasAncientWill_(stack, type)) {
would need to me changed to
if (hasAncientWill(stack, type)) {,
using the newly added hasAncientWill of AncientWillContainer.

The following method could be removed:

• public boolean hasAncientWill onEntityAttacked,

I think I pretty much already explained why when talking about its current static version, but this method is really not needed currently, and will have absolutely no used once everything related to Ancient Wills is moved to AncientWillContainer.

Notes

First, I would like to talk about onDamageTarget some more, with my suggestion, adding new Ancient Wills be hard, but to be fair, the mod is not really made with the idea of adding more ancient wills. I could not come up with a solution for this issue, but I do feel that this would have to be adressed eventually, from what I understand of the Ancient will System, you currently need two mixins to add your own, one to add it to the AncientWillType enum, and one more to add a new onEntityAttacked method to the Player class. Even with my current proposition, that would not be any simpler to implement.

Secondly, while I am not an active member of the botania community, I have loved that mod for years. As such, if there is a need, I don't mind implementing everything I said earlier and doing a pr. (It shouldn't be that hard for me, since I already thought of the solutions and written them down here).