Funwayguy / RealisticItemDrops

8 stars 4 forks source link

Please don't cancel/deny EntityJoinWorldEvent #8

Open thiakil opened 7 years ago

thiakil commented 7 years ago

Cancelling the event causes world.spawnEntity to return false. Many mods check this and won't delete the source item in mechanical droppers - so this causes an infinite dupe bug.

Funwayguy commented 7 years ago

Wait what? So I should NOT replace the item? which is the whole point of the mod. I'm not really sure what you expect me to do instead.

thiakil commented 7 years ago

So I should NOT replace the item

That's not what I said. Does your event handler only cancel the event? I think not. From your GitHub:

event.getEntity().setDead();
((EntityItem)event.getEntity()).setInfinitePickupDelay();

Those two lines alone should be enough to stop the EntityItem from being picked up, no?

Heck you could even ASM transform spawnEntity or EntityItem itself for all I care.

Funwayguy commented 7 years ago

As much as I'm not okay with the mechanical dropper's necessity to check if the item exists after being dispensed nor comfortable with duplicate items existing within the same tick even if one is dead, I've posted beta builds on Curse so you can test your theory.

NOTE: It only takes one mod to skip an isDead and/or pickupDelay check for a dupe exploit to occur within that tick.

thiakil commented 7 years ago

As much as I'm not okay with the mechanical dropper's necessity to check if the item exists after being dispensed

Could you explain your view on that a little? World.spawnEntity returns false when an Entity cannot be spawned (e.g. Chunk is not loaded, restoringBlockSnapshots is true, an eventhandler denies it).

For example, an Entity's passengers won't be spawned if the spawnEntity fails.

It is reasonable to assume that if it returns false, no item was spawned. Cancelling the event while subsequently spawning something anyway seems rather illogical to me.

NOTE: It only takes one mod to skip an isDead and/or pickupDelay check for a dupe exploit to occur within that tick.

EntityItem.setItem(ItemStack.EMPTY) perhaps? When you cancel the event, this occurs every tick a spawn is attempted

Another way would be to use World.addEventListener and spawn your entity / remove the standard one forcefully.

LemADEC commented 6 years ago

Cancelling the spawn entity event means the spawning action is denied and shouldn't happen in the first place. Other mods may deny such spawn event after your handling of it, for example due to area protection. Consequently, any entity change during the event is prone to cause issues.

I suggest to do entity replacement upon the next tick (i.e. in the main thread, after the entity spawn event). Once setDead() is called no mod should ever assume the entity is still valid. If they do, it's a bug on their side that they need to fix, not you.