Need better hooks for other mods to implement compatibility with VS to address bad behavior when some block entities are relocated

dwayn commented 8 months ago

Mod Name

Sophisticated Storage

This issue occurs when only Valkyrien Skies, addons, and the mod I have specified are installed and no other mods

[X] I have tested this issue and it occurs with only Valkyrien Skies, addons, and the mod I have specified

Minecraft Version

1.19

Mod Loader

Forge

Issue description

I opened a ticket with Eureka (https://github.com/ValkyrienSkies/Eureka/issues/295) initially because I thought the duplication bug I came across with Sophisticated Storage block entities on VS Eureka ships was an issue with Eureka. Upon digging deep into it, I realized that this is a fundamental bug in VS2.

I ended up writing a thirdparty compatibility mod for Sophisticated Storage that implements Clearable for sophisticated's inventory blocks and sets the block's packed state when clearContent is called specifically from Valkyrien Skies only so that it does not drop its contents. That mod is now published on curseforge: https://www.curseforge.com/minecraft/mc-mods/valkyrienskiessophisticatedstoragecompat and the source is here if you want to take a look: https://github.com/dwayn/ValkyrienSkiesSophisticatedStorageCompat

This post is not to advertise the mod I made, I just provided that as an example of one compatibility patch I have had to make so far, but to see if we could get something in VS2 that would make adding compatibility for other mods a lot easier going forward.

After looking through things and working on this compatibility fix (it is kind of hacky how it does it, but it fixes the duplication behavior in a reasonable way for now), I have some thoughts on some things that could be done in VS to make building compatibility a little easier for other mods, and would love to get a chance to chat with one of the devs that is familiar with how things work around ship assembly/disassembly and block relocation.

I am not quite sure what the best approach would be for integrating hooks in the block relocation flow, but short version, I am thinking having one or more interfaces defined for different phases of the assembly/disassembly process that other mods could implement on their blocks/blockentities so that their blocks are properly prepped for a relocation and reconfigured if necessary upon relocation. Seems like at minimum, the interfaces needed would be:

pack: this is where the block entity could save any extra state it needs before the block is cloned to be moved
destroy or destroy prep: this would implement anything custom that needs to be implemented to destroy the old block without bad side effects like dropping items from an inventory causing item duplication
unpack (including info on the position translation that was done): block entity could use the data it packed previously to reconfigure itself at the new location

And potentially could use these if they could reasonably be added

prep assembly: this would be a place to allow block entities that have dependent interactions with other blocks/coords to note things like the other block's position relative to themselves
complete assembly: this would be a place to allow block entities to reconnect their dependent interactions with other blocks in the new location

I am not sure how feasible the prep and complete assembly steps would be as it would depend on walking all the blocks to identify what is included in the ship and calling prep assembly on them if implemented, walking all the blocks a second time to actually relocate them (calling the pack/destroy/unpack on them as relocated), and then walking all the blocks a third time to call complete assembly on any that implement it.

Issue reproduction

Build any ship, add a sophisticated storage chest or barrel inventory to the ship, put items in the inventory. Every time you assemble or disassemble the ship, all the items in the sophisticated inventory are spit out on the ground and still exist in the block entity that is part of the ship (this is due to the items being dropped from the sophisticated block that is destroyed during the relocate block step).

Logs

No logs necessary on this one as there are no error logs from it, just unwanted item duplication behavior.

dwayn commented 8 months ago

For context, here are some other bug reports of item duplication issues with other storage mods as well, so this is not just a sophisticated storage issue: https://github.com/ValkyrienSkies/Eureka/issues/270 https://github.com/ValkyrienSkies/Eureka/issues/251

I am interested in helping get these other mods working with VS (because I really want to see VS become a staple in modpacks), but this could be made a lot easier with some better ability for other mods to hook into the block relocation process to allow them to prep for relocation and reconfigure themselves once relocated.

hlysine commented 1 month ago

Hi, Create: Copycats+ dev here. I got a bug report from a user finding a similar item duplication bug when Create: Copycats+ is used with VS2, and I decided to investigate further. Instead of creating new interfaces for other mods to implement compatibility with VS2, VS2 should clean up properly after relocating blocks, similar to how Create assembles contraptions so that extra items are not dropped.

This is Create's relocation flow: https://github.com/Creators-of-Create/Create/blob/fa4f5243bcf8c8b312a9d97fe4ac0408798ef964/src/main/java/com/simibubi/create/content/contraptions/Contraption.java#L1014-L1022 This is where VS2's code is problematic: https://github.com/ValkyrienSkies/Valkyrien-Skies-2/blob/eac19c84e2fe89f73e277e1383f3c164c7433829/common/src/main/kotlin/org/valkyrienskies/mod/util/RelocationUtil.kt#L32-L63

Basically, there are two things that VS2 should do:

Right after block entity NBTs are serialized, VS2 should remove the block entity.
When setting a position to air, VS2 should include the UPDATE_MOVE_BY_PISTON and UPDATE_SUPPRESS_DROPS flags to signal to other mods that they should not drop their contents. Since these are vanilla flags, other mods should always respect them regardless of whether VS2 is installed.

thetheaplant commented 1 month ago

This has been addressed by PR 854.

hlysine commented 1 month ago

The block entities are removed but the update flags are still not set. A lot of blocks from Create rely on these flags to react appropriately when assembled.

PriestOfFerns commented 1 month ago

All of the assembly related bugs got fixed in this commit, the other PR is outdated

6k911 commented 1 month ago

https://github.com/ValkyrienSkies/Valkyrien-Skies-2/issues/698#issuecomment-2275861248

Yo I'm the user, Eureka ships devs also agree it's a VS2 issue (I thought it was from their end)

ValkyrienSkies / Valkyrien-Skies-2