search

DarkPacks / Crackpack-3

Other
5 stars 1 forks source link

Assassin Bees will attempt to cast any Tile Entity in range to IBeeHousing #113

Open mjevans opened 4 years ago

mjevans commented 4 years ago

Server crash when Assassin Bees (Career Bees) trigger in industrial apiary (was night on install, crashed literally right after I used a sleeping bag to make it day).

I had to reproduce this in single player, and thankfully my little test setup had additional blocks that I wouldn't have thought are related.

The fridge in question is on the other side of a wall, and I was using it to store bees since I thought it was cool to render them when I opened the door. I hadn't realized this bee tried to kill things in any inventory, not just apiaries (things that can produce byproducts). Career Bees should catch that exception and abort processing that block.

[Server thread/ERROR] [minecraft/MinecraftServer]: Encountered an unexpected exception
net.minecraft.util.ReportedException: Ticking block entity
    at net.minecraft.server.MinecraftServer.func_71190_q(MinecraftServer.java:773) ~[MinecraftServer.class:?]
    at net.minecraft.server.MinecraftServer.func_71217_p(MinecraftServer.java:668) ~[MinecraftServer.class:?]
    at net.minecraft.server.integrated.IntegratedServer.func_71217_p(IntegratedServer.java:185) ~[chd.class:?]
    at net.minecraft.server.MinecraftServer.run(MinecraftServer.java:526) [MinecraftServer.class:?]
    at java.lang.Thread.run(Thread.java:748) [?:1.8.0_232]
Caused by: java.lang.ClassCastException: net.blay09.mods.cookingforblockheads.tile.TileFridge cannot be cast to forestry.api.apiculture.IBeeHousing
    at com.rwtema.careerbees.effects.EffectAssassin.performPosEffect(EffectAssassin.java:47) ~[EffectAssassin.class:?]
    at com.rwtema.careerbees.effects.EffectWorldInteraction.performEffect(EffectWorldInteraction.java:46) ~[EffectWorldInteraction.class:?]
    at com.rwtema.careerbees.effects.EffectBaseThrottled.doEffectBase(EffectBaseThrottled.java:84) ~[EffectBaseThrottled.class:?]
    at com.rwtema.careerbees.effects.EffectBase.doEffect(EffectBase.java:167) ~[EffectBase.class:?]
    at forestry.apiculture.genetics.Bee.doEffect(Bee.java:187) ~[Bee.class:?]
    at forestry.apiculture.genetics.Bee.doEffect(Bee.java:168) ~[Bee.class:?]
    at forestry.apiculture.BeekeepingLogic.queenWorkTick(BeekeepingLogic.java:276) ~[BeekeepingLogic.class:?]
    at forestry.apiculture.BeekeepingLogic.doWork(BeekeepingLogic.java:253) ~[BeekeepingLogic.class:?]
    at net.bdew.gendustry.machines.apiary.TileApiary$$anonfun$2$$anonfun$apply$mcV$sp$1.apply$mcV$sp(TileApiary.scala:137) ~[TileApiary$$anonfun$2$$anonfun$apply$mcV$sp$1.class:?]
    at net.bdew.gendustry.machines.apiary.TileApiary$$anonfun$2$$anonfun$apply$mcV$sp$1.apply(TileApiary.scala:129) ~[TileApiary$$anonfun$2$$anonfun$apply$mcV$sp$1.class:?]
    at net.bdew.gendustry.machines.apiary.TileApiary$$anonfun$2$$anonfun$apply$mcV$sp$1.apply(TileApiary.scala:129) ~[TileApiary$$anonfun$2$$anonfun$apply$mcV$sp$1.class:?]
    at scala.util.DynamicVariable.withValue(DynamicVariable.scala:58) ~[scala-library-2.11.1.jar:?]
    at net.bdew.gendustry.misc.DataSlotErrorStates.withSuspendedUpdates(DataSlotErrorStates.scala:80) ~[DataSlotErrorStates.class:?]
    at net.bdew.gendustry.machines.apiary.TileApiary$$anonfun$2.apply$mcV$sp(TileApiary.scala:129) ~[TileApiary$$anonfun$2.class:?]
    at net.bdew.lib.Event0$$anonfun$trigger$1.apply(Event.scala:19) ~[Event0$$anonfun$trigger$1.class:?]
    at net.bdew.lib.Event0$$anonfun$trigger$1.apply(Event.scala:19) ~[Event0$$anonfun$trigger$1.class:?]
    at scala.collection.immutable.List.foreach(List.scala:383) ~[scala-library-2.11.1.jar:?]
    at net.bdew.lib.Event0.trigger(Event.scala:19) ~[Event0.class:?]
    at net.bdew.lib.tile.TileTicking$class.update(TileTicking.scala:24) ~[TileTicking$class.class:?]
    at net.bdew.gendustry.machines.apiary.TileApiary.func_73660_a(TileApiary.scala:44) ~[TileApiary.class:?]
    at net.minecraft.world.World.func_72939_s(World.java:1838) ~[amu.class:?]
    at net.minecraft.world.WorldServer.func_72939_s(WorldServer.java:613) ~[oo.class:?]
    at net.minecraft.server.MinecraftServer.func_71190_q(MinecraftServer.java:767) ~[MinecraftServer.class:?]

Additionally: the server I'm playing on is run by someone using a container, so it's currently in a startup/crash loop. This might have auto-fixed if config/forge.cfg had (about line 59) set to B:removeErroringEntities=true

mjevans commented 4 years ago

There's an older bug from about a month ago https://github.com/rwtema/Careerbees/issues/37 they had the issue of normal minecraft beds causing the crash.

mjevans commented 4 years ago

It looks like (currently) line 34 was already trying to guard against this outcome... https://github.com/mjevans/Careerbees/blob/master/src/main/java/com/rwtema/careerbees/effects/EffectAssassin.java#L34

Looking at this I only have wild guesses about where Java might be going wrong.

https://github.com/ForestryMC/ForestryAPI/blob/master/forestry/api/apiculture/IBeeHousing.java#L19

As someone that rarely touches Java code seeing an issue that looks like it shouldn't exist, it might be better for a developer that is up to date on Java quirks to take a look at this.

sam-kirby commented 4 years ago

On the subject of B:removeErroringEntities - this will never be set true by default. If you need a reason, look no further than the line above where in the comment it states: "BE WARNED THIS COULD SCREW UP EVERYTHING USE SPARINGLY WE ARE NOT RESPONSIBLE FOR DAMAGES."

sam-kirby commented 4 years ago

Have taken a look at the actual issue, canHandleBlock does not appear to be called as part of the bee doing work in a hive. It seems it only gets called inside the onUpdate method of EntityBeeSwarm, which would then call handleBlock which would itself call performPosEffect.

I need to look into this further, but it could be these bees are not intended to do work as part of an apiary and instead only when made into an entity in some manner; maybe gendustry does not properly respect something that would cause this to happen. Or perhaps it's broken and if there's any tile entity nearby to the apiary this will happen.

mjevans commented 4 years ago

I need to look into this further, but it could be these bees are not intended to do work as part of an apiary and instead only when made into an entity in some manner; maybe gendustry does not properly respect something that would cause this to happen. Or perhaps it's broken and if there's any tile entity nearby to the apiary this will happen.

https://www.curseforge.com/minecraft/mc-mods/career-bees

"Assassin: Instantly kills active queens in nearby hives. Place in a bee gun for full regicide experience."

This is supposed to work only on 'hives', and should have an effect similar to frames that instant-end the lifecycle.

sam-kirby commented 4 years ago

Ah, sounds like the bee gun is what would transform it into a swarm entity then.

But it clearly does sound like they should work inside an apiary too, yes.

mjevans commented 4 years ago

In creative testing use of the gun on a hive has the desired effect, and lack of desired effect vs other entities.

The bee gun's onItemRightClick does use a 'swarm' targeted along a vector https://github.com/rwtema/Careerbees/blob/master/src/main/java/com/rwtema/careerbees/items/ItemBeeGun.java (also I just noticed that last night I accidentally copied from the fork I was looking at in case there was an obviously fixable problem).

sam-kirby commented 4 years ago

It just needs to check canHandleBlock in performPosEffect and respond accordingly, but the project seems abandoned. Unfortunately it's licensed as all rights reserved so there's not much we can do.

mjevans commented 4 years ago

Reading your comment, I see that I got lost last night while looking at the code before bed, though I slightly disagree since it looks like those two functions serve slightly different use cases.

It seems that canHandleBlock is something added by CareerBees here https://github.com/rwtema/Careerbees/blob/master/src/main/java/com/rwtema/careerbees/effects/ISpecialBeeEffect.java

I'm making a leap of faith guess that the canHandleBlock check was added for the gun and that your suggestion is correct.

There's also another, even older, Career Bees bug: https://github.com/rwtema/Careerbees/issues/23

I think the above is a clear enough goal that I will work this later tonight, a clean build of the published code was successful with just chmod +x gradlew && gradlew build ; so I should be able to actually test it.

sam-kirby commented 4 years ago

My comment was intended as guidance if anyone here wanted to put together an ASM patch mod, which is the only way I see this being resolved without the mod receiving attention from its author.

canHandleBlock performs all the necessary checks already, it exists, and it would be fairly easy to add that check using ASM. It is already used as a gate on calling performPosEffect in other areas of the codebase.

sam-kirby commented 4 years ago 
diff --git a/src/main/java/com/rwtema/careerbees/effects/EffectAssassin.java b/src/main/java/com/rwtema/careerbees/effects/EffectAssassin.java
index a778a9e..3817ef5 100644
--- a/src/main/java/com/rwtema/careerbees/effects/EffectAssassin.java
+++ b/src/main/java/com/rwtema/careerbees/effects/EffectAssassin.java
@@ -44,6 +44,7 @@ public class EffectAssassin extends EffectWorldInteraction {

    @Override
    protected boolean performPosEffect(World world, BlockPos blockPos, IBlockState state, IBeeGenome genome, IBeeHousing housing) {
+       if (!canHandleBlock(world, blockPos, genome, null)) return false;
        IBeeHousing house = (IBeeHousing) world.getTileEntity(blockPos);
        if(house == null) return false;
        ItemStack queenStack = house.getBeeInventory().getQueen();

Does appear to fix this. Creates unnecessary double check when used with the bee gun, but that's probably rather underused, and fixing the crash is more important.

Alternatively, the following diff may be better as it will apply this change for all world interaction bees whilst not introducing the double check. I have confirmed it works for assassin bees but have not tested others. There is not much point doing further work on this at this time however.

diff --git a/src/main/java/com/rwtema/careerbees/effects/EffectWorldInteraction.java b/src/main/java/com/rwtema/careerbees/effects/EffectWorldInteraction.java
index edcb218..3e31190 100644
--- a/src/main/java/com/rwtema/careerbees/effects/EffectWorldInteraction.java
+++ b/src/main/java/com/rwtema/careerbees/effects/EffectWorldInteraction.java
@@ -43,7 +43,7 @@ public abstract class EffectWorldInteraction extends EffectBaseThrottled impleme

                BlockPos blockPos = new BlockPos(x, y, z);
                IBlockState state = world.getBlockState(blockPos);
-               if (performPosEffect(world, blockPos, state, genome, housing))
+               if (canHandleBlock(world, blockPos, genome, null) && performPosEffect(world, blockPos, state, genome, housing))
                        break mainLoop;
            }
    }
mjevans commented 4 years ago

I forgot to work on a local branch, but I've tested this locally and it seems to fix all the issues.

Since I don't normally write Java, I would like at least a second pair of eyes on the diff before I make a pull request.

sam-kirby commented 4 years ago

Didn't get time for this until now, but see you've already made a pull. Still, here's some small changes.

mjevans commented 4 years ago

I agree it adds clutter, but it might have been nice to add in a debug mode that conditionally enabled it, so I had wanted to leave it as comments.

You mean if ( object ) vs if ( object == null ) (or what I probably should have used, if ( null == object ) ? The null is present to more clearly document that this is a test for null as a means of improving the readability and maintainability of the code.

TEVAlidatedSides exists because Java doesn't seem to have a multiple return type and this is the closest thing to a struct. Something like it still makes sense as a means of de-duplicating the code between the check for can work and the check for the target being a valid entity.

Did you mean line 51 in that file? I think the link you wanted to paste didn't get copied, since this is a dup of the second point but is phrased as if talking about another item. Do you mean the assassin bee hive location check? I can see how that would clarify things now that a time has passed and I'm reading things fresh.

I had left that in from my local testing (to make sure I could tell my build and the mod-pack shipped version apart). I can remove since you've said that's the convention.

+

Additional Notes for myself: Add an early escape for TE-block-check, reconsider simplifying it as a return of valid-faces (test for list length), and a 'return on first valid face' flag.

Before I make a new revision did you have any suggestions on how to accomplish reusing the side check between the 'can work' check and the 'do work, pre-validate' check?

sam-kirby commented 4 years ago

You mean if ( object ) vs if ( object == null ) (or what I probably should have used, if ( null == object ) ? The null is present to more clearly document that this is a test for null as a means of improving the readability and maintainability of the code.

No, I mean the null check is effectively taken care of by if (tileEntity instanceof IBeeHousing. In the case tileEntity == null, then tileEntity instanceof IBeeHousing is false.

Did you mean line 51 in that file? I think the link you wanted to paste didn't get copied, since this is a dup of the second point but is phrased as if talking about another item. Do you mean the assassin bee hive location check? I can see how that would clarify things now that a time has passed and I'm reading things fresh.

Yes, I clearly messed up that link.

Before I make a new revision did you have any suggestions on how to accomplish reusing the side check between the 'can work' check and the 'do work, pre-validate' check?

Do you mean avoiding calling canHandleBlock twice when using a Bee Gun? If so then doing the canHandleBlock check in EffectWorldInteraction::performEffect would be better as in the second diff I posted above.

If you mean having to check valid faces in EffectPower::canHandleBlock and EffectPower::doEffectBase, I can't see an obvious way to avoid doing that.

mjevans commented 4 years ago

It isn't clear what the intent of the canHandleBlock method is supposed to be; if there is documentation for the API it might be in other parts of the code that I haven't read. I can see potential use cases for calling it over the effect range (in which case having a specific failure code 'will never touch' may be useful in selecting a different target, than selecting a target and getting back 'no update required'). Returning failure or an error code range isn't well documenting; using an ERRNO style return or an enum would also require a clear API decision and I'm not sure what's driving the current set of choices.

All of the work that canHandleBlock does still needs to be done in the handling function, as it's guard, but it's useful to keep the results of that work since they were just done; particularly when that work gets to the point of checking if other blocks are valid.

I'm not sure how busy tomorrow is, but I do know that I want to have more focus and wakefulness than I do now; I will try to update my PR soon.

mjevans commented 4 years ago

Sam, could you take a glance at the current state of my modifications?

Edit: it seems Github is willing to automatically do it for the PR

https://github.com/rwtema/Careerbees/pull/39/files

git diff fe5384aed416cc4fe66531ed9674fa368348de56 5bd511f50207fe9c1239fce397e52bd7caaeb894

mjevans commented 4 years ago

I think someone might have to Tweet it at https://twitter.com/RWTema (that account seems to still be active).

That someone will not be me, as I don't use Twitter, Facebook, Discord, etc...

mjevans commented 4 years ago

The clean patch I made has been submitted as an easier to review pull request.

Someone ELSE who actually uses Twitter might want to Tweet it at https://twitter.com/RWTema (that account seems to still be active). I don't use Twitter, Facebook, Discord, etc...