ForestryMC / MagicBees

Repository for the MagicBees Minecraft mod - a Forestry addon.
Do What The F*ck You Want To Public License
20 stars 18 forks source link

[1.7] NodeHelper.growNodeInRange looks wrong #32

Closed rmunn closed 7 years ago

rmunn commented 7 years ago

At the core of the NodeHelper.growNodeInRange function in the 1.7 branch is the following loop and test:

int rollAttempts = 0;
do {
    aspectToAdd = getWeightedRandomAspect(world.rand);
    ++rollAttempts;
}
while (aspectsBase.getAmount(aspectToAdd) < 255 && 20 < rollAttempts);

if (20 <= rollAttempts) {
    return false;
}

(The code after this section adds a few points to that aspect and updates the node).

The comparisons in that while line look incorrect to me. The intent of this code seems to be to keep picking a random aspect until you pick one that's under 255 -- and if you make 20 attempts and still get nothing but aspects that are 255 or greater, then give up. But in fact, this is not what the code will actually do: the while loop will never actually loop! On the first run through the loop, rollAttempts will be 1, so the test 20 < rollAttempts will test whether 20 is less than 1, which is, of course, false. So the && operator will return false, and the loop will end after the first iteration no matter what. Then the if statement will check whether 20 is less then or equal to 1, which is also false, and so the first aspect chosen will always be grown, even if it's already more than 255!

To better follow what I believe to be the intent of this code, I believe that that test in the while loop should be changed to:

while (aspectsBase.getAmount(aspectToAdd) > 255 && rollAttempts < 20);

Then what the loop will do is: "Did we get an aspect that's 255 or less? Okay, we've picked an aspect we can add to, so we end the loop. Did we get an aspect that's 256 or more? Then try again, unless we've already made 20 attempts, in which case give up so we don't loop infinitely."

This bug only affects the 1.7 branch since it's specific to the Thaumcraft integration. The 1.10/1.11 branch does not include Thaumcraft integration (because Thaumcraft doesn't exist for those Minecraft versions), so it's not affected by this bug.