Sophisticated Storage

Sophisticated Storage

20M Downloads

Item duplication issue with Valkyrien Skies 2 + Eureka

dwayn opened this issue ยท 10 comments

commented

When running VS2 and Eureka and assembling or disassembling a Eureka ship, if there is a sophisticated storage chest on the ship with contents, the entire contents of the chest are spit out into the world as if the chest had been broken.
I have filed a corresponding issue on Eureka's issue tracker with details about this here: ValkyrienSkies/Eureka#295

I am filing an issue here too because in going through their outstanding issues, I did come across an issue that seems related and mentions that storage mods may need to implement/use the clearable class for their storage block entities to make them compatible with Valkyrien Skies across the board. See comment here: ValkyrienSkies/Eureka#270 (comment)

I have also considered seeing if I could possibly figure out making a separate mod to add compatibility between VS/Eureka and other mods that implement custom handling of block entity clearing, but this would be delving into a coding ecosystem I am completely unfamiliar with.

commented

closing as there's a compat mod now to handle this

commented

I did some digging and decided it looked not too bad to try and create a compatibility mod to patch the behavior, so for now I have created this: https://github.com/dwayn/ValkyrienSkiesSophisticatedStorageCompat to make sophisticated storage work with VS2. I have posted it to curse, so once it is approved, it will be available to whoever wants it.
Effectively it just sets the packed state (same thing as applying packing tape from the same mod) on sophisticated storage chests/barrels when Clearable is called by VS2.
The effect is that when VS2 calls relocate block, it clones the block and its metadata, calls the Clearable implementation I added to the Sophisticated container which sets itself as packed (only when it is called from VS2), so that when VS2 destroys the container block to relocate it to wherever the blocks are moved for physics simulation (i haven't figured out exactly where this is but it is not in the same place in the world where the ship was built), the container only drops itself and not its contents, allowing VS2 to place the cloned block in the new location.
btw, it's actually quite interesting how VS manages things to allow you to have actual interactive blocks on a moving ship by having you actually interacting with blocks in another stationary location that are rendered in the ship's location, really quite an awesome approach.

commented

So cool that you have made a compat here, but just be aware that when I change one of the internal things that you are referring to (which I admit is not very likely) your compat will break and you will need to fix it.
Also I am not sure what exactly they are doing with blocks/items when on the ship and yes it may not drop the items when you set it to packed, but they may be some unforseen consequences depending on how exactly they turn it into item and then back into block. Also some upgrades only work in the location of the actual block which would be in that invisible location but that's probably the least of the worries.

Anyway I feel like this may be a big rabbit whole which I honestly don't have time for now and would much rather focus on new features with that time so if you don't mind I would leave it at this.

commented

Yeah, I do realize that all it will take is you changing something in the internals of SS and it breaks my compat mod, but that is why I made it a point to use the public API on the base BlockEntity to call setPacked(), in hopes that since it was a public function it would be much less likely you would change in minor version updates.

Regarding what Valkyrien Skies 2 is doing, here is generally what happens when you "assemble" a ship that then gets physics applied to it.:
(Note: this part is context to understand why the blocks are being relocated in the first place) The VS2 code picks some new chunk location where the blocks are going to be relocated to, this is somewhere in the world (not sure if a separate dimension or same dimension very far away) as they structure is moved to this other place and then the ship is rendered as kind of an entity or as maybe a viewport on the structure's location (i think, details get fuzzy here for me) that the player can see and some trickery that allows the player to interact as if the blocks were located where the player is. Somehow they map player interactions with blocks on the ship that they see with the blocks in their actual location while the ship is assembled.

Once it picks/creates the new location for the ship's blocks, VS2 walks all the blocks connected to the ship helm that was activated (with some logic to make it stop walking if it thinks it has hit ground so it doesn't try to create a ship that is the world) and for each of those blocks, it clones the block and its state/metadata. Then, if the BlockEntity implements Clearable, it calls clearContent() on the block (in vanilla and a number of modded BlockEntities this just deletes all the items and/or resets the state of the block) so that when the block is relocated, it does not drop its items. What happens during relocation is the old location of the BlockEntity is set to air (you can recreate this effect in creative by filling a chest with items and using the /fill command to set the block to air and all) and the cloned value of the block with all of its metadata is placed in the other location as part of the ship. This results in all the items spilling to the ground if the behavior of the BlockEntity is setup to drop all the items when it is broken.

You can see the code that is doing this here: https://github.com/ValkyrienSkies/Valkyrien-Skies-2/blob/1.18.x/main/common/src/main/kotlin/org/valkyrienskies/mod/util/RelocationUtil.kt#L28-L69
Specifically this bit:
`
var state = fromChunk.getBlockState(from)
val entity = fromChunk.getBlockEntity(from)

val tag = entity?.let {
    val tag = it.saveWithFullMetadata()
    tag.putInt("x", to.x)
    tag.putInt("y", to.y)
    tag.putInt("z", to.z)

    // so that it won't drop its contents
    if (it is Clearable) {
        it.clearContent()
    }

`

So for sophisticated storage, I noticed that it had a packed state for when packing tape is applied and i realized that the least intrusive way to stop the items from dropping when the block entity is set to air would be to implement Clearable for sophisticated chests/barrels which would cause VS2 to call clear after it had cloned the block and before it sets it to air and then in the clear function, just set the packed state on the block so it does not drop the items. I did add code in my implementation to only apply this logic of calling setPacked if the clearContent() call is coming from a "com.valkyrienskies" class in hopes that this will minimize the possibilities of side effects.

As a separate thread of thought here, if you could provide me some insight into Sophisticated Storage operation, I could also potentially just submit a pull request to properly implement Clearable for Sophisticated Storage containers.
Is there any particular reason why implementing Clearable (https://nekoyue.github.io/ForgeJavaDocs-NG/javadoc/1.18.2/net/minecraft/world/Clearable.html) with a clearContent() function that just deletes all the items/upgrades/etc contained in the BlockEntity would be a bad thing?
My concern is that I do not know the innards of SS, so I am not sure if Clearable was purposely left out because there are bad side effects or something. Also, I could just try it out and see if it breaks anything myself, but I would appreciate any insight you could add.

commented

I added two implementations for clearable for you to take a look at. The first requires a small change to be added to SophisticatedCore to add a deleteItems inventory helper function, so there are PRs for SophisticatedStorage and SophisticatedCore. The second is just a standalone implementation where the iterator code for clearing the items is embedded inside StorageBlockEntity's class.
Both solutions are functionally the exact same implementation, but I threw the standalone one together just in case you don't want to necessarily expose a deleteItems function in SophisticatedCore's InventoryHelper.

I implemented these for 1.19 as that is the minecraft version that the modpack I am working on fixing (https://www.curseforge.com/minecraft/modpacks/magitech-fantasia) is running, but if you would like me to back/forward port this to the other versions, I would be happy to do so, just let me know which implementation you prefer and I will happily port this change to the other minecraft versions.

commented

Sorry, but I am not in favor of adding this kind of implementation. First of all that implementation also needs to be supported going forward and the other thing is I am totally confused why inventory needs to be cleared at all, the mod should be able to work with storage without actually deleting its contents or should properly extract the items from the storage.

Feel free to have this as a compatibility mod, but it just seems broken to me so it's not going to be part of the mod.

commented

That makes sense, this is why I initially looked into making it as a compatibility mod because I am unsure if there would be other unintended consequences to having the Clearble trait implemented, like if are there other mods that might call clearContent if the clearable trait is implemented and end up deleting the contents of a chest unexpectedly.

commented

I actually didn't realize that Clearable was a thing, but as far as I can see the reason it exists is for structure generation to be able to replace block with inventory without items spilling out. But in the vanilla case the whole inventory is also disregarded and just plain vanishes so I see the case for converting storage to flying machine very differently because in that case you actually care about the items and want to keep them in which case the mod actually needs to copy them somehow and should be able to just use regular item handler capability and methods.

commented

Yeah, I think what I am going to do is reach out to VS2 team and see about (maybe with a PR) restructuring their code just a little bit to make it easy for other mods to add compatibility with VS2. Eg: something like adding a prepare for relocation function that other mod authors can implement to provide explicit compatibility for any type of block/entity/etc (think create contraption blocks, inventories, etc).
If I can get them to add something like this, I will likely come back with a PR to implement the compatibility here.