mekanism / Mekanism

A mod for Minecraft
MIT License
1.37k stars 522 forks source link

Cardboard box duplicates charged certus quartz #7053

Closed archiecarrot123 closed 2 years ago

archiecarrot123 commented 3 years ago

Issue description:

When you place a cardboard box on a chest that contains charged certus quartz (from Applied Energistics 2), it is dropped out of the chest, like the chest was broken. No other items are dropped. The charged certus quartz remains in the chest after the cardboard box is opened, despite having fallen out.

Steps to reproduce:

  1. Install AE2 and Mekanism
  2. Place chest
  3. Place charged certus quartz in the chest
  4. Place a cardboard box on the chest, charged certus quartz will drop
  5. Remove the cardboard box
  6. Open the chest, there will still be charged certus quartz in the chest

Version (make sure you are on the latest version before reporting):

Forge: 36.0.55 Mekanism: 1.16.5-10.0.21.488 Other relevant version: Applied Energistics 2 8.2.0

If a (crash)log is relevant for this issue, link it here: (It's almost always relevant)

attached

https://user-images.githubusercontent.com/16769714/111052429-cf2de700-84ae-11eb-9e07-6462da6ea0af.mp4

video

pupnewfster commented 3 years ago

It was a bit unclear in your post but can you reproduce this with any other items except for charged certus quartz? If you can't produce this with any other item except for charged certus quartz I am inclined to say the bug is on their end.

archiecarrot123 commented 3 years ago

It hasn't happened with any other items, however I haven't tried many others.

BlueAgent commented 3 years ago

Could this be due to https://github.com/MinecraftForge/MinecraftForge/issues/7609 combined with the use of custom item entities? Charged certus being used for in world crafting and having those lightning particles.

pupnewfster commented 3 years ago

No idea as I am not sure we even fire those events really beyond updating the block's state. But if it is only happening with their charged certus quartz and not things like dirt I definitely think it is on AE2's end that they are doing some weird thing to entities containing their items there isn't anything we can do about that. Going to go ahead and close this as based on what archie says it sounds like it isn't happening with mundane items which means it is extremely likely to be something that we can do anything about.

My best guess is their custom item entities aren't actually item entities at all https://github.com/mekanism/Mekanism/blob/1.16.x/src/main/java/mekanism/common/CommonPlayerTracker.java#L108-L114 or it has to do with them removing them themselves then adding them back which there isn't much we can do about

BlueAgent commented 3 years ago

Forge has custom item entities get replaced in the EntityJoinWorldEvent with HIGHEST priority scheduling them to be spawned into the world the following tick.

Because that is occuring first, mekanism doesn't get to prevent the drops with their handler as it was already "prevented" by Forge's custom item entity handler first.

pupnewfster commented 3 years ago

Hmm, I will re-open this for now but I am not really sure there is much we can do about this.

gjerm commented 3 years ago

Getting similar behaviour with Astral Sorcery's Constellation Papers. Please let me know if you would like me to create a separate issue for this. Issue over on their repo: https://github.com/HellFirePvP/AstralSorcery/issues/1770

BlueAgent commented 3 years ago

I noticed that the SetBlockCommand doesn't drop items (when using replace mode) for vanilla tiles.

This is because it calls IClearable#clearObj, which tiles implementing IInventory implement, clearing all contains items (so they aren't dropped when removed).

Although, I think it is recommended for Forge mods to not use IInventory, but use the IItemHandler capability instead, which modded tiles would have to implement IClearable#clear and clear that handler themselves.

I think a fix for this issue would potentially require changes in Forge and/or maybe having more mods more implement IClearable on their tiles?

For now though it might be okay to do the same as SetBlockCommand and use IClearable#clearObj?

This will fix dupes with vanilla and modded blocks that extend from IInventory (e.g. Iron Chests extends from LockableLootTileEntity) and also any tiles that implement IClearable manually (I don't know of any off the top of my head).

This won't fix tiles that don't do do the above, which is most modded tiles. (e.g. Crates from Druidcraft.)

BlueAgent commented 3 years ago

Edit: Ignore Below. This doesn't actually work because doTileDrops doesn't prevent all drops e.g. items inside a chest. It only prevents items directly dropped from blocks broken.

I think a better method would be to:

This feels somewhat hacky though... But it'll work for the majority of cases I think. It won't work for anything that spawns drops in the next tick after being broken for some reason (something might but I don't know). Some gamerules have a change listener that sends a packet, but doTileDrops does not (for now).

Still has some issues though. If anything drops items due to a block update (during the setBlockState), it will delete those drops. So might have to set it without a block update, then trigger a block update afterwards. This actually also an issue with the current Cardboard Box implementation. (Test using a cardboard box on farmland with a crop on top of it.)

Alternatively do the above with World#restoringBlockSnapshots and setting it to true instead. This is probably a very bad idea though... XD This doesn't work. I forgot about https://github.com/MinecraftForge/MinecraftForge/issues/7609.

pupnewfster commented 3 years ago

The solution I probably will look into when I get a chance to look into is discussing with forge about moving their listener from highest to high so that mods can cancel spawns of items with custom item entities by using highest.

Technici4n commented 2 years ago

Hmmm, did this end up being fixed somehow?

pupnewfster commented 2 years ago

It should be fixed? I has been quite a while since I tested though (and I think I tested with AS not AE2) https://github.com/mekanism/Mekanism/commit/ce40a58b8fb7ae1f026a7bc5caedef512fc31d53#diff-8099868176762c57c5cde3346ee425c56c44d4e77777e137d831cbb10304db7aL92-L99

Technici4n commented 2 years ago

I don't think we have AE2 custom item entities behave differently from AS' in that regard. That fix looks appropriate, perfect. :+1: