AndorsTrailRelease / andors-trail

Andor's Trail
89 stars 31 forks source link

Integer division breaks Magic Finder #29

Closed 1da1a172 closed 4 years ago

1da1a172 commented 4 years ago

AndorsTrail/src/com/gpl/rpg/AndorsTrail/controller/SkillController.java

private static int getRollBias(ConstRange chance, Player player, SkillID skill, int perSkillpointIncrease) {
        int skillLevel = player.getSkillLevel(skill);
        if (skillLevel <= 0) return 0;
        return chance.current * skillLevel * perSkillpointIncrease / 100;
}

Here for magic finder, skillLevel is the level of the Magic Finder skill, perSkillpointIncrease is 50, and chance.current is (usually?) 1, from the item chance. Notably, these are all ints, as is the return type. So if we have a skillLevel of 1, we get: 1 * 1 * 50 / 100 = 0 because integer division truncates. Even if these were floats or doubles, it wouldn't matter, though.

AndorsTrail/src/com/gpl/rpg/AndorsTrail/controller/Constants.java

public static boolean rollResult(final ConstRange r, int bias) { return rollResult(r.max, r.current + bias); }
...
private static boolean rollResult(final int probabilityMax, final int probabilityValue) { return rnd.nextInt(probabilityMax) < probabilityValue; }

The random number is always an integer, so even if probabilityValue were 1.5f, that extra 0.5 isn't ever going to help.

Suggestions: 1) Change the description of magic finder to indicate that you are only getting a bonus for every 2 skill points 2) Don't use ints for the calculations (make sure you get them all!) 3) In src/com/gpl/rpg/AndorsTrail/model/ability/SkillCollection.java, change PER_SKILLPOINT_INCREASE_MAGICFINDER_CHANCE_PERCENT to 100 (which ends up being a roundabout way of just setting the bias to the skill level)

Chriz76 commented 4 years ago

Thanks for reporting this. You can also fix it yourself if you want and be part of the community?

1da1a172 commented 4 years ago

I wasn't sure which direction the devs wanted to go with it. As a player, I obviously want option 3, but the devs probably have a better idea of the balance of the game.

Chriz76 commented 4 years ago

Good point. We will check and discuss it

1da1a172 commented 4 years ago

The random number is always an integer, so even if probabilityValue were 1.5f, that extra 0.5 isn't ever going to help.

I just realized that isn't entirely true. the extra 0.5 will always help, since it is a <, not a <=. It just isn't having the effect one might think.

Chriz76 commented 4 years ago

It might be possible to change the chance e.g. from 1.5/100 to 3/200

Rijackson commented 4 years ago

Not option 1 for sure! That will really annoy players that took MF. Number 3 seems the easiest, as long as here are no consequences elsewhere.

Chriz76 commented 4 years ago

I would prefer if you find a way for option 2.

Chriz76 commented 4 years ago

There are only three calls using the bias. So it might be easy to change int getXXXBias(...) to ConstRange getModifiedXXXChance(chance, ...)

Chriz76 commented 4 years ago

Should be fixed and will be part of the next release: https://github.com/NutAndor/andors-trail/commit/6dc33bcac2907b806b1aa0d24569231126910ebb