Chunk-wise entity lists often don't get updated correctly (Entities disappear)

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);
                        }
                    }
                }
            }
        }
    }

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.

Would it make sense to rather directly update the chunk entity lists as soon as the position of an entity changes? Otherwise MC-89667 would remain unfixed and require a different solution.

if your having issues with /tp command go into the code and apply this fix needs an if statement around it though.
https://bugs.mojang.com/browse/MC-129685?focusedCommentId=452773&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-452773

I looked at your code and that code should be cleaned up there but, spawning the entity into the world has nothing to do with the /tp command issue the issue is the chunk isn't loaded and the entity ins't added to the chunk so it never saves.

Yes forceUpdate is a bit buggy based on what I seen the area isn't loaded and force update is true yet it still returns there is probably a reason for this though entities if the chunks are not loaded shouldn't be allowed to update depending on intent of coder and method. The proper solution is to load the chunk first if not loaded and add the entity to that chunk

I haven't debugged this enough perhaps it occurs in loaded chunks to but, the player keeps the chunks loaded and the entity isn't added to the chunk but, still saves. I don't know maybe it does add it to the chunk if it's loaded need more debugging.

"Would it make sense to rather directly update the chunk entity lists as soon as the position of an entity changes? Otherwise MC-89667 would remain unfixed and require a different solution."

Yes editing all the entity.setLocation/positions if chunk detected has been changed it would make sense to remove the entity from that chunk and add it to the new one. This would be more of a proper fix for everything. Then the entity selectors from the other issue mentioned here should work as well if not they should be able to easily wip up a fix

Could have sworn they made this bug fix a long time ago like in beta guess they must have reverted their code since jeb took over

@unknown Thanks for your input, just one thing:

All your edits get sent to us watchers into their emails, so each time you change anything in your post, we get notified via email, which is a bit too much if there's loads of edits 🙂

So far I (and all the others) have gotten 9 notifications from you on this bugpost, in the past 40 minutes.

I usually pre-write comments and let it sit for a few minutes or longer, think about it, to make sure I won't have to add or delete too much off it again, or if so, only once; but that's just a suggestion for you, maybe you didn't know about the notification thing 🙂

This is a bit offtopic, but when editing this post, the links in it got removed.
It happens in Visual mode.

Reddit post.

interesting fact the entity never gets added to the chunk in the same tick even if the chunk is loaded so here is my fix for all the related issues for the new /tp doTeleport() Command:
https://gist.github.com/jredfox/86630098b66b6f43978c8e103fd4db02

You might ask well why provide chunk? If it's loaded it gets from cached if it's unloaded it helps fix the issue and regardless the entity isn't added to the chunk so the rest of the code forces it to do so. 

I am going to say player transportation is also not handled correctly from world server chunk map if player loads to new chunks but, I don't know really how to fix it so I am going to let mojang make a EntityPlayer patch during the doTeleport() since updating the next tick isn't how it should be handled. It's unkown to me if player chunk map is for all players in all dimensions or just one dimension? That's why I am letting mojang fix the patch for players I fixed the regular entities though.

Note this doesn't support cross dimensional teleport if you want that you can take a look at CMDDim "/tpdim" for evil notch lib as it fully supports it. I can't take all the credit brandon's core order to do what helped out alot and fixed it up a bit. Note it's still wip for regular entities since I disagree with brandon's core on a couple hard coded things.

If they apply the fix above they should also fix the detection for wrong entity location since if an entity enters a new chunk at a certain location it will freak out and kill the entire mob stack saying wrong entity location. I am getting around to this by saying it's better that the vanilla bug of the mob not being there till next tick is better then the entire mob stack randomly dying when entering a new chunk. 

It's currently (1.13pre1) hard to confirm because the positioned commands fail to find the entity every time.
I also tried this with no success:

execute positioned 0 100 0 run tp @e[tag=MC-108469,distance=10] 100 100 0

There might be another bug here or I might just not see through the new command system.
So the way to reproduce needs fixing.

With distance=10 you'll be looking for an entity that is only exactly 10 blocks away from the position, rather than anywhere between the position and 10 blocks. You'll have to specify a range, which uses ".." as the separator:

execute positioned 0 100 0 run tp @e[tag=MC-108469,distance=..10] 100 0

(where x..y would mean "between x and y", while no value in either position would mean absolute minimum/maximum).

Thank you Mr. W 🙂
I already suspected that it was just some change I didn't know yet.
The commands in the post are updated to the new system now.

Things make so much more sense now.

I always assumed that this was MC-119971 alone. But instead, there is an ugly interaction between these two bugs. MC-119971 is the situation where a chunk is reloaded while it is in the midst of being zlib compressed before being saved to disk. During that time, the chunk is not listed anywhere that the main thread can know about, so an old version gets loaded. This results in deletions and duplications at chunk boundaries (blocks and entities being pushed by pistons, item deletion and duplication in hoppers).

But seeing MC-108469, things make so much more sense. I've seen it many times where a minecart gets inexplicably stuck at some point along a track, even on a powered rail. I eventually found that if you break the block beneath the rail that seems to have an invisible boundary, another minecart will materialize and fall into the hole.

It seems like MC-119971 (fix available) is responsible for making the duplicate, while MC-108469 is responsible for making the invisible one.

To add an alternative fix – I realized a patch I added in https://bugs.mojang.com/browse/MC-136995 requires a field CraftBukkit added, to retain if the entity is in the world or not, which I used to fix this bug in Paper:

protected void onEntityAdded(Entity entityIn)
    {
        for (int i = 0; i < this.eventListeners.size(); ++i)
        {
            ((IWorldEventListener)this.eventListeners.get(i)).onEntityAdded(entityIn);
        }
        entityIn.valid = true;
    }

    protected void onEntityRemoved(Entity entityIn)
    {
        for (int i = 0; i < this.eventListeners.size(); ++i)
        {
            ((IWorldEventListener)this.eventListeners.get(i)).onEntityRemoved(entityIn);
        }
        entityIn.valid = false;
    }

Then this code only needs a very minor change:

if (!entityIn.valid && !entityIn.setPositionNonDirty() && !this.isChunkLoaded(l, j1, true)) // add valid check at start
            {
                entityIn.addedToChunk = false;
            }
            else
            {
                this.getChunkFromChunkCoords(l, j1).addEntity(entityIn); // If added to the world, it will always be added to the chunk
            }

The downside to Xcom's fix is that dimension teleporting will trigger chunk loads to an unnecessary chunk as it first updates the position of the entity before snapshotting it to copy into the new dimension, and that would trigger the chunk to load.

however, the entity being removed from the world before its moved would set valid=false and then not load the chunk with my supplied version.

MC-98153 was already fixed in 13 though. The bug where entity's would pingpong using portals.

Confirmed for 19w04a, took me ages to figure out it was this bug that caused my system to fail

The steps to reproduce only partially work in 19w12b due to MC-146470.
/say @e[tag=MC-108469]
This command will not be able to detect the entity now, but the warning message after reloading the world persists, and I was still able to delete the entity.

Since MC-146470 is likely to be fixed soon, I will not change the description.

I cannot reproduce the second half of step 7, as @unknown said, but also not step 8. But the armour stand is actually gone after all these steps. Are there new reliable reproduction steps for the commands part?

@unknown I added new commands, however the entity selector based reproduction should be taken with a grain of salt, as it's highly dependant how entity selectors are optimized. So it might change between versions or even be affected by things like how many entities are loaded etc.
The warning on reloading has been consistently there and likely will keep working and thus should be the preferred reproduction.

I can reproduce it with those commands in 1.15-pre1.

I can't seem to reproduce this in 1.16