Xcom6000
Assigned
Reported
MC-171901
Item desync bug
Awaiting Response
MC-128878
Dispensers and droppers rotating then rotating back
Duplicate
MC-122524
Dragon eggs no longer replace the block they land on in lazy chunks
Works As Intended
MC-119994
Fires in the nether cause chunks to permanently load and cause lag
Awaiting Response
Comments
This bugs not fully resolved properly. The route of the bug stems in parts of the entity update code that haven't been fixed yet. The reason tripwires permanently stay on is related to ghost entity's existing in the chunks but not added to the global entity list. Any form of ghost entity needs to be caught and throw a proper exception in the console or give any hints or warnings that entity's that aren't properly added to the correct lists exist in the world. If no exceptions or warnings are sent then any bug creating ghost entity's can cause memory leaks and other issues as they can build up in the loaded chunks indefinitely without being handled.
MC-98153 was already fixed in 13 though. The bug where entity's would pingpong using portals.
There are two bugs related to leashes. One is regarding invisible leads and one have to do with the leashes breaking. Kevin Gagnon suggestion for the fix certainly does the job but I would suggest to not fix the bug in the simpler way as suggested by Kevin and go for a more interesting approach that will have deeper mechanics for those that seek it out.
Invisible leashes bug.
When walking further then 80 blocks from mobs that are attached with leashes and walking back the leashes become invisible. This is because leashes are part of EntityTracker and have a hardcoded range of 80 blocks as all animals have a range of 80.
else if (entityIn instanceof IAnimals)
{
this.addEntityToTracker(entityIn, 80, 3, true);
}else if (entityIn instanceof EntityHanging)
{
this.addEntityToTracker(entityIn, 160, Integer.MAX_VALUE, false);
}The issue comes from the fact that leashes are sent to the client after the entity first become entity processing. The packets that link entities with there respective leashes are sent to players in EntityLiving.java by the following code.
public void setLeashedToEntity(Entity entityIn, boolean sendAttachNotification)
{
this.isLeashed = true;
this.leashedToEntity = entityIn;
if (!this.world.isRemote && sendAttachNotification && this.world instanceof WorldServer)
{
((WorldServer)this.world).getEntityTracker().sendToAllTrackingEntity(this, new SPacketEntityAttach(this, this.leashedToEntity));
}
if (this.isRiding())
{
this.dismountRidingEntity();
}
}
Its clear that the leash packets are sent to only players within range. This means that any player that is further then 80 blocks from the entity never receive the packets and assume the mobs don't have a leash. This makes all mobs lose there leashes unless players can somehow teleporting to the mobs directly via commands or logging in next to them. The same issue is that the leashes are removed on the client if the player have a higher render distance and simply walk further then 80 blocks to stop seeing the mob and walks back.
The fix was done with aid from Pokechu22. There is a code in EntityTrackerEntry.java that re-updates players in case a player gets in range in public void updatePlayerEntity(EntityPlayerMP playerMP).
By placing the following line of code in the updatePlayerEntity somewhere in the large if statement the bug of invisible leashes fixes itself. The leashes simply re-update as the player gets close enough again to the animals.
if(this.trackedEntity instanceof EntityLiving){
playerMP.connection.sendPacket(new SPacketEntityAttach(this.trackedEntity, ((EntityLiving)this.trackedEntity).getLeashedToEntity() ) );
}
Breaking leashes
The second bug regarding the breaking leashes comes from what Kevin Gagnon explains. The NBT of entities never save to disk as the entity needs to become entity processing at least for 1 gametick to reload the leash in the following code found in EntityLiving.java recreateLeash(). The leash is simply recreated and the leash can be saved to disk again. If the leash never becomes updated the leash isn't saved breaking it the next time the chunk is reloaded. This happens if the mob is loaded into lazy chunks and never update.
As an alternative fix I suggest that the recreateLeash() code is run directly after the entity is placed into the world after loading the chunk. This can be done by having the recreateLeash() run in the spawn entity into world. This will ensure that entities that are loaded get there leashes reattached instantly without problems as there leashes otherwise wont be attached until they update at least ones. This solves the breaking of leash bug and ensure the mob doesn't need to become loaded to become attached with its leash. It is a safer way to fix the bug as it guaranties that the mob never is detached from its leash at any given time that can cause issues in rare instances.
Just as a suggestion. Add the one "entityIn.postLoad();" line in onEntityAdded in WorldServer.java
protected void onEntityAdded(Entity entityIn)
{
super.onEntityAdded(entityIn);
this.entitiesById.addKey(entityIn.getEntityId(), entityIn);
this.entitiesByUuid.put(entityIn.getUniqueID(), entityIn);
Entity[] aentity = entityIn.getParts();
entityIn.postLoad(); // <-- Add this line
if (aentity != null)
{
for (Entity entity : aentity)
{
this.entitiesById.addKey(entity.getEntityId(), entity);
}
}
}This line could then be used to post load the leashes to animals right after the animal is loaded into the world.
This bug is related to the update order of tile entities. When tile entities are saved to disk they are saved using a hashlist. But before they are saved the game keeps a strict FIFO. The order is critical in most instances making the save process lose the proper order in which tile entities should be updated causing issues to both flying machines (generally anything to do with pistons), hoppers and most notably instant wires.
Of course the loading process will lose the order if incorrect order of chunks are loaded but internally inside each chunk the order scrambles as the saving process uses a hash order based on the position. The order can be kept by simply using a linked hash map making anything that relies heavily on the correct order correctly ordered after reload.
The list is found in Chunk.java
private final Map<BlockPos, TileEntity> chunkTileEntityMap;this.chunkTileEntityMap = Maps.<BlockPos, TileEntity>newHashMap();The saving happens in AnvilChunkLoader.java
for (TileEntity tileentity : chunkIn.getTileEntityMap().values())
{
NBTTagCompound nbttagcompound3 = tileentity.writeToNBT(new NBTTagCompound());
nbttaglist2.appendTag(nbttagcompound3);
}This clearly shows that saving happens based on a hash implementation instead of the order that is used during runtime as FIFO. If the newHashMap is changed to newLinkedHashMap the order is kept making any instant wire function properly.
There is a separate bug related to pistons as well where the proper order of moving blocks aren't saved properly.
Writing to NBT saves the progress by using last progress
compound.setFloat("progress", this.lastProgress )
Reading from NBT then loads it into both progress and last progress creating an inconsistency
this.progress = compound.getFloat("progress");
this.lastProgress = this.progress;It should be changed to
Writing to NBT
compound.setFloat("progress", this.progress);
compound.setFloat("lastProgress", this.lastProgress);
Reading from NBT
this.progress = compound.getFloat("progress");
this.lastProgress = compound.getFloat("lastProgress");
These 2 small fixes will help any inconsistency that happens to pistons after reloads.
Edit:
Another potential unexplored problem can be the scheduled block events. They are processed after even chunk unloading process and tile entities plus the auto-save in the "sendQueuedBlockEvents()". This means that any block events that were scheduled never gets processed if an auto-save happens.
As this list keeps track of all block events its responsible for both updates to blocks such as pistons and is critical in flying machines and other piston based contraptions. When the chunk is unloaded the list keeps track of any block events until the chunk is reloaded. But if the server shuts down the list is simply lost.
This list should probably be saved to disk in some ways to be recovered after a server restart to not cause issues.
This bug is created because of the Temple generator getting an offset when generating. Temples like any other structure has an outer main bounding box and all inner structure bounding boxes, for the case of witch huts only one inner bounding box aka hut exists. To spawn a witch inside a which hut the position needs to be inside both bounding boxes meaning they need to perfectly overlap or any non overlapped areas will fail to spawn a witch.
As is known there is a bug that makes the outer Temple bounding box generate always at Y=64 but the hut bounding box can be offset above or below the Temple bounding box. The main problem is that the Temple and the structure bounding boxes are created prior to generating any other blocks. The Temple is first created then its child structure that places the hut into the world. The hut has a fixed and valid location at Y=64. The Temple bounding box is then rapped around the inner hut bounding box. This can be seen at MapGenScatteredFeature.java line 172.
public Start(World worldIn, Random random, int chunkX, int chunkZ, Biome biomeIn)
{
super(chunkX, chunkZ);
if (biomeIn != Biomes.JUNGLE && biomeIn != Biomes.JUNGLE_HILLS)
{
if (biomeIn == Biomes.SWAMPLAND)
{
System.out.println("swampland");
Thread.dumpStack();
ComponentScatteredFeaturePieces.SwampHut componentscatteredfeaturepieces$swamphut = new ComponentScatteredFeaturePieces.SwampHut(random, chunkX * 16, chunkZ * 16);
this.components.add(componentscatteredfeaturepieces$swamphut);
}
else if (biomeIn != Biomes.DESERT && biomeIn != Biomes.DESERT_HILLS)
{
if (biomeIn == Biomes.ICE_PLAINS || biomeIn == Biomes.COLD_TAIGA)
{
ComponentScatteredFeaturePieces.Igloo componentscatteredfeaturepieces$igloo = new ComponentScatteredFeaturePieces.Igloo(random, chunkX * 16, chunkZ * 16);
this.components.add(componentscatteredfeaturepieces$igloo);
}
}
else
{
ComponentScatteredFeaturePieces.DesertPyramid componentscatteredfeaturepieces$desertpyramid = new ComponentScatteredFeaturePieces.DesertPyramid(random, chunkX * 16, chunkZ * 16);
this.components.add(componentscatteredfeaturepieces$desertpyramid);
}
}
else
{
ComponentScatteredFeaturePieces.JunglePyramid componentscatteredfeaturepieces$junglepyramid = new ComponentScatteredFeaturePieces.JunglePyramid(random, chunkX * 16, chunkZ * 16);
this.components.add(componentscatteredfeaturepieces$junglepyramid);
}
this.updateBoundingBox();
}"ComponentScatteredFeaturePieces.SwampHut componentscatteredfeaturepieces$swamphut = new ComponentScatteredFeaturePieces.SwampHut(random, chunkX * 16, chunkZ * 16);
this.components.add(componentscatteredfeaturepieces$swamphut);"
These 2 lines generate the Temple and hut and places them into the world. Then the Temple is wrapped around the hut at line "this.updateBoundingBox();".
Later in the code after the world is generated the hut is placed into the world at the topological terrain. The hut bounding box is then edited in Y height to match the average topological area based on the 4 corners offsetting its location based on the terrain its generated at. The Temple bounding box is not edited at this stage creating an offset. This can be seen in ComponentScatteredFeaturePieces.java in the method addComponentParts under the subclass SwampHut.
public boolean addComponentParts(World worldIn, Random randomIn, StructureBoundingBox structureBoundingBoxIn)
{
if (!this.offsetToAverageGroundLevel(worldIn, structureBoundingBoxIn, 0))
{
return false;
}
else
{
this.fillWithBlocks(worldIn, structureBoundingBoxIn, 1, 1, 1, 5, 1, 7, Blocks.PLANKS.getStateFromMeta(BlockPlanks.EnumType.SPRUCE.getMetadata()), Blocks.PLANKS.getStateFromMeta(BlockPlanks.EnumType.SPRUCE.getMetadata()), false);
---more code that was removed---"offsetToAverageGroundLevel" Edits the hut bounding box when placing it into the world. While the caller to generate and place the hut into the world is called from StructureStart.java line 70 in a method called "writeStructureComponentsToNBT" where the Temple is calling the different "components" to generate. After they are generated in the case of Temple structures unlike other structures the inner bounding boxes are altered in some instances.
TL.DR.
To fix this bug can be done by adding "updateBoundingBox();" method call inside the "writeStructureComponentsToNBT" funtion inside the class StructureStart.java at line 85 like the code example. This is to update the outer bounding box after the inner structure bounding boxes are edited before being saved to disk.
public NBTTagCompound writeStructureComponentsToNBT(int chunkX, int chunkZ)
{
NBTTagCompound nbttagcompound = new NBTTagCompound();
nbttagcompound.setString("id", MapGenStructureIO.getStructureStartName(this));
nbttagcompound.setInteger("ChunkX", chunkX);
nbttagcompound.setInteger("ChunkZ", chunkZ);
nbttagcompound.setTag("BB", this.boundingBox.toNBTTagIntArray());
NBTTagList nbttaglist = new NBTTagList();
for (StructureComponent structurecomponent : this.components)
{
nbttaglist.appendTag(structurecomponent.createStructureBaseNBT());
}
updateBoundingBox(); // add this line
nbttagcompound.setTag("Children", nbttaglist);
this.writeToNBT(nbttagcompound);
return nbttagcompound;
}A small optimization can also be done by removing most other updateBoundingBox(); calls in unneeded places as its done here in the critical location after the editing of inner bounding boxes are done.
Structure blocks have a bug rotating rails. The 90 and 270 degree rotation work fine but 180 degree have a bug where straight rails can rotate into a broken state when loading in a structure with straight rails. All other types of rails, curved or sloped rails can get into an odd state but when attempting to reload the structure a 2nd time the reload fixes the miss connected rails, all but the straight rails. Picture attached shows the bug were straight rails are saved into the structure and when the structure is rotated 180 degrees the rails don’t load correctly.
This is caused by rails having no set 180 degree rotation. The switch case lacks that rotation as it naturally doesn't have to rotate. But what happens is that the default rotation is chosen and the default rotation is the same rotation you place rails at. In some instances this angle is 90 degrees from the single placed rail causing the error. BlockRail.java, BlockRailDetector.java and BlockRailPowered.java all have the same exact method “withRotation”. This method lacks the 3 180 orentations that are related to straight rails. The switch statement fails and the rotation ends up setting the default rail orientation for the straight rails.
Code suggestion would be to move this method to the superclass and add the missing rotations to the 180 degree turn. Code suggestion for clarity.
public IBlockState withRotation(IBlockState state, Rotation rot)
{
switch (rot)
{
case CLOCKWISE_180:
switch ((BlockRailBase.EnumRailDirection)state.getValue(SHAPE))
{
// missing direction statement.
case NORTH_SOUTH:
return state.withProperty(SHAPE, BlockRailBase.EnumRailDirection.NORTH_SOUTH);
// and this one.
case EAST_WEST:
return state.withProperty(SHAPE, BlockRailBase.EnumRailDirection.EAST_WEST);
case ASCENDING_NORTH:
return state.withProperty(SHAPE, BlockRailBase.EnumRailDirection.ASCENDING_SOUTH);
case ASCENDING_SOUTH:
return state.withProperty(SHAPE, BlockRailBase.EnumRailDirection.ASCENDING_NORTH);
case SOUTH_EAST:
return state.withProperty(SHAPE, BlockRailBase.EnumRailDirection.NORTH_WEST);
case SOUTH_WEST:
return state.withProperty(SHAPE, BlockRailBase.EnumRailDirection.NORTH_EAST);
case NORTH_WEST:
return state.withProperty(SHAPE, BlockRailBase.EnumRailDirection.SOUTH_EAST);
case NORTH_EAST:
return state.withProperty(SHAPE, BlockRailBase.EnumRailDirection.SOUTH_WEST);
}
case COUNTERCLOCKWISE_90:
switch ((BlockRailBase.EnumRailDirection)state.getValue(SHAPE))
{
case ASCENDING_EAST:
return state.withProperty(SHAPE, BlockRailBase.EnumRailDirection.ASCENDING_NORTH);
case ASCENDING_WEST:
return state.withProperty(SHAPE, BlockRailBase.EnumRailDirection.ASCENDING_SOUTH);
case ASCENDING_NORTH:
return state.withProperty(SHAPE, BlockRailBase.EnumRailDirection.ASCENDING_WEST);
case ASCENDING_SOUTH:
return state.withProperty(SHAPE, BlockRailBase.EnumRailDirection.ASCENDING_EAST);
case SOUTH_EAST:
return state.withProperty(SHAPE, BlockRailBase.EnumRailDirection.NORTH_EAST);
case SOUTH_WEST:
return state.withProperty(SHAPE, BlockRailBase.EnumRailDirection.SOUTH_EAST);
case NORTH_WEST:
return state.withProperty(SHAPE, BlockRailBase.EnumRailDirection.SOUTH_WEST);
case NORTH_EAST:
return state.withProperty(SHAPE, BlockRailBase.EnumRailDirection.NORTH_WEST);
case NORTH_SOUTH:
return state.withProperty(SHAPE, BlockRailBase.EnumRailDirection.EAST_WEST);
case EAST_WEST:
return state.withProperty(SHAPE, BlockRailBase.EnumRailDirection.NORTH_SOUTH);
}
case CLOCKWISE_90:
switch ((BlockRailBase.EnumRailDirection)state.getValue(SHAPE))
{
case ASCENDING_EAST:
return state.withProperty(SHAPE, BlockRailBase.EnumRailDirection.ASCENDING_SOUTH);
case ASCENDING_WEST:
return state.withProperty(SHAPE, BlockRailBase.EnumRailDirection.ASCENDING_NORTH);
case ASCENDING_NORTH:
return state.withProperty(SHAPE, BlockRailBase.EnumRailDirection.ASCENDING_EAST);
case ASCENDING_SOUTH:
return state.withProperty(SHAPE, BlockRailBase.EnumRailDirection.ASCENDING_WEST);
case SOUTH_EAST:
return state.withProperty(SHAPE, BlockRailBase.EnumRailDirection.SOUTH_WEST);
case SOUTH_WEST:
return state.withProperty(SHAPE, BlockRailBase.EnumRailDirection.NORTH_WEST);
case NORTH_WEST:
return state.withProperty(SHAPE, BlockRailBase.EnumRailDirection.NORTH_EAST);
case NORTH_EAST:
return state.withProperty(SHAPE, BlockRailBase.EnumRailDirection.SOUTH_EAST);
case NORTH_SOUTH:
return state.withProperty(SHAPE, BlockRailBase.EnumRailDirection.EAST_WEST);
case EAST_WEST:
return state.withProperty(SHAPE, BlockRailBase.EnumRailDirection.NORTH_SOUTH);
}
default:
return state;
}
}I thought Grum confirmed fixing this bug as per the suggestion above. Can this be tested with a snapshot to see if its fixed. It's already fixed as far as I remember and if not the suggested fix is the least invasive until a complete rewrite of the netcode is needed.
While looking into the function "updateEntityWithOptionalForce" it might be worth noting a small optimization where the if boolean "forceUpdate" can be moved into the upper if check to improve entity optimization sightly.
From this
if (!(entityIn instanceof EntityPlayer))
{
int i = MathHelper.floor(entityIn.posX);
int j = MathHelper.floor(entityIn.posZ);
int k = 32;
if (forceUpdate && !this.isAreaLoaded(i - 32, 0, j - 32, i + 32, 0, j + 32, true))
{
return;
}
}To this
if (forceUpdate && !(entityIn instanceof EntityPlayer))
{
int i = MathHelper.floor(entityIn.posX);
int j = MathHelper.floor(entityIn.posZ);
int k = 32;
if (!this.isAreaLoaded(i - 32, 0, j - 32, i + 32, 0, j + 32, true))
{
return;
}
}The boolean can be done a few lines up as it is unnecessary to enter the upper if check when its going to skip the 2nd if check anyway.
Bug can be seen here by EDDxample.
https://www.youtube.com/watch?v=xuFLfSI43bI
To fix this bug fully as the code suggestion above was only for entity's that move themself into unloaded chunks. The issue with pistons is that pistons can also move entity's without sending an update on the entity. To fix that pistons need to notify the entity to send an update to the entity that is being moved.
Simply adding "world.updateEntityWithOptionalForce(entity, false);" to line 209 in class TileEntityPiston.java in the function "moveCollidedEntities" in addition to the fix suggested above.
Some entity's need to be fully updated to save there correct position and therefore move back to the old position before the piston move. An extra entity update would fix this but as the entity is clearly in an unloaded area it makes no sense to do so in this instance. This happens to Minecarts for example. This last noted bug is to minor and trivial as the entity is there but in the wrong position. Most other entity's are reloaded in the correct position including armorstands.
private void moveCollidedEntities(float p_184322_1_)
{
EnumFacing enumfacing = this.extending ? this.pistonFacing : this.pistonFacing.getOpposite();
double d0 = (double)(p_184322_1_ - this.progress);
List<AxisAlignedBB> list = Lists.<AxisAlignedBB>newArrayList();
this.func_190606_j().addCollisionBoxToList(this.world, BlockPos.ORIGIN, new AxisAlignedBB(BlockPos.ORIGIN), list, (Entity)null, true);
if (!list.isEmpty())
{
AxisAlignedBB axisalignedbb = this.func_190607_a(this.func_191515_a(list));
List<Entity> list1 = this.world.getEntitiesWithinAABBExcludingEntity((Entity)null, this.func_190610_a(axisalignedbb, enumfacing, d0).union(axisalignedbb));
if (!list1.isEmpty())
{
boolean flag = this.pistonState.getBlock() == Blocks.SLIME_BLOCK;
for (int i = 0; i < list1.size(); ++i)
{
Entity entity = list1.get(i);
if (entity.getPushReaction() != EnumPushReaction.IGNORE)
{
if (flag)
{
switch (enumfacing.getAxis())
{
case X:
entity.motionX = (double)enumfacing.getFrontOffsetX();
break;
case Y:
entity.motionY = (double)enumfacing.getFrontOffsetY();
break;
case Z:
entity.motionZ = (double)enumfacing.getFrontOffsetZ();
}
}
double d1 = 0.0D;
for (int j = 0; j < list.size(); ++j)
{
AxisAlignedBB axisalignedbb1 = this.func_190610_a(this.func_190607_a(list.get(j)), enumfacing, d0);
AxisAlignedBB axisalignedbb2 = entity.getEntityBoundingBox();
if (axisalignedbb1.intersectsWith(axisalignedbb2))
{
d1 = Math.max(d1, this.func_190612_a(axisalignedbb1, enumfacing, axisalignedbb2));
if (d1 >= d0)
{
break;
}
}
}
if (d1 > 0.0D)
{
d1 = Math.min(d1, d0) + 0.01D;
field_190613_i.set(enumfacing);
entity.moveEntity(MoverType.PISTON, d1 * (double)enumfacing.getFrontOffsetX(), d1 * (double)enumfacing.getFrontOffsetY(), d1 * (double)enumfacing.getFrontOffsetZ());
field_190613_i.set(null);
if (!this.extending && this.shouldHeadBeRendered)
{
this.func_190605_a(entity, enumfacing, d0);
}
// Add this update to the entity's that are being pushed.
world.updateEntityWithOptionalForce(entity, false);
}
}
}
}
}
}This bug is related to the way the entity's are processed. The entity code in World.java on line 1957: "updateEntityWithOptionalForce" causes this issue.
This is because entity's that are suddenly moved into a new chunk that is not loaded never get added to the unloaded chunk they have entered. They are removed from the list of entity's that are added to chunks and they stay in memory until the chunks around the entity are loaded to make it entity process. Then the entity is added to the chunk it is in. Because all data is stored per chunk basis it means that if entity's do not belong to any chunk they simply get unloaded along with the world without being saved to disk. This can happen to entity's that are moving faster then 32 blocks per game tick and other rare instances when pistons push entity's.
Detailed code explanation.
In World.java line 1959 in updateEntityWithOptionalForce this if check can be found
if (!(entityIn instanceof EntityPlayer))
{
int i = MathHelper.floor(entityIn.posX);
int j = MathHelper.floor(entityIn.posZ);
int k = 32;
if (forceUpdate && !this.isAreaLoaded(i - 32, 0, j - 32, i + 32, 0, j + 32, true))
{
return;
}
}This code allows for entity's to be skipped if there is a chunk within a 5x5 area around that is not loaded. The term entity processing is used to describe this.
Later in the same function at line 1987 the entity gets updated at the line "entityIn.onUpdate();" and it might have been moved at this point.
if (forceUpdate && entityIn.addedToChunk)
{
++entityIn.ticksExisted;
if (entityIn.isRiding())
{
entityIn.updateRidden();
}
else
{
entityIn.onUpdate();
}
}If the entity moves into a new chunk the entity gets removed from the old chunk and added to the next chunk it enters. This happens at line 2022.
if (!entityIn.addedToChunk || entityIn.chunkCoordX != l || entityIn.chunkCoordY != i1 || entityIn.chunkCoordZ != j1)
{
if (entityIn.addedToChunk && this.isChunkLoaded(entityIn.chunkCoordX, entityIn.chunkCoordZ, true))
{
this.getChunkFromChunkCoords(entityIn.chunkCoordX, entityIn.chunkCoordZ).removeEntityAtIndex(entityIn, entityIn.chunkCoordY);
}
if (!entityIn.setPositionNonDirty() && !this.isChunkLoaded(l, j1, true))
{
entityIn.addedToChunk = false;
}
else
{
this.getChunkFromChunkCoords(l, j1).addEntity(entityIn);
}
}This is where the bug shows up. at line 2039 an if check is done on the entity to make sure that the chunk the entity have moved into is loaded. If the entity have moved into a unloaded chunk it simply gets a boolean check "addedToChunk" as false without being added to the chunk it has entered and at the same time a few lines above it gets removed from the old chunk it was in.
At this point the entity gets stuck in memory as it dosen't have a 5x5 loaded chunk area around itself to become processing while it is not saved to any chunk and not able to get saved to disk. The exception to this is when the entity is teleported as in goes through portals or teleported by other means. This rare instance the "setPositionNonDirty" is set to true and the entity is added to the chunk it has entered. But this only happens when entity's are moved via teleportation.
This is probably not as intended because the entity can only be saved to disk and unloaded from memory if the player loads the area where the entity is in again and makes the entity become entity processing. Only then does the entity notice that it doesn't belong to any chunk and adds itself to the chunk it is in. The entity's that are moved into unloaded chunks can cause corruption and other issues when the world is shut down without being properly saved to disk. Mobs can be lost because of this. If entity's are not allowed to get saved to disk they also cause lag as they clog the memory by being added to list of loaded entity's without being able to get removed.
Suggested fix
As entity's can only be saved to disk and removed from memory when they are added to a particular chunk. It is suggested to simply add all entity's to the chunk they enter by removing the if check and explicitly add them to said chunk they enter. The only reason this check must have been done would have been to save some additional memory space prior to the "save-all chunks" every 900 game ticks. This must have been here to prevent chunks from becoming loaded prematurely. With the added 900 game tick "save-all chunks" added to the game makes this particular issue a non issue. Entity's can simply be added to chunks they enter and temporarily load the chunk to get saved to it. Later they can get unloaded and removed from memory while being saved to disk.
This code suggestion can simply be done to fix this bug.
if (!entityIn.addedToChunk || entityIn.chunkCoordX != l || entityIn.chunkCoordY != i1 || entityIn.chunkCoordZ != j1)
{
if (entityIn.addedToChunk && this.isChunkLoaded(entityIn.chunkCoordX, entityIn.chunkCoordZ, true))
{
this.getChunkFromChunkCoords(entityIn.chunkCoordX, entityIn.chunkCoordZ).removeEntityAtIndex(entityIn, entityIn.chunkCoordY);
}
// if (!entityIn.setPositionNonDirty() && !this.isChunkLoaded(l, j1, true))
// {
// entityIn.addedToChunk = false;
// }
// else
// {
this.getChunkFromChunkCoords(l, j1).addEntity(entityIn);
// }
}It is not harmful to the game to add an entity to an unloaded chunk. What will happen is that the chunk it is temporarily loaded and later unloaded by the 900 game tick "save-all chunk unloading" cycle. This suggested fix however dosesn't fix the issue that piston causes to entity's that are moved into the next chunk. The fix to make sure entity's get properly saved to disk when pushed by pistons needs a more complex fix that haven't been looked into. Most likely will need a better entity movement check done by pistons.
The behaviour to change all entity's controlling the minecart is quite simple. In EntityMinecart.java a simple if statement can be changed to make only players control the minecart.
line 545:
if (entity instanceof EntityLivingBase)
{
double d6 = (double)((EntityLivingBase)entity).field_191988_bg;
if (d6 > 0.0D)
{
double d7 = -Math.sin((double)(entity.rotationYaw * 0.017453292F));
double d8 = Math.cos((double)(entity.rotationYaw * 0.017453292F));
double d9 = this.motionX * this.motionX + this.motionZ * this.motionZ;
if (d9 < 0.01D)
{
this.motionX += d7 * 0.1D;
this.motionZ += d8 * 0.1D;
flag1 = false;
}
}
}changed to
if (entity instanceof EntityPlayer)
{
double d6 = (double)((EntityLivingBase)entity).field_191988_bg;
if (d6 > 0.0D)
{
double d7 = -Math.sin((double)(entity.rotationYaw * 0.017453292F));
double d8 = Math.cos((double)(entity.rotationYaw * 0.017453292F));
double d9 = this.motionX * this.motionX + this.motionZ * this.motionZ;
if (d9 < 0.01D)
{
this.motionX += d7 * 0.1D;
this.motionZ += d8 * 0.1D;
flag1 = false;
}
}
}Changing EntityLivingBase to EntityPlayer makes the minecart only be controllable by the player.
Bug or not it wasn't harmful at all and an endgame item used in a very niche mechanic that brought usefulness in a very balanced way. It added content and emergent gameplay in a creative and useful ways that made the game more valuable.
Please open this bug report so Mojang devs can at least see this and make that choice.
Me, Tim Miller, and Pokechu22 found the fix!
add ((EntityPlayerMP)entityIn).connection.captureCurrentPosition(); to Teleporter.java to line 186.
if (entityIn instanceof EntityPlayerMP)
{
((EntityPlayerMP)entityIn).connection.setPlayerLocation(d5, d6, d7, entityIn.rotationYaw, entityIn.rotationPitch);
((EntityPlayerMP)entityIn).connection.captureCurrentPosition();
}What is happening is that the player is being moved to the new portal location but as there is a speculated redundant code in the NetHandlerPlayServer.java function update().
public void update()
{
this.captureCurrentPosition();
this.playerEntity.onUpdateEntity();
this.playerEntity.setPositionAndRotation(this.firstGoodX, this.firstGoodY, this.firstGoodZ, this.playerEntity.rotationYaw, this.playerEntity.rotationPitch);captureCurrentPosition saves the location of the player. Then onUpdateEntity updates the players positions to teleport to the other dimensions portal location. Then the position of the player is set in setPositionAndRotation back to the older saved position in captureCurrentPosition creating a pink pong effect. Later the position is updated by the teleport confirmation making the player end up in the correct destination eventually. But this ping pong effect causes massive issues with players being placed in the wrong location in the correct dimension, both fire and chunk loading happens due to this and causes extra strain on the server when travelling through a portal as unneeded chunks are loaded. The simplest fix is to update the position via captureCurrentPosition in the place where the player is being teleported to i.e. the part of the Teleport.java code that updates the players position to the new portal exit position.
As a suggestion given above margin is suggested to be added in AxisAlignedBB.java both in “calculateXOffset” and “calculateZOffset” not in “calculateYOffset”. There might be edge cases where “calculateYOffset” might also benefit from this fix. As there are no side effects adding this tweak then I would suggest also adding it to the “calculateYOffset” as well giving all axis a simple margin fix. Mostly as there are edge cases where other types of entity's glitch through some other types of blocks in rare situations.
The sole reason to update all players with any block update is that there doesn't exist any code currently that can update a single block for a single player. There would need to be added 6-7 new methods going from World.java to ServerWorldEventHandler.java, PlayerChunkMap.java, PlayerChunkMapEntry.java. These classes would need additional functions just to handle updating a single targeted player regarding a block update. This would resolve the issue of not updating all players but it would honestly be a large rewrite that would probably be discarded because of the amount of changes.
There is no need to add heavy rewrite when the block updates are only sent to nearby players that can view the mined block in that same area around that block. There is often no more then 10-30 players at maximum in the same exact spot in minecraft getting at max 29 extra unneeded block updates. This update also only is sent out when blocks are not instant mined which also reduces the spam significantly. Adding the notification as shown is acceptable with the current architecture in place unless the number of players some day increases into the 100s in the same chunk. When and if that ever becomes a common practice then adding the tweaked optimized 100lines of code is a valid concern. But with the current state where at most 4-5 players view the same block and having the server send a few extra packages is a legitimately acceptable load. Carpet mod have been running this fix for a few months without noticing any performance changes at all on the server.
After MrGrim pointed out the flaw of all the current suggestions he helped improve the fix for ghost blocks. A simple modification to the BlockPistonBase.java is needed to remove the ghost blocks for both server and client. The fix is to simply check the block in front of the pistons that is being grabbed. If the block in front is a moving block 36 then all sticky pistons will ignore pulling any block on the client. This is achieved by adding an extra meta data in one of the parameters on the server and send it to the client. If said meta data contains the property that it can't pull the block in front then it will ignore any pulling of blocks resulting in a perfect synchronized behaviour to how the server behaves. This fixes the ghost blocks in the most efficient and lag friendly way with minimal intrusion to the current code.
These are the 2 code suggestions that shows the changes that was made to fix ghost blocks created by pistons.
This part of the code is for the server making sure to add a meta check if the pulling blocks aren't moving
https://i.imgur.com/FmMnjKX.png
This part of the code if for the client making sure the blocks in front can't be pulled if the server doesn't pull them.
https://i.imgur.com/7mPPFoS.png
As Markku pointed out. There are numerous reasons we chose to discard the NBT hitbox saving.
1. Issue of backwards compatibility.
2. Corrupted hitboxes would persist.
3. Any hitbox loaded would need to be corrected and as been in described in length because of the rounding artefacts would create an ambiguous state when a hitbox isn't at its correct size.
4. If hitboxes are corrected it could still result in the same overlapping walls problem that was started out with.
5. Extra storage for no reason when a simpler solution fixes the issue.
It was the best idea until we stumbled into the margin fix, after that it seamed natural to only add a margin with very little drawback instead of saving the hitbox.
The proposed fix is to simply change the size of the hitbox based on the centre position. Then check if the entity is overlapping any other hitbox. If its overlapping any other hitbox it should move the entity back out but only by first checking if its not overlapping with the old hitbox (the last check is to make sure that entity's aren't moved out of blocks they were previously occupying).
These two images shows a code proposal.
https://i.imgur.com/8sVcGr2.png
and
https://i.imgur.com/tdyxPtR.png
Pokechu22 helped point out the code where mobs are resized. It happens in EntityAgeable.java in "setScale" (MCP code). This line is called when a mob changes size to an adult. This then calls a method "setSize" (MCP code) in Entity.java where the size is changed. Studying the code it shows clearly that the resizing is done in a very odd fashion.
If the resizing is done towards smaller then the entity simply shrinks without problems keeping the centre position as the reference point, no problems observed. If it's done towards expanding then its done differently and is the reason this problem happens. The expansion strictly expands the current hitbox towards the positive axies as shown here
this.setEntityBoundingBox(new AxisAlignedBB(axisalignedbb.minX, axisalignedbb.minY, axisalignedbb.minZ, axisalignedbb.minX + (double)this.width, axisalignedbb.minY + (double)this.height, axisalignedbb.minZ + (double)this.width));"axisalignedbb.minX + (double)this.width". After the hitbox is resized its moved back by the total width amount.
this.moveEntity(MoverType.SELF, (double)(f - this.width), 0.0D, (double)(f - this.width));.Basically moved back by a full expanded amount.
Two observed issues that can be seen by this is
A. The mob expands and shifts its original position.
B. As illustrated in the picture some edge cases results in mobs never being moved properly back and placing them inside other blocks.
https://i.imgur.com/DpP41Mf.png
This bug shows up due to there being an old player entity object added to a chunk list. The chunk list then gets flushed along with all other entity's when the chunk is written to disk. The entity's have there entity trackers removed along with being removed from the world. Given the incorrect player object is part of a chunk that is written to disk the player tracker object gets removed as well.
This bug may show up in other instances but the main one found using nether portals due to accurate reproducibility is nether portals. Using vanilla 1.12.2 following the procedure of going from over world using a nether portal into the nether then back into over world several times and then waiting for the auto-save reproduces this bug accurately.
A global fix would be to never remove the player tracker object by removing the player object in chunk lists. This happens in World.updateEntities MCP mappings.
This is where the flushing of player objects happens removing the player trackers along with the false player objects.
Given this bug was tested on portals the bug can easily be seen in the portal code in PlayerList.transferEntityToWorld MCP mapping names. This method is responsible for removing the player from the old dimension and placing the player in the new dimension in the correct chunk. But player is placed inside a chunk with the method call:
Specifically the lines:
This method adds the entity into the specific sub chunk list but never removes the player object. Given that the sub chunk has a lingering player object added to its list that then gets flushed along with the chunk being written to disk.
MrGrim suggested a clean fix for the specific case of the nether portals. But given there might be other instances of the player writing itself to a chunk and never having it removed then it might be prudent to make a global fix instead of individual fixes in the code.
MrGrims nether portal code fix in PlayerList.transferEntityToWorld:
As seen in the code suggestion above the player is simply removed from the chunk it belongs to in the old world it belonged to before adding it to the new world. Given portals lack this removal the tracker object can then be deleted if the player enters the same dimension before the chunk is written to disk having an old player object in a random chunk list in the same dimension.