Michael Kreitzer
- entrigant
- entrigant
- America/Indiana/Indianapolis
- Yes
- No
MC-9568
This could be used maliciously and may warrant a higher priority. A video example can be found on PearlescentMoon's first Hermitcraft episode at timestamp 25:40: https://youtu.be/HIVv5TLrf8g?t=1540
With very little effort any user can severely lag even a high powered server such as that one. It's definitely not just client side. In other Hermitcraft videos you can see chat messages such as "who grew a tree?!", and I do believe there have been some lag related deaths.
OK, I should have probably done this first, but I'm having a hell of a time re-creating this. I've tried with single player and on a vanilla server/client. Even tried with some of the server side mods I know Hermitcraft uses. After seeing reports I wasn't willing to upgrade my own server to find out, but I wanted to be sure.
Does anyone have reproduction steps or a test world with this problem?
The bug
I was fighting with guardians and the fight is not how it was before - guardian beam rendering stopped working. Only bubbles render now.
This happens when the game time reaches 2800000.
Download the MC-165595 world attachment if you want to reproduce this.
Code analysis
Code analysis by Michael Kreitzer can be found in this comment.
![[Mod] ampolive](../../../data/avatars/users/18949.svg){kind=small}
As mentioned by Michael Kreitzer, the reason this issue occurs is because of an integer overflow, caused by float to int conversions inside Mth.cos() and Mth.sin(). More specifically, it should overflow when the world time is above approximately 2745160 (might be slightly more or less than that), as that is the earliest number that would cause an overflow within GuardianRenderer#render(...):
... float $$8 = (float)$$0.level.getGameTime() + $$2; ... float $$18 = $$8 * 0.05f * -1.5f; ... float $$31 = Mth.cos($$18 + 5.4977875f) * 0.282f; ... GuardianRenderer.vertex($$46, $$48, $$49, $$31, $$41, $$32, $$20, $$21, $$22, 1.0f, $$50); ...
I had some issues with escapees syill when trying out the code provided by Xcom. I've built on his idea and come up with the attached. The screenshot shows a pen with a mix of iron bars, cobblestone walls, and full blocks in a ring shape, and all of the cows in the pen started as baby variants. I've managed to reproduce this a few times with various combinations of blocks without any animals escaping thus far.
The changed code first sends "null" as the first argument to "this.world.getCollisionBoxes" to prevent it sending nearby entity collision boxes. After that it walks through the list of all block collision boxes that intersect the new entity collision box that did not intersect the old one. It then checks the X and Z axis independently, and if the new collision box collides with the block collision box it moves the new entity collision box in the direction opposite of the relative position to the old entity collision box on that axis only if the old entity collision box completely clears the block collision box on that axis.
It's a bit of a mouthful, but it seems to work.
MC-9568 Proposal.txt
"This was clearly a bug." - So was rocket jumping. What's your point? It was also a source of emergent gameplay that didn't cause adverse effects. It's unintentional nature doesn't invalidate it as a mechanic, and it should at least be discussed as a candidate for preserving before being blithely written off.
This is not a suggestion or a request for a change. This is a report of a prior emergent mechanic that longer functions, something one would expect to be tracked in an issue tracker. Intended or not, it is a mechanic in current stable releases that does not function in current snapshots, and its future as a mechanic should have its due course in the appropriate forum. This will be my final statement on the matter, but please reconsider the reactionary summary judgement applied here. A mechanics origin shouldn't dictate its fate.
I would like to provide some evidence of the intended nature of the block dropping behavior in case there is some confusion there. Here is the relevant section of code from 1.12.2:
Inside the large if statement are two others. The first checks if the piston has finished extending or not by looking for block 36 2 blocks ahead of the direction the piston is facing. If it find block36, it "drops" the block there (the call to "clearPistonTileEntity" does this). It then sets a flag that causes the next if statement to evaluate false, and that statement is the one that "pulls" the block with the call to "doMove".
The code is very explicitly and intentionally written to drop blocks. It's not accidental as somebody had to expend effort to put this there.
And on Nov 10th, 2015 _jeb marked
MC-5726WAI. I guess he changed his mind, but it has been codified in the games source code since then in a very explicit and intentional way.It's not the applications role to make assumptions about the capabilities of the display hardware. If Minecraft takes an interest in color management it should respect the color profile installed by the OS by not resetting it or changing it. Many games do this, and there's even dedicated software to detect when games do this and set it back. There's an excellent write up about this here: https://forums.guru3d.com/threads/games-color-profiles.387074/
At the very most Minecraft should detect when alternate color spaces are reported by the OS such as Rec. 2020 when rendering to a display device compliant with HDR10. It can then adjust the texture colors so that they are displayed properly assuming the textures target something like sRGB.
I think I have an idea what might have happened. In the sticky piston retraction code there is a big nasty if statement for determining if the piston can pull the block or structure in front of it. I think it originally looked like this:
if (!movingState.isAir() && isPushable(movingState, level, twoPos, direction.getOpposite(), false, direction) && (movingState.getPistonPushReaction() == PushReaction.NORMAL || movingBlock == Blocks.PISTON || movingBlock == Blocks.STICKY_PISTON))
While Grum was working on
Something probably got missed. The fix is probably a simple one.
MC-54026, I recall him expressing frustration at how unwieldy that if statement is in EigenCraft. I suspect he tried to refactor it.I found the comment I was remembering:
Removing the following check would produce the behavior seen (based on 1.12.2 code):
isPushable(movingState, level, twoPos, direction.getOpposite(), false, direction)
Later on in the moveBlocks function a similar, but not quite the same check, is done. I have to use MCP naming for the rest of this. In MCP the moveBlocks function is named canPush.
BlockPistonStructureHelper blockpistonstructurehelper = new BlockPistonStructureHelper(worldIn, pos, direction, extending); if (!blockpistonstructurehelper.canMove()) { return false; }The call to blockpistonstructurehelper.canMove() contains:
if (!BlockPistonBase.canPush(iblockstate, this.world, this.blockToMove, this.moveDirection, false, this.moveDirection)) { if (iblockstate.getMobilityFlag() == EnumPushReaction.DESTROY) { this.toDestroy.add(this.blockToMove); return true; } else { return false; } }You can see that this check is slightly different. The first direction parameter is not reversed, which is what causes the glazed terracotta block to be pulled, and if a block is marked as a destructable block the function allows the move and add the block to the list of blocks to break.
This helper function is specifically designed for pushing and not retracting. Adding the original check back to the if statement should be all that is needed to fix this bug.
Unfortunately this is only partially resolved. "Destructable" blocks are fixed, but sticky pistons are still pulling glazed terracotta blocks. Can this be re-opened?
In an idealized and, most importantly, mutable, potential intended future state where 108, 8328, 123217, 122911, and all other bugs have been resolved or had suitable alternative mechanics implemented, then this bug might be irrelevant. If that's the case, then the code that causes this "block teleportation" would be redundant anyway. However, we're not in this ideal future state, and this change in behavior breaks many builds.
It was my understanding, and even my experience working with Grum on trying to maintain sticky piston block event ordering, that the devs were committed to not breaking builds and maintaining 1.12 behavior as much as possible. If that is still true, then this should be looked at. The fix is probably not difficult. It's probably just a case of wrapping the line of code that clears the block entity in an "if (isSticky)" statement.
What's even more notable is that the clearing of that block entity probably exists intentionally to maintain the behavior of 122911, which, if you recall, was restored based on the same commitment to maintaining compatibility and not breaking builds. Even though Grum really dislikes that bug, he went out of his way to preserve it for us. Please do the same here.
I think I understand. Keeping track of all of these inter-related things is quite the task. In that case, fixing this bug would be the most correct behavior possible now. The "teleportation" that is in place now would be the bug. Allow me to explain.
That this happens with sticky pistons is one aspect of the 122911 bug. Technically, it could be coded so that the block drop in 122911 happens without the "teleportation" aspect, but the way it presents in the code is that the act of teleporting the block is what sets the flag to drop the block in 1.12. So, both behaviors have always been discussed together and never separately. Both behaviors were removed early in the snapshots for 1.13, but Grum agreed to restore both behaviors shortly after.
So, for a sticky piston when a redstone pulse is cut off before the extension completes the following happens:
In 1.12, a regular piston doesn't perform the last two steps. In 1.13, the 3rd step, however, is being performed. This is what breaks existing builds.
If all of the other mentioned bugs were fixed, then the case of what to do when a piston is "short pulsed" would simply never come up. The clearing of the piston head Block Entity can be considered a work around for this special case that can happen now, and it's the bare minimum needed to handle it. So, asking for an example of a behavior change that doesn't depend on short pulses is an impossible task.
Therefore, there's no reason for this to block any other bugs. Restoring 1.12 behavior, in this case, is not something that needs to be undone later down the line.
I hope that helps clarify.
I found a cause and a fix for Panda's test world. However, it doesn't seem to cover all cases.
While walking the visibility graph it won't add a neighbor subchunk if there's no visibility path from the direction the algorithm entered the subchunk to the direction of the neighboring subchunk. When it got to the chunk with the iron blocks from the subchunk below the player (which is the 2nd subchunk processed) while there was no lapis block, it would end there. There's no path to any neighbor due to the solid wall of iron blocks at the direction the path "entered".
The only other paths to the disappearing subchunks are the subchunks to the right and left of the player. The problem is, the algorithm was setup in such a way that once a path moves in a certain direction, it could never go back in the opposite direction, ever. So, take e.g. the left. From the player it would go left (now it can never go right again), down (now it can never go up again), forward (now it can never go back again), forward, and forward. So, in this entire scene with all subchunks above the bottom two out of the Frustum, there's no valid path to those two subchunks that disappear.
When you placed a block to fill the lapis wall, it blocks the down, forward path. Now, the path is straightforward; forward, down, forward, foward, forward, and it spreads from there.
If you clear the inherited direction restriction, it clears up this test case nicely, and in some brief real world testing it seemed to cause no significant increase in subchunks rendered or any appreciable drops in FPS. I'm attaching a screenshot from MCP 9.42 (I know.. old.. sorry I do not have anything newer setup). I renamed a lot of the variables to help make things clearer.
Now, as for the test case that still fails:
-1288030513686422296
/tp -254 81 480
(edit: this seed came from a 1.12.2 test world.. if somebody with a more current dev environment can confirm any of this it'd be much appreciated)
I'm going to try to find a synthetic/controlled case for this one. I suspect that "bidirectional" path requirement is related, and possibly unnecessary. I'll play with the idea of neighbors only caring if they can see each other.
In the second test case the problem is caused because the chunk above and to the south of the one that disappears are the only neighbor chunks that can "see" it. With the visibility check checking the path from the source face to the destination face, then:
All other possible paths will fail the visibility check.
Removing the sticky reverse path and replacing the source to target visibility check essentially means you end up with every chunk in the frustum that is in some way connected to the camera chunk being rendered. This does mean any cave with an entrance in the view frustum will be rendered and the scanner ends up scanning a lot more neighbors in some situations. However, the typical situation of looking out to the horizon doesn't really change and is always the most expensive.
All in all, this algorithm just doesn't work. It's results were always haphazard and it's lucky it hasn't caused any more problems than it has. It's trying to do visibility checking without taking the camera into account. At best you exclude a few caves that have no entrances or paths to anything in the frustum or a few chunks behind a mountain that occupies the entire width of the frustum.. maybe.
I hope this is intended to be replaced by Blaze3D.
Last comment.. hopefully.
Keeping the "sticky reverse path" works if you only check if any path exists to the target face. That helps bring the checks down a bit. The performance hit isn't nearly as bad as I feared.
I did this analysis on 1.12.2, but I imagine the cause remains the same. This issue appears to happen when a regular tower generates on top of a 3 story "house tower". Specifically, when the structure "tower_base" is generated above the structure "third_floor_c". The bottom of the "tower_base" structure intersects with where the chests are. I'm attaching a screenshot that shows the outer blocks of the bounding box using lapis blocks. This causes the chests to be replaced with air blocks spilling their contents.
The way these structure files are processed it is perfectly legitimate to not specify certain blocks inside the bounding box. They just get ignored. If you delete the air blocks at coordinates <3, 0, 0> and <4, 0, 0> in the structure file, the chests will not be destroyed.
I can detail the precise cause of this in 1.12.2 using MCP 9.42. I don't know how relevant this will be with the current Blaze3D work, but for anyone else looking...
In RenderGlobal.setupTerrain there is this line:
Set<EnumFacing> set1 = this.getVisibleFacings(blockpos1);This creates an ad-hoc VisGraph to find all adjacent subchunks visible from the view position. In the situation described by this bug it will only find 1. Later, this happens:
This removes any subchunks found in the previous call that are opposite the cardinal direction (or more precisely the 6 possible directions for EnumFacing) that the viewer is facing.
One scenario would be only the eastern side of the subchunk is open and the player is facing a heading of 45.1 degrees. Technically, the players cardinal direction is west. The visible subchunk to the east is removed from the set.
This causes the game to determine that the only visible subchunk is the one that the viewer is standing in, and it skips everything else to only render it.
The adjacent subchunk frustum checks later in this function are probably sufficient, and this attempt at optimization could be entirely eliminated. However, it would be more appropriate to perform frustum clipping checks here than than this naive cardinal direction check.
This is
MC-63020.As far as I can tell this has been resolved in 1.16.2-pre1. In 1.16.1, according to profiling, 50% of the render threads CPU time was consumed with light updates. This was due to a block update limit of 64 in a chunk that would cause a new chunk data packet to be sent to the client which the client would have to relight entirely. This could cause tens or even hundreds of thousands of light updates in a tick that all happened in the render thread.
In 1.16.2-pre1 this limit has been removed, and the server sends delta packets and light updates for any number of block changes. This has reduced light calculations at the client by a factor of 100, from 50% to 0.6% in profiling, and has greatly increased FPS and reduced client side lag considerably.
Nearly all other efficiencies are classic block entity render and render chunk rebuild inefficiencies that have always existed. I'll leave it up to the mods and Mojang if this should continue to be tracked here.
The fix for this is pretty easy, so easy I hope to see it implemented soon. The Guardian entity renderer alters the texture animation and other properties using the world tick time. Converting the world tick time to a float and using fractional render ticks causes overflows eventually.
The solution is dead simple and initially done by AMereBagatelle in this PR for the Sodium mod: https://github.com/jellysquid3/sodium-fabric/pull/419
It uses the current bounded day clock tick instead of the world tick. That is all.
Thanks!
There's a good chance the client lag spike is being hidden from me due to a large disparity in system specs. I was searching for a bug behind the issue in HC and thought this was it, but you're right. Your original videos only show a client side spike. The other issue is likely datapack or mod related. Sorry to bother.
I'm going to delete my original comments.
This also impacts using AdoptOpenJDK 16 to launch a modded 1.16.5 game. Worked just fine yesterday. Deleting "C:\Program Files (x86)\Minecraft Launcher\game\JavaCheck.jar" did allow me to launch the game.