search

Tfarcenim / DankStorageFabric

Creative Commons Zero v1.0 Universal
11 stars 9 forks source link

Danks in Docks wipe their inventory when the chunk is unloaded and loaded again. #18

Closed dandin87 closed 3 years ago

dandin87 commented 3 years ago

Danks in Docks wipe their inventory when the chunk is unloaded and loaded again. If the dock is in a spawn chunk it does not unload and therefore does not get deleted. It will however get deleted when in a spawn chunk on server restart.

Fourmisain commented 3 years ago

I did some digging into this. The dank seems to be properly serialized, but on deserialization, the container size is 0 here: https://github.com/Tfarcenim/DankStorageFabric/blob/f5c8abb547f73d44d372d3067489b75e66870582/src/main/java/tfar/dankstorage/inventory/DankInventory.java#L181 and thus the items won't be loaded and are lost.

After some debugging I found this is because the dank stats (which include it's size) are not read and thus 0. After comparing commits, this is because of https://github.com/Tfarcenim/DankStorageFabric/commit/f5c8abb547f73d44d372d3067489b75e66870582, which was implemented to fix #15, the dank stats are not stored and loaded as NBT anymore.

This commit either needs to be reverted (and #15 fixed differently), or the dank stats need to be read differently. Maybe it's possible to read the dank stats from the block state? Not sure if that's easily accessible inside the block entity though.

Tfarcenim commented 3 years ago

This won't be a problem in 1.17 as I'm changing danks to no longer store the data directly. Personally I think 1.16's time is running out so I'll rewrite that now rather than trying to prop up an old, buggy system for the sake of save compat.

In the future, the blockstate will only be used for model purposes and have nothing to do with the actual contents/stats

Fourmisain commented 3 years ago

I'm changing danks to no longer store the data directly

What do you mean by not storing the data "directly"?

I don't see much wrong with how the data is currently de/serialized, only with how it's handled, initialization mainly.

Personally I think 1.16's time is running out

Yeah, version 1.09a works perfectly fine (except for slot locking), so a backport for the fixes isn't really needed.

rather than trying to prop up an old, buggy system for the sake of save compat

If you're breaking save compat, just make sure to warn people before updating, with big bold letters: "EMPTY YOUR DANKS BEFORE UPDATING" Something like that.

Tfarcenim commented 3 years ago

What do you mean by not storing the data "directly"?

Dank contents will now be saved on the world instead of on the item

If you're breaking save compat, just make sure to warn people before updating, with big bold letters:

that's why I waited until 1.17 to update it, 1.10(a) was supposed to be a bugfix, but it also has an accidental break that I can't fix now.

Fourmisain commented 3 years ago

Dank contents will now be saved on the world instead of on the item

Hm... What would be the benefit of that? You still need to associate the dank with the data, so wouldn't you still need to store some data in the dank ItemStack or dock BlockEntity either way?

Tfarcenim commented 3 years ago

Hm... What would be the benefit of that?

Less chances of accidental item wipe or bookban

You still need to associate the dank with the data, so wouldn't you still need to store some data in the dank ItemStack or dock BlockEntity either way?

Easy, store an id to the nbt that looks up the contents on the world save.,

Fourmisain commented 3 years ago

Easy, store an id to the nbt that looks up the contents on the world save.,

Yeah, that's what I meant, you still store the id on the dank/dock, so at least in theory it could get "lost" somehow, effectively losing the data too, but I guess that's much less likely to happen, so nevermind about that.

Less chances of accidental item wipe

This change by itself doesn't really protect the data though. E.g. if your dank couldn't read the data and turns into an empty dank, since it still has the id associated to it, it might then overwrite the existing data.

The only way to prevent item wipes is to make sure that whenever the data could not be read correctly is to forcibly disable the saving of data - which could then turn into another issue where if you put items in a "broken" dank, they wouldn't be saved and are thus lost - so ideally, the whole dank would need to be disabled. (And that could be done with the current system too.)

Less chances of (...) bookban

That's a fair point!

I wonder though, doesn't the world data have a size limit too?

One disadvantage I can think of is that data might be too persistent because it doesn't automatically get deleted anymore. So you might have to check if a dank item or a dock got destroyed - and I can already think of a few ways that are probably undetectable.

That may not be an actual problem though.

Tfarcenim commented 3 years ago

fixed in 2.0

BurnyLlama commented 3 years ago

Is there any chance this can be fixed for 1.16.5? I want to use danks in combination with ME Systems from Applied Energistics 2 (which does not yet have a 1.17 version)... Or is there a way to compile the 1.17 version against 1.16.5?

Tfarcenim commented 3 years ago

Sure, but any 1.16.5 fixes will not be made by me. I simply do not have the time as work is more important. If you still insist on 1.16.5, use 1.9a which doesn't have this issue

BurnyLlama commented 3 years ago

Sure, but any 1.16.5 fixes will not be made by me. I simply do not have the time as work is more important. If you still insist on 1.16.5, use 1.9a which doesn't have this issue

Oh, thanks! Didn't know about 1.9a :)