Run data fixers on tile entities/items containing other tile entities/items
williewillus opened this issue ยท 6 comments
Datafixers will not run on anything holding itemstacks (drives, any machines holding stacks), meaning any stack format changes made by Mojang or other modders will not be run.
I don't think this is going to happen anytime soon. The good old solutions like dummy items with a conversion mechanism should be preferred by modders.
This is most likely not possible due to AE2 relying on immutable itemstacks.
Should we expose them to any external consumer, it could easily corrupt every cell storing the same item.
Especially with it being completely undocumented, there is no information about its behaviour. Nothing about important details like when it does run. E.g. on chunk load. If so how well does it handle the multithreaded chunkloading adding by Forge? As if this wasn't already a massive issue for TE related data. And so on.
Would forge provide a manual way to trigger it, it should be easier to support. But it isn't as usual. Also this can easily end up with a very bad performance issue. ItemStacks are already a nightmare due to caps and now adding some automatic migration on top, which might run every time a chunk is loaded will continue to add up.
fixers don't run if the data version of the mod/game is newer than that of the fixer, so the overhead is once, at migration-time. All walkers/fixers are run at chunk load time, see AnvilChunkLoader::loadChunk__Async (it invokes for CHUNK fixtype, and the walkers take care of recursing into the entity/te lists)
but anyways, I think it should be run because vanilla stacks will become corrupted inside drives if mojang adds a fixer in a minor patch (I don't know if this has happened, but it's possible). it also prevents any other mods from using this nice system for tweaking their formats
The idea is nice. But really lacking for anything a bit more sophisticated than vanilla chests.
As usual Forge turns a probably non thread safe vanilla thing into a multithreaded system with who knows what issues will arise from it. Not that this kind of changes hasn't caused issues since 1.7.10 and only saw some fixes in 1.12.
Further it is designed as essential requirement, but only opt in. As long as there is a single mod not supporting it, we have to constantly consider corrupted cells are being used. Based on how notoriously bad the documentation for Forge is, no or incompletel support will certainly be the rule not the exception.
While simultaneously it prevents from explicitly fixing cells at a more convenient point. E.g. when actually loading them. Which would also cover fixing any old cell, which survived somehow in a bag or some other inventory without support for it.
Further the NBT data we use internally to store cells is not perfectly clean. Should any fixer decide to work on a clean state and only migrated necessary tags it can easily corrupt the whole cell. That is something I wanted to fix, but will not be possible before 1.13. As the system who could fix it, is not sufficient for it due to being opt in.
That might also break caps in general should someone not consider that anything could add new caps to itemstacks and they have to explicitly migrate them over. Or what happens when a cap needs a migration, but has to fight over a shared version with the normal itemstack.
I simply do not see it happening before 1.13 (or maybe even 1.14 should it need to mature a bit more). There are simply to many potential issues currently and we are not talking about losing the content of a random furnace but potentially all personal assets. Should there be a 1.12.3 with a fixer, it's is probably easier to manually migrated it.
Fixers are threadsafe are since each individual fixer is self contained and (from mojang's impls) pure functions from input to output.
That being said your concerns are understandable, I just wanted to raise awareness in the issue. Thanks!