sokratis12GR / ArmorPlus

ArmorPlus is a mod based on exploration, killing, building, getting geared up, fight the bosses and explore the depths of your worlds.
https://smarturl.it/armorplus?IQid=google
GNU Lesser General Public License v3.0
23 stars 23 forks source link

Enchants above default max level throw errors or crash #223

Closed Xetaxheb closed 5 years ago

Xetaxheb commented 5 years ago

Among other possible ways to do this, one is obtaining them off a miniboss from a mod that gives some enemies enchanted armor that can drop; the error in the gist happened when a skeleton wearing lifesteal 4 armor shot me and it crashed, also throws errors if I wear it myself.

Affected Versions (Do not use "latest")::

Affected Side (Client or Server):

What happens: Enchants throw errors

What you expected to happen:: Enchants not to throw errors

If its a crash, post your crash report (via. pastebin/gist/etc): https://gist.github.com/Xetaxheb/d9a3dc478a9f7eebf8b6cf1dc88932b7 etc

Your configuration files from config/armorplus/ (registry.cfg mainly): Nothing interesting here

sokratis12GR commented 5 years ago

Enchants above default max level aren't allowed. There is a max and min level for enchantments set for a reason. Also, chestplates should NOT be able to get the Life Steal enchant. Its ONLY available for swords and tools

Xetaxheb commented 5 years ago

Oh? What is that reason? And any item in the game can get any enchant, you should put in a check for it to not function at all if it's on an invalid item if you really feel that way.

It looks like poor coding to me, why not just make

public enum Levels { ZERO(0.0f), ONE(0.5f), TWO(1.5f), THREE(2.5f), ;

into healingfactor = enchantmentLevel - 0.5f;

BTW: Levels lvl = Levels.values()[level]; is the reason the error is thrown, you're not checking for "if (level > 3) {level = 3};" pretty easy fix if you really insist on an arbitrary limit. (which neuters the enchant when players are in a setup balanced around greater than 20 hearts)

Xetaxheb commented 5 years ago

Ditto with this

public enum Levels { ZERO(), ONE(23, 0, false, 0, 0), TWO(23, 0, true, 23, 0), THREE(23, 1, true, 46, 0);

23, floor(enchantmentLevel / 3), enchantmentLevel > 1, (enchantmentLevel - 1) * 23, 0)

sokratis12GR commented 5 years ago

Oh? What is that reason? And any item in the game can get any enchant, you should put in a check for it to not function at all if it's on an invalid item if you really feel that way.

Invalid. Any item can have any enchantment ONLY by using custom commands.

I know where this can be fixed. but what is the point ? QoL sure, can be dealt with.

I've intentionally made it limited up to lvl 3, because of how overpowered it can be.

Poor coding

Sure, I mean I like getting criticized but mainly if it is constructive.

It looks like poor coding to me, why not just make

public enum Levels { ZERO(0.0f), ONE(0.5f), TWO(1.5f), THREE(2.5f), ;

into healingfactor = enchantmentLevel - 0.5f;

Because it really just "happens" to fit the values, so IF I decide to expand the level cap later on I can easily do it. Its called future-proofing.

Xetaxheb commented 5 years ago

Invalid. Any item can have any enchantment ONLY by using custom commands. Except plenty of mods circumvent this in various ways. My example was one that adds randomly enchanted armor to miniboss mobs, but there are plenty of others. Let alone that it shouldn't be crashing anyway just because someone commanded it in ??

I know where this can be fixed. but what is the point ? QoL sure, can be dealt with. The point is it causes a crash... I don't really care if it won't function either more powerfully or even at all above your desired cap, but it can't be causing crashes.

Because it really just "happens" to fit the values, so IF I decide to expand the level cap later on I can easily do it. Its called future-proofing.

You can make equations or if statements to fit pretty much any power curve you want while also providing support to situations where the enchant is higher than your expected cap. It's not hard, you're just being obstinate :^)

I don't really mind making a reflection mod that disables your mods enchants outright if it comes to that. I can't have it crashing just because you didn't code it right.

sokratis12GR commented 5 years ago

I'm stubborn, mainly because this issue is caused, by 3rd party mod.

Except plenty of mods circumvent this in various ways.

Yeah well, there is always a blacklist to add enchantments to. IF an enchantment is not supposed to be applied to an item, it should be considered before adding them. Why wouldn't other mods check if it can be applied ;).

The point is it causes a crash... I don't really care if it won't function either more powerfully or even at all above your desired cap, but it can't be causing crashes.

Once again, will be fixed, forgot to mention this code is particularly old, so my old stupid self hasn't future-proofed this.

You can make equations or if statements to fit pretty much any power curve you want while also providing support to situations where the enchant is higher than your expected cap. It's not hard, you're just being obstinate :^)

Yeah, if statements:

I don't really mind making a reflection mod that disables your mods enchants outright if it comes to that. I can't have it crashing just because you didn't code it right.

Go for it :). Code it right.

Xetaxheb commented 5 years ago

Java is a compiled language, extra if statements are virtually no overhead in this situation.

And I just found one that lets me do it already, saves me 10 minutes of work https://minecraft.curseforge.com/projects/enchantments-control No more lifesteal or ferocity.

sokratis12GR commented 5 years ago

Java is a compiled language, extra if statements are virtually no overhead in this situation.

You don't get it don't you ? Its not overheat that is the problem here, its bad code decisions that will be extra if statements when there is always a better way.

sokratis12GR commented 5 years ago

image ;)

sokratis12GR commented 5 years ago

Here you go: ArmorPlus 1.12.2-11.23.0.59