-
Notifications
You must be signed in to change notification settings - Fork 149
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes for WorldSaveData dimension handling (#1398) and fluid in pipes… #1399
base: master
Are you sure you want to change the base?
Changes from 4 commits
9619b5a
bf14f64
82fca84
f4e5d18
36d2b78
608bf20
3929a74
34c7401
9b63d6d
02ebd18
51205a1
ab7a60e
4bb2990
94ff7cb
fe74783
ad872d2
e7b76d6
fc3b7f1
809e445
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,27 +9,38 @@ | |
import net.minecraft.world.storage.WorldSavedData; | ||
import net.minecraftforge.common.util.Constants.NBT; | ||
|
||
import java.lang.ref.WeakReference; | ||
import java.util.*; | ||
|
||
import gregtech.api.util.GTLog; | ||
|
||
public abstract class WorldPipeNet<NodeDataType, T extends PipeNet<NodeDataType>> extends WorldSavedData { | ||
|
||
private World world; | ||
protected boolean isFirstTick = true; | ||
private WeakReference<World> worldRef = new WeakReference<>(null); | ||
protected List<T> pipeNets = new ArrayList<>(); | ||
protected Map<ChunkPos, List<T>> pipeNetsByChunk = new HashMap<>(); | ||
protected WorldPipeNet<NodeDataType, T> oldData = null; | ||
protected boolean checkedForOldData = false; | ||
|
||
public static String getDataID(final String baseID, final World world) { | ||
if (world == null || world.isRemote) | ||
throw new RuntimeException("WorldPipeNet should only be created on the server!"); | ||
final int dimension = world.provider.getDimension(); | ||
return baseID + '.' + dimension; | ||
} | ||
|
||
public WorldPipeNet(String name) { | ||
super(name); | ||
} | ||
|
||
public World getWorld() { | ||
return world; | ||
return this.worldRef.get(); | ||
} | ||
|
||
protected void setWorldAndInit(World world) { | ||
if (isFirstTick) { | ||
this.world = world; | ||
this.isFirstTick = false; | ||
// Reset the world as the dimensions are loaded/unloaded | ||
if (world != this.worldRef.get()) { | ||
this.worldRef = new WeakReference<World>(world); | ||
onWorldSet(); | ||
} | ||
} | ||
|
@@ -122,6 +133,46 @@ protected void removePipeNet(T pipeNet) { | |
|
||
protected abstract T createNetInstance(); | ||
|
||
/* | ||
* This method is invoked during tile loading | ||
* | ||
* It's purpose is to move pipenets from the old data file to new one based on matching block position | ||
*/ | ||
public void checkForOldData(final BlockPos blockPos) { | ||
// No old data | ||
if (this.oldData == null || this.oldData.pipeNets.isEmpty()) | ||
return; | ||
|
||
// We have new data at this position so don't try to fix | ||
if (getNetFromPos(blockPos) != null) | ||
return; | ||
|
||
// See if we have a pipenet for this block pos in the old data | ||
T foundOldData = null; | ||
final List<T> oldPipeNets = this.oldData.pipeNetsByChunk.getOrDefault(new ChunkPos(blockPos), Collections.emptyList()); | ||
for (T pipeNet : oldPipeNets) { | ||
if (pipeNet.containsNode(blockPos)) { | ||
if (foundOldData != null) | ||
{ | ||
// We have 2 pipenets at this position? | ||
GTLog.logger.warn("Found duplicate pipenets in old data at " + blockPos + " [" + foundOldData + ',' + pipeNet + ']'); | ||
return; | ||
} | ||
foundOldData = pipeNet; | ||
} | ||
} | ||
// Nothing found | ||
if (foundOldData == null) | ||
return; | ||
// Move the old data into the new data | ||
GTLog.logger.info("Fixing old data for " + foundOldData + " found at " + blockPos); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same thing. |
||
this.oldData.removePipeNet(foundOldData); | ||
this.oldData.markDirty(); | ||
this.addPipeNetSilently(foundOldData); | ||
this.markDirty(); | ||
foundOldData.setWorldData(this); | ||
} | ||
|
||
@Override | ||
public void readFromNBT(NBTTagCompound nbt) { | ||
this.pipeNets = new ArrayList<>(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,16 +27,25 @@ public static void registerTickablePipeNet(Function<World, TickableWorldPipeNet< | |
|
||
@SubscribeEvent | ||
public static void onWorldTick(WorldTickEvent event) { | ||
getPipeNetsForWorld(event.world).forEach(TickableWorldPipeNet::update); | ||
final World world = event.world; | ||
if (world == null || world.isRemote) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You shouldn't check world for null here, these events simply should never have it null. |
||
return; | ||
getPipeNetsForWorld(world).forEach(TickableWorldPipeNet::update); | ||
} | ||
|
||
@SubscribeEvent | ||
public static void onChunkLoad(ChunkEvent.Load event) { | ||
getPipeNetsForWorld(event.getWorld()).forEach(it -> it.onChunkLoaded(event.getChunk())); | ||
final World world = event.getWorld(); | ||
if (world == null || world.isRemote) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto. |
||
return; | ||
getPipeNetsForWorld(world).forEach(it -> it.onChunkLoaded(event.getChunk())); | ||
} | ||
|
||
@SubscribeEvent | ||
public static void onChunkUnload(ChunkEvent.Unload event) { | ||
getPipeNetsForWorld(event.getWorld()).forEach(it -> it.onChunkUnloaded(event.getChunk())); | ||
final World world = event.getWorld(); | ||
if (world == null || world.isRemote) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same thing again. |
||
return; | ||
getPipeNetsForWorld(world).forEach(it -> it.onChunkUnloaded(event.getChunk())); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -283,6 +283,11 @@ public void readFromNBT(NBTTagCompound compound) { | |
@Override | ||
public void onLoad() { | ||
super.onLoad(); | ||
final World world = getWorld(); | ||
final BlockPipe<PipeType, NodeDataType, ?> pipeBlock = getPipeBlock(); | ||
if (world != null && !world.isRemote && pipeBlock != null) { | ||
pipeBlock.getWorldPipeNet(world).checkForOldData(getPos()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. 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. |
||
} | ||
this.coverableImplementation.onLoad(); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -154,4 +154,19 @@ protected FluidPipeProperties readNodeData(NBTTagCompound tagCompound) { | |
return new FluidPipeProperties(maxTemperature, throughput, gasProof); | ||
} | ||
|
||
@Override | ||
public NBTTagCompound serializeNBT() { | ||
final NBTTagCompound nbt = super.serializeNBT(); | ||
nbt.setTag("FluidNetTank", this.fluidNetTank.writeToNBT(new NBTTagCompound())); | ||
return nbt; | ||
} | ||
|
||
@Override | ||
public void deserializeNBT(final NBTTagCompound nbt) { | ||
super.deserializeNBT(nbt); | ||
final NBTTagCompound tankData = nbt.getCompoundTag("FluidNetTank"); | ||
// Guard against old data that didn't have this tag | ||
if (tankData != null) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use if (nbt.hasTag("FluidNetTank")) here instead. |
||
this.fluidNetTank.readFromNBT(tankData); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should use logging formatting instead of string concatenation here.