Kademlia

Confirmed for 1.13-pre1 see latest image

Hey,

thanks for the clarifications. As stated I am not against the idea or try to argue for the NBT-Saving idea. I just want to make sure the given reasons are valid reasons for the argument here. Lets look at the given list:

1. Issue of backwards compatibility.
I would disagree that this would be a factor talking about Minecraft.

2. Corrupted hitboxes would persist.
I would disagree with this also. The reason being that hitboxes do not 'persist'. They area changed on each tick to represent the given position and dimension of an entity. Thus always being corrected. The only reason for saving the AABB in the NBT data is because the code-flow is broken by saving/loading entities. You could probably even not save the AABB and just re-set the initial position of the entity later in the NBT-Loading method to get the same working flow again - calling .setPosition and re-calculating the AABB in the NBTLoad-Method is whats breaking said flow.

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.
As far as i can tell this is answered with 2.? Feel free to correct me.

4. If hitboxes are corrected it could still result in the same overlapping walls problem that was started out with.
I do unfortunately not understand to what you are referring here with 'hitboxes are corrected'

5. Extra storage for no reason when a simpler solution fixes the issue.
I agree that additional storage is to be prevented but don't think its a strong point considering the amount. And I´d argue the use of 'simpler' in the sentence strongly 😉

(6.) One of the initial arguments as I understood it is that the margin-Solution would fix changes to the Entity-Dimension made by the developers (See the quote in my last comment). As far as I can tell from your explanation this would definitely not be the case based on the defined margin-Size. If you ONLY talk about changes made to the code-flow and recalculation of position/hitboxes etc. then I´d agree.

As far as I can tell the main difference in our understanding of the code (and feel free to correct me here) is that you consider hitboxes to be fixed values in the entity while I am of the impression that they are only used as dynamically created temporary values that change each tick anyway.

Do you have a explanation what needs to actually be changed in the code to make a working build similar to my reddit-post linked above?

(@Timothy regardless of whether or not bugs get fixed I don't think you are helping yourself with the off-topic parts of the comment)

It is not sufficient to just include the hitbox in the NBT data, because sometimes standard hitbox sizes change due to bug fixes and other reasons.

While I agree with most of what you said I do not agree with this conclusion:
If a 'standard hitbox sizes change due to bug fixes' the change of said hitbox will most likely not be in the range of your proposed margin of

final double margin = 1.0 / (1L<<27);

but rather bigger than 1.0 / (1L<<6) making the solution for said problem still fail in the future, it would more ore less be the problem of growing.

("Other reasons" would have to be specified to be a valid point.)

For growing up and NBT saving see also: https://www.reddit.com/r/Mojira/comments/6tf9n7/insight_into_the_bug_fix_decisionmaking_process/dlkyqkn/

See https://hub.spigotmc.org/jira/browse/SPIGOT-1097

worldserver.flushSave(); is called by CommandSaveAll and results in a syncronous call to the mentioned method.
This can result in the creation of two iterators which are not thread safe even when using a ConcurrentHashMap and can result in at least a NPE on .next().

@Killerbeenl ( @Marcono1234 )
Regarding the PC-Version (I have never used the MCCE):
While this is certainly a possibility you are re-loading the world and thus the mentioned precision-problem (first mentioned here AFAIK; not by me: http://bugs.mojang.com/browse/MC-2025?focusedCommentId=74619) is sufficient to cause the problem. So I´d argue this does not indicate another problem on its own.
As far as I know vanilla MC always looks up the blocktype when moving entities and sync-loading needed chunks. But its hard to tell as servers generally pause entities on bordering chunks to keep the server from constantly loading/unloading "border+1"-chunks.

Apart from the mentioned precision-problem I fixed another related bug on my servers. This was mentioned in the comments here as well some years ago: The BoundingBox of an Entity that is changing its state from Baby to "GrownUp" increases and can result in entities moving out of fenced areas if the entity was standing directly at the fence while growing up. This can be fixed by relocating the entity in the event of growing up (my ugly code to fix this: https://hastebin.com/socaxakiko.cpp).

Since adding those two additions no player (on the servers) was able to reproduce the problem but as stated above on servers entities near the border of the loaded world wont move.

As far as i can tell this was fixed with https://bugs.mojang.com/browse/MC-46345
Every version above 16w42a should be fixed currently. Thus 1.10.2 should still have the problem.

MC-46345 was marked as fixed, if this is actually the case (no version released yet) this should fix MC-80782 and MC-98707 too.
If MC-46345 somehow does not fix the other two the severity of the memoryleak may increase greatly.

@Marcono1234
Teleporting is enough to produce the problem in vanilla. You can use the Test-Map in this Report to reproduce the problem with vanilla within one or two teleports.
The difference is instantly visible (Fixed spigot first, spigot normal at 30sec): https://youtu.be/zj1Y43Buyik

The question if it 'proves it works' is harder of course. But as this only happens on chunk-reloading an not in normal gameplay this problem must be generated by this entity loading mechanism. I compared all Entity fields at runtime that either changed trough saving/loading to/from NBT or are never saved.

Hey,
I would still argue there is a .setPosition() in your methodflow. Try adding it right before the end of the catch block.

roundoff: Yes that is the definition of a numeric precision problem. The amount of difference is not the point. Example:
0.00000000000001 to -0.0000000000001 would completely change your program logic.

The same goes in this case.
An Entity standing somewhere at 0.5XXXX
and a BoundingBox up to 0.62499999999999998
Will be rounded to 0.625 or higher. Thus changing the actual block-location of the bounding-box test code.

In this case 0.62499999999999998 would be the maximum to not get treated as "standing on Block 1" (possibly blocktype dependent)
By rounding this up to 0.625 on re-initialisation of the BoundingBox the Entity will now be treated as "standing on Block 1" before it was only "standing on Block 0"

Updated Video with the mentioned Test-Map and latest Build: https://www.youtube.com/watch?v=-m2zcTQJ7gk

If it still happens to you if the loading-code is at the end of the method I would bet on a CPU type dependency (fastmath, SSE...) but that would really surprise me.

@md_5 cannot reproduce with that map within ~5minutes of teleporting.
Im guessing you did not have the exact order?
setPositon(...) in the loading method MUST be called before loading the AABB data - not the other way around.

Hey,

I recently found and fixed this bug. This is caused by the numeric precision of float/double and is a round-off error caused by the re-initialisation of an entities bounding box when loading the entity from its NBT-Data.

An example-fix would be to save the current bounding box of an entity to the NBT Tag. But there are other ways to fix this too.
Video of the Problem in SP and the working solution on my server: https://youtu.be/jSJGQAGSmPo

Entity.java

public NBTTagCompound e(NBTTagCompound nbttagcompound) {
        try {
            nbttagcompound.set("Pos", this.a(new double[] { this.locX, this.locY, this.locZ}));
            
            // kade this fixes the fences bug
            nbttagcompound.set("AABB", this.a(new double[] { this.boundingBox.a, this.boundingBox.b,this.boundingBox.c,this.boundingBox.d,this.boundingBox.e,this.boundingBox.f}) );
 [...]
}


[...]



    public void f(NBTTagCompound nbttagcompound) {
        try {
[...]   
            this.setPosition(this.locX, this.locY, this.locZ); // kade this causes the problem
[...]   
            if(nbttagcompound.hasKey("AABB")){            
	            NBTTagList aabb = nbttagcompound.getList("AABB", 6); //kade edit this fixes the fences bug
	            boundingBox = new AxisAlignedBB(aabb.e(0),aabb.e(1),aabb.e(2),aabb.e(3),aabb.e(4),aabb.e(5));
            }
[...]   
      }

Possibly caused by MC-10976. The bug described there was already present in minecraft 1.4.x

As mentioned in point 4, yes it is.
Edit: I´d argue a new topic with all information is worth it, as this is neither only a single-player problem as the title suggests, nor does this only happen when quitting the game.

I understand what you are trying to say but this is still not very helpful to the ticket:

Your viewing the network/memory cost from the wrong side. By default all Sections are transferred (100% usage). Sometimes there are sections not transferred because of this faulty code (they should be anyways and everyone expects them to!). If you take typical values, lets say a viewdistance of 6 and and 5 sections each chunk. Thats 169 Chunks or 845 Sections. A missing section because of this bug is about 0,1%. This concern is not justified and even a recommendation to implement something to save 0,1% is a pretty hard sell.

I do not understand why you wrote this.

• MCP Naming is not a concern here

• The network cost is already explained in the conclusion. Your elaboration adds nothing new.

• Your remark regarding a null-Section of a chunk is wrong. The code is built around identification of null-Sections.

• Your recommendation adds work on the server, the client and can have complicated new validation cases. For a problem of this magnitude this is absolutely over engineering. I have already stated that these changes are validated and working for over two years now. There is no need to complicate the issue buy adding unnecessary additional work.

• The whole idea behind the isEmpty-Check is faulty. It only hides information. Removing it is the ideal solution.

Confirmed 1.10; same seed same coordinates

Confirmed for 1.10 😞

Hey, thank you for responding. Showing this via a worldmap would be quite difficult. I hope relating to the code helps:

This is based on the currently available minecraft_server.1.9.4.jar (and deopfuscated spigot codes)
Obfuscated:
Class: aht.java
Method (~Line 426): public int c(cl \u2603, boolean \u2603)

Deobfuscated:
Class: World.java
Method ( ~Line: 623): public int c(BlockPosition blockposition, boolean flag)

This method would be something like getMaxLightLevel(Position position, boolean firstCall).
The method will try to get the highest light level. It returns the light-level of the block itself if it has one (air/non-solid block)
or the highest of the five closest blocks (up,north,east,south,west). This second part has the problematic code.

Methodflow should be:

• Return 15 (maxLight) if invalid position

• if(firstCall && getBlockType(position).isSolid())

  • recursive call to self with neighbor1-block and firstCall = false

  • recursive call to self with neighbor2-block and firstCall = false

  • recursive call to self with neighbor3-block and firstCall = false

  • recursive call to self with neighbor4-block and firstCall = false

  • recursive call to self with neighbor5-block and firstCall = false

  • return highestValue of those calls

• *else*

  • *Chunk c = getChunkAt(position)* (this is called for all 5 recursive calls from above and results in a forced-chunk-load if the chunk is not yet loaded)

  • return c.getLightLevel(position)

Chunkloads based on this code will happen every time the growth-algorithm tries to grow a plant at the border of a players visible (loaded) area.

Adding something like this code before the highlighted chunk-loading would fix this

if(!isLoaded(position)){ //kade edit; do not load a chunk just to look at the light-value
    	return 0;
   }

On a side-note the same happens when trying to spread grass, mushrooms, mycel, cocoa etc.
In these files there are checks to get the blocktypes of the surrounding blocks and try to spread these blocks afterwards.
Those calls force sync-chunkloads too.

Let me know if there is something missing or additional information are needed.

Sorry I dont understand you comment. None of your reports are even related to client-side memoryleaks and maps. For this Bug it does not matter what server you join. Just join random Servers with maps for some time and your client will crash at some point.

A player in a team with collision disabled
(/scoreboard teams option a collisionRule pushOwnTeam)
will still be able to push teammates if riding a (horse) vehicle.
This seems to be a result of the client-sided logic of the player being pushed.