TeamWizardry / Wizardry

Delve into spell creation and become a wizard
Other
64 stars 20 forks source link

crash caused by missing check of infinite loop of crop magic #341

Open Largosaki opened 3 years ago

Largosaki commented 3 years ago

4.3.4 - MC 1.12.2

wizardy:staff use some magic on plant,

some crop (like pam's harvestcraft)

It spreads horizontally and is always only one block high

In the open air will lead to infinite loops cause a crash

public class ModuleEffectThrive implements IModuleEffect { while (world.getBlockState(otherTargetPos.up()).getBlock() == block) { otherTargetPos = otherTargetPos.up(); state = world.getBlockState(otherTargetPos); block = state.getBlock(); }

should we introduce some checks like this? while (otherTargetPos.up().y < 255 || world.getBlockState(otherTargetPos.up()).getBlock() == block) { }

murapix commented 3 years ago

Ah, I see what you're looking at - the otherTargetPos loop code would be run even if only targetPos was looking at the plant. Is this a crash you were actually able to have happen though? We've used Thrive many a time, and not actually ever seen this be a problem

Largosaki commented 3 years ago

Yes, he keeps recurring crashes on my server

I tried to modify it to avoid the crash, although he might execute immediateBlockTick on the air block

currently working well

while (otherTargetPos.getY() < 255 && world.getBlockState(otherTargetPos.up()).getBlock() == block && !world.isAirBlock(otherTargetPos.up()))

<255 is just an additional check but I think it makes sense

otherTargetPos will check the top of the crop and its top for the first time. I guess this can correctly index things like sugar cane. (Because the first cast will make him at least two blocks high) But Pam Farm, which is always only one block high, is still only one block high after the first practice. This will cause repeated inspections of Air == Air to the sky.

taking into account the correct casting he should grow like this

while ( world.getBlockState(otherTargetPos.up()).getBlock() == block) { if (otherTargetPos.getY() > 254 || world.isAirBlock(otherTargetPos)) return true; }

murapix commented 3 years ago

Would be better to fix this by blocking the otherTargetPos section if the targetPos section ran, but this works too. As for actually fixing this, I'll try to get it into the code, but having multiple versions in dev makes things screwy as hell. The 1.12 versions are officially on life support at this point though, so for a while you might just need to not use Thrive on blocks

Largosaki commented 3 years ago

thanks for your reply I don’t know much about him just want the game to run normally there should be better code

PhotonChaos commented 3 years ago

If you can verify that this solution works and causes no other issues, please create a Pull Request!