TheCBProject / CBMultipart

An API for dynamically handling different functional parts in the one block space.
Other
22 stars 27 forks source link

Fix world lag when SavedMultipart cannot be loaded #41

Closed drzpk closed 6 years ago

drzpk commented 6 years ago

In the issue #40 I've written that there was a massive lag caused by the fix you provided. I figured out what was causing it.

The issue was occurring only when a saved multipart tile entity couldn't be loaded properly. Take look at the TileNBTContainer.update method: when the variable newTile is null, tile entity should removed from world. But somehow it isn't (I have no idea why). The problem is that none of the conditions (failed and loaded) is changed afterwards and a tile entity is being removed 20 times per second. Note that every time the World.removeTileEntity method is called (even if that tile doesn't exist anymore) container block's neighbors get notified. Multiply this by a large amount of corrupted multipart tile entities (in my case after update 1.10 to 1.12) and you've got a lag.

covers1624 commented 6 years ago

Its worth noting that TileNBTContainer's update method should only be hit as a last resort, if that inst the case then there are other issue to look into.

drzpk commented 6 years ago

Looks like the method responsible for creating composite tile entity returns null only when a TileNBTContainer is empty:

val newTile = TileMultipart.createFromNBT(tag)

Shouldn't a container block just be removed in such cases? There's no point in keeping them empty.

Besides, after more thorough analysis I'm pretty sure that calling World.removeTileEntity is the source of the issue I've been experiencing:

  1. TileNBTContainer wishes to remove tile entity without removing its container.
  2. Tile is removed, but its neighbors have to be notified of this event.
  3. World calls Block.onNeighborChange event on adjacent blocks.
  4. BlockMultipart receives the event and wants to get the TileMultipart instance.
  5. It the tile doesn't exist anymore (because it was removed - see the 1st point), it will be recreated (see World.getTileEntity)
  6. If corrupted BlockMultiparts are adjacent (i.e. they're cables), the whole procedure starts over.
covers1624 commented 6 years ago

I think i have a cleaner fix for this. Give me a few to write it up.

covers1624 commented 6 years ago

I believe https://gist.github.com/covers1624/99eed3a286820de14146730bf19025f6 should fix the problem, It covers all edge cases you pointed out. https://cdn.discordapp.com/attachments/159971888485892096/476441440440942602/ForgeMultipart-1.12.2-2.5.0.null-universal.jar
Please verify.

drzpk commented 6 years ago

I can confirm that almost everything works properly now.

But there's one minor problem left: log is being flooded with these messages: SavedMultipart at BlockPos{x, y, z} still exists after 600 ticks! It's caused by multipart tile entities that disappeared during game update (1.10 -> 1.12), but their container is still there. Why the container block can't be removed when this happens? I just checked few of these phantom blocks im McEdit and their NBT tags are empty (don't contain any parts).

covers1624 commented 6 years ago

Hmm, i guess its safe to set those blocks to air then. Please try this build: https://cdn.discordapp.com/attachments/159971888485892096/476456361891201035/ForgeMultipart-1.12.2-2.5.0.null-universal.jar Here's the patch incase you were curious: https://gist.github.com/cc67011b05660e93e05dbde71065aa32

drzpk commented 6 years ago

As far as I can tell, problem is solved.

Thank you for dealing with this issue so quickly.

covers1624 commented 6 years ago

No problem, Suppressed by https://github.com/TheCBProject/ForgeMultipart/commit/c6a0dcf4d3e283adbd4fbb9e9813a6d6c3398418
Also, Thanks for your effort here, even though i ended up with a slightly different implementation.