Fixes for WorldSaveData dimension handling (#1398) and fluid in pipes…

[warjort]

… not being persisted (#1357)

What:
I have included the fix for #1357 (fluid in pipes is not persisted across restarts/reloads) because I found the main issue while testing that fix.

The main fix is for #1398 which is about properly separating pipenets by dimension and not running pipenet processing on the client.

How solved:
Data for dimensions other than the overworld now have their dimension number appended to the WorldSaveData data id.
e.g. gregtech.fluid_pipe_net is used for the overworld and gregtech.fluid_pipe_net.-1 is used for the nether.

The reference to the world in the WorldPipeNet has been changed to a WeakReference so it doesn't stop garbage collection
of an unloaded dimension, and setWorldInit() can now handle reloading of dimensions by checking for a change instead
of the firstTick flag.

Additionally there are corrections to stop pipenet processing running on the client.
With an additional check to try to spot this happening in future changes.

Outcome:
Fixes: #1398, correctly separate pipenets by dimension.
Fixes: #1357 persist fluid in pipes across reloads.

Possible compatibility issue:
This is the elephant in the room.

People that have built bases in other dimensions, e.g. in space in OmniFactory or FTB interations will have their pipenets currently saved against the overworld.
Despite that it would have kind of worked provided no cables/pipes have the same blockpos in the 2 dimensions.
e.g. PipeNet.getNetFromPos() would choose a "random" dimension's pipenet if block positions are shared

This fix looks for non-overworld data somewhere else, so old pipenet data for non-overworld dimensions will not be used.
There is no way to migrate this old data. The necessary dimension info is not available in the old data to fix it.

To fix these old non-overworld cable/fluid networks, the blocks will need to be broken and replaced to recreate them.

Please fill in as much useful information as possible. Also please remove all unused sections.

[LAGIdiot]

Thank you for creating this PR.

These are some serious claims regarding problems in our WorldPipeNet I will need to look at this myself as I am not currently able to tell if it is correct. Obviously I am quite surprised because we did not have any reports regarding this. And fix which will break someone bases is not something I can easily allow in mod.

If anyone come in contact with something which could be relate to problems which this fixes please step up and let me know.

[warjort]

Did you try the simple test on the bug report?
Add a debug statement to the code that sets fire to the fluid pipe to see what it is doing.

For the central issue, look at WorldServerMulti which is the thing that handles the non-overworld dimensions.

In WorldServerMulti.init() you will find

        this.mapStorage = this.delegate.getMapStorage();

mapStorage is where the pipenets get stored
delegate is the overworld, see forge's dimension manager:

WorldServer world = (dim == 0 ? overWorld : (WorldServer)(new WorldServerMulti(this, isavehandler, dim, overWorld, this.profiler)).init());

[warjort]

To try to avoid breaking old data, I was thinking we could do something to try detect old data and then turn off the bug fix.
i.e. only new worlds would get the bug fix.

This could be something like changing the overworld data to have .0 on the end, in line with how the other dimensions are saved.

If we can find data with the old data ids, we leave the bug in place.

It's not ideal, but since people have been happily playing with this bug in place they probably wouldn't notice it still being there.

The could also manually enable the bug fix for an old world if they have only created pipenets in the overworld
by changing the name of the saved data

i.e. in saved-game-name/data/ change gregtech-e_net.dat (old name) to gregtech-e.net.0.dat (new name) and the same
for the fluid pipes

This patch still uses the old name for the overworld to try to migrate data.

[warjort]

I have implemented my suggestion above.

Now the main bug fix for #1398 will only be available for new worlds, or more accurately if it detects there is no old data.

The fix for fluid pipe persistence #1357, will be there in old and new worlds.

[LAGIdiot]

Thank you for providing compatibility for current worlds this will make it more likely to get accepted.

Regarding test on my end I was not yet able to do it as my current dev environment is broken.

[serenibyss]

Is this possibly related to #1256? I understand that the issue is showing itself from a different part of the PipeNet, being TileEntityPipeBase#getPipeWorld(), but I think it may be caused by the same type of problem.

[warjort]

I don't think it is related. The first crash doesn't involve pipes and the second is in the cover processing of a pipe which is "duplicate code" of the metatileentity processing.

The pipenet code typically checks for World.isBlockLoaded() before accessing tiles so it can't cause cascades.

[LAGIdiot]

I finally get to this and tested your claims and implementation.

As you pointed out there are problems with with WorldPipeNet. And your analysis is up to point and it needs to be fixed.

When testing on new world there were no problems found.

When testing conversion from before this updated I have confirmed that pipenet does not get broken in all dimensions (fluids still go through system without need to replace pipes). But I have found out that wooden pipes when transporting steam in nether still does not burn. Can you please have a look at this case?

[warjort]

For the pipes don't burn in the nether.

If you have old data, yes that will still be a problem.
The bug fix is not activated when old data is present to avoid breaking old saves.

The way to activate the bug fix for old saves is to go into saves/world-name/data and rename gregtech.pipenet-name.dat to gregtech.pipenet-name.0.dat and then do the break and replace on pipes not in the overworld.
Where "pipenet-name" is "e-net" for cables and "fluid_pipe_net" for fluid pipes.

[Archengius]

Can't we just discard old data entirely and lazily rebuild it once any of the emitter blocks requires it?

[warjort]

No, because the saved data remembers any manual disconnections that were made with a wrench.

[warjort]

If we could rebuild the data lazily, there would be no need to save the data at all.

[Archengius]

If we could rebuild the data lazily, there would be no need to save the data at all.

Incorrect, the point of saving data is to avoid world access and chunk loading while doing net-related operations. Other than that, there is no point in saving that data at all.

No, because the saved data remembers any manual disconnections that were made with a wrench.

Incorrect again. Wrench disconnections are stored inside of the pipe tile entities, and are simply mirrored into the saved data. They can be easily reconstructed from just tile entity data.

Overall, I see nothing stopping us from implementing lazy data regeneration.

[warjort]

Ok, so you can in principle regenerate the node data from the pipes.

But doesn't that mean forced chunk loading to do recalcuations of things like cable route paths or the total fluid capacity of the fluid pipes when a pipe is loaded? And doing this everytime a pipe is loaded (or maybe there is a way to do this only if you detect it's node data is missing?). You may also need to cascade into neighbouring tiles as well in case they have things like shutters or to check their capabilities.

My statements above assumed that you don't want this to happen, see my comments on #1256 about doing something like the pipenet optimisations for the redstone signals in tiles and covers.

Also, if the philosophy is that the pipe data is just a cache, my fix for storing the fluid data there is wrong, since it is the actual data.

[Archengius]

Yeah, you will definitely need to force load chunks and cascade into neighbour tiles to regenerate the data. But again, you only need to do it once and only if data in question is missing.

Design philosophy that pipe net is just a data cache is not entirely right. It is the case for cables, but for pipes network in fact owns the currently stored fluid.

On the other side, loss of stored fluid is not that critical because internal buffer of pipes is rather small.

Alternative solution would be saving/loading old data as-is, but once network element requires access to the pipe net owning given position, pipe net in question is removed from old data and moved into the new data associated with the world original requester pipe is in. That way fix can be applied to old world's without completely regenerating their pipe nets.

[warjort]

I am not saying this is not doable, but the purpose of this is to try to fix old data.

A hackier/easier alternative is to just copy the old data into every dimension.
This could be done automagically when the old data is detected and there is no new data for that dimension.

But this runs the risk that at some point a blockpos will overlap with old data from a different dimension.
If that never happens it would be a complete fix.

[warjort]

We seem to have crossed and have had the same idea?

[Archengius]

No, my approach involves keeping old data shared with all dimensions, and then moving it once owner dimension is found. The difference is that if you just copy the data for all dimensions, it will remain dangling for all the dimensions except needed one, and if you have a shared old data, you can clear it once ownership has been claimed by one of the worlds.

[warjort]

ok, i will give that a try and see what problems I find.

The most obvious one is where there is already a conflicting blockpos in the data.
In that case, the save is already broken so maybe notifying the user of the problem and telling them to fix it manually is ok?

[warjort]

Ok, I have committed a fix that automatically moves the old data into the new data as Archengius suggested.

This looks to be working except for the overlapping blockpos which I will address in a seperate comment.
You will get something like the following in your log when it fixes data:

[14:19:39] [Server thread/INFO]: Loading dimension -1 (New World) (net.minecraft.server.integrated.IntegratedServer@308a24d3)
[14:19:41] [Server thread/INFO]: Fixing old data for gregtech.common.pipelike.cable.net.EnergyNet@970ba6f found at BlockPos{x=56, y=53, z=62}
[14:19:41] [Server thread/INFO]: Fixing old data for gregtech.common.pipelike.fluidpipe.net.FluidPipeNet@11187d04 found at BlockPos{x=56, y=53, z=64}
[14:19:41] [Server thread/INFO]: Fixing old data for gregtech.common.pipelike.fluidpipe.net.FluidPipeNet@74178b7f found at BlockPos{x=66, y=53, z=64}

On implementation details:
The best place I could find to put the check is in TileEntityPipeBase::onLoad(). This is the only place where you have access to the tile's world before the cover processing starts looking at other tiles and potentially cascading other loads.
I wanted the fix to happen in a predictable/controlled place where no side effects of different entry points make it difficult to reason about the state.

There are additional safety checks before it fixes data.
e.g. it seems to be possible to have a pipe tile without a PipeBlock, I don't think this is possible in onLoad, but I checked it anyway.
The most important check is that it won't try to fix data if the new data already has something for that block positiion.

[warjort]

For the overlapping blockpos of pipenets in different dimensions this is worse than I thought.

I had thought the getNetFromPos() would choose a random pipenet based on how it processes the list of pipenets in a chunk.

But from my testing, this doesn't seem to happen in practice. Instead, the pipenets will get merged when you create them. Worse than that, it is not just the same blockpos, it is neighbouring blocks as well, because of the way addNode() checks the block connections to the surrounding blocks.

If this has already happened in somebody's world, the data is broken and unfixable. They are probably already experiencing buggy behaviour?

I have however, placed a warning in the code in case it detects such duplicate data. It won't attempt to fix it.

[warjort]

Actually the "It won't attempt to fix it" is only half true.

If there is another pipe in the same pipenet that does not overlap with a pipe in a different dimension, that is enough for the basic data fix to work.

[LAGIdiot]

I understand gist of last update to this PR. But I am not confident enough to review if this approach is correct. So please @Archengius can you look through this.

[serenibyss]

For the overlapping blockpos of pipenets in different dimensions this is worse than I thought.

I had thought the getNetFromPos() would choose a random pipenet based on how it processes the list of pipenets in a chunk.

But from my testing, this doesn't seem to happen in practice. Instead, the pipenets will get merged when you create them. Worse than that, it is not just the same blockpos, it is neighbouring blocks as well, because of the way addNode() checks the block connections to the surrounding blocks.

If this has already happened in somebody's world, the data is broken and unfixable. They are probably already experiencing buggy behaviour?

I have however, placed a warning in the code in case it detects such duplicate data. It won't attempt to fix it.

Is this possibly related to #1256? I understand that the issue is showing itself from a different part of the PipeNet, being TileEntityPipeBase#getPipeWorld(), but I think it may be caused by the same type of problem.

I believe quite confidently now that these two issues are related. The CME crash caused in this issue only occurs when the ID of a material with CableProperties is changed for a save.

[Archengius]

Other than multiple minor formatting/codestyle issues, there is a core issue in the way you are doing the conversion of the old data. I've covered it by the comment on onLoaded. Tl;dr: it's a pretty hot spot and trying to convert data here is probably a bad idea. Alternatives have been suggested in my comment there too.

[Archengius]

You shouldn't check world for null here, these events simply should never have it null.
Might be a good idea to refactor if-not-return to if-then statements, too.

[Archengius]

Ditto.

[Archengius]

Same thing again.

[Archengius]

You should use logging formatting instead of string concatenation here.

[Archengius]

Same thing.

[Archengius]

Use if (nbt.hasTag("FluidNetTank")) here instead.

[Archengius]

I'm not sure if this is a good place to check for legacy data. Since it will be called pretty often (actually for every pipe-like tile entity loaded), it might incur some overhead, especially taking into account that usually pipe tile entities don't perform any work themselves and don't even tick unless they have to.

Besides, I'm not really sure if you can obtain the block object in question at that point. You probably can, though, because it's saved in NBT. Although it was not always the case, but I think we shouldn't care too much about ancient versions where pipe block was actually retrieved from the world object and never saved.

It might be a good idea to move these checks to some kind of place which is not called that often, or at least not for all pipelike tile entities in the world. Some places that come to mind quickly: "active" fluid pipe tile entities, "source" type cable tile entities, onBlockAdded which checks for neighbor networks.
Although it leaves logic split across multiple places, so it's not the best solution there.

Alternatively we can try to move migration logic to getNetFromPos, e.g. if pipe net is not found, it will try to check for it by digging through old data and restoring it from there. Since getNetFromPos is mostly used only in 2 cases (performing actual transfer logic and joining neighbor nets), and on case 1, which is also the most common one, it will always have a valid network, it shouldn't incur any kind of noticeable performance impact. On case 2 it shouldn't really matter, since that logic is only caused by players placing/breaking blocks, and these actions just don't happen every tick, and even then, hash map lookup is pretty fast.

I personally think last suggestion is the best one to implement, but I'm always open for a civil discussion, especially from @LAGIdiot.

[warjort]

I have done the changes you asked for, with one exception.

getCompoundTag() already does the hasKey() test with the additional check that it is actually a compound tag (type 10).
Adding my own check would just be duplicate work, or maybe I am missing something?

The only reason for adding this check is so we don't run into an NPE with old data.
Once the data has been loaded and saved it will always have this tag.

[warjort]

On the main issue, I can see that putting the check for old data at the end of getNetPos() is slightly more efficient.
I have done this and it works.

Most of the calls are from network topology changes.
The two main ones are to get the energy container for the cables and the fluid tank handler for the fluid pipes.

These are only obtained for the "active" pipes as you say, and they are cached after their first use.
But since the cached object is stored in the pipe tile, they are still going to have the same frequency as the onLoad()?
i.e. when the pipe is unloaded/loaded with its chunk the cached pipenet will need to be refreshed in the new pipe tile.

It does however mean that non-active pipes don't have to do this processing.
The downside to this is that they also won't take part in checking for old data.

If as I mentioned above, one of these pipes could be used to resolve a pipe conflict across dimensions, it won't happen.
But as I also said above, I don't think that kind of data exists in practice.

As an aside. I think the FluidPipeFluidHandler could actually be resolved once and shared between all fluid pipes in the pipe network?
Its only reason to be in the pipe is to lazy construct itself using the position stored in the pipe to determine the pipe network.

[warjort]

As an aside. I think the FluidPipeFluidHandler could actually be resolved once and shared between all fluid pipes in the pipe network?
Its only reason to be in the pipe is to lazy construct itself using the position stored in the pipe to determine the pipe network.

But maybe that doesn't save you very much since you still need to go through getNetFromPos() at least once.

[Archengius]

I don't think sharing fluid handler will save much. Honestly though, both cables and pipes ideally should be refactored to use network segments to make their behaviour more consistent.
And as of getCompoundTag, it's just a normal practice to use hasTag instead of retrieving it and checking for null afterwards. It cannot be really questioned since entire project is kinda following it now.

[warjort]

On getCompundTag(). I can see both, even in the same code, e.g. MetaTileEntity :-)

 @SideOnly(Side.CLIENT)
    public int getPaintingColorForRendering() {
        if (getWorld() == null && renderContextStack != null) {
            NBTTagCompound tagCompound = renderContextStack.getTagCompound();
            if (tagCompound != null && tagCompound.hasKey("PaintingColor", NBT.TAG_INT)) {
                return tagCompound.getInteger("PaintingColor");
            }
        }
        return paintingColor;
    }

    /**
     * Called from ItemBlock to initialize this MTE with data contained in ItemStack
     *
     * @param itemStack itemstack of itemblock
     */
    public void initFromItemStackData(NBTTagCompound itemStack) {
        if (itemStack.hasKey("PaintingColor", NBT.TAG_INT)) {
            setPaintingColor(itemStack.getInteger("PaintingColor"));
        }

But it is difficult to find anything that is doing the same as this usecase.
All the nested compound tags seem to be either lists or assumed to exist.

I don't think "everybody else is doing it" is a good excuse for doing it the wrong thing (or in this case the inefficient thing). :-)

Same with those changes you asked me to make for logging.
It went from some StringBuilder.append()s (that's how concatination is implemented by the compiler)
to a parse/tokenisation of a String and a new Object[] construction.
And the result is in my opinion less readable/more error prone.
I could understand if it was translated and so need to be done that way.

[Archengius]

About the getTagCompound, here is the very simple difference between the code you provided and what we have in this PR: getTagCompound in the provided code is called on ItemStack, since stacks can have no component at all and it's a pretty normal thing. In the code of PR (and the cases I mentioned) we are talking about retrieving children tags from the NBTTagComponent. Since numbers and other stuff are technically subtags too but they cannot have null value, we are using hasTag to check for their existence before trying to use them, because otherwise we would end up with 0 if the tag doesn't exist. So to keep things uniform, you should use hasTag instead of checking getCompoundTag result for null.

And regarding logging, no, string concat is not readable at all, that's why people invented formatting and built up logging libraries that support it. It's much easier to see the original message in one place without having it split by the values inside.
Overall, I would really appreciate if you just applied these changes without arguing on their purpose, which is my opinion not too much to ask.

[warjort]

I did your change and while I was testing it I found real problem

Basically the FluidNetTank was not marking the pipenet as dirty when the fluid amount changes.
So the fluids were still not always getting persisted properly.

I can't believe I hadn't spotted it before, but I guess the way I was testing it meant the pipenet was always getting marked as dirty due to connection changes when I broke tanks.
I was doing that to make sure the fluid I was seeing in the pipe was actually the saved fluid and not just more fluid from the tank.

Anyway, I will do some more testing on this tomorrow or later to see if I can find another way to break it.
All this pettifogging is demotivating.

[warjort]

And that's why you don't make nonfunctional changes, it introduces unintended bugs.

-        if (nbt.hasKey("FluidNetTank", NBT.TAG_COMPOUND));
+        if (nbt.hasKey("FluidNetTank", NBT.TAG_COMPOUND))

[LAGIdiot]

@Archengius Can you please give this another look. There were some changes from your last comment.