One client side TileEntity getting replaced with a new instance for some reason

[BenignBanana]

Hey guys,

After hours of banging my head against a wall I finally managed to narrow down my code not working to one specific issue. For some unknown reason, one of my multiblock TileEntities is getting replaced by a new instance after a sync from the server. I have no idea what's causing this, the other identical TE is working perfectly fine, I'm at the end of my wisdom and I just hope that maybe someone here has any idea. The code is way too all over the place to post here (though you're welcome to check out the GitHub for the main library  and the mod itself, most of the code in question is in the library and all relevant classes have multiblock or sync in them). I did take some fairly detailed debug notes though, so here they are: 

/* type | side | type mem location | syncable id mem location | action */
boiler server 10066 10053 setting proper id
boiler server 10096 10143 setting proper id
tank server 10105 10112 setting proper id
tank server 10100 10037 setting proper id

tank client 10182 10154 reading proper id from stream
boiler client 10209 10196 reading proper id from stream
boiler client 10174 10222 reading proper id from stream
tank client 10259 10246 reading proper id from stream

/* type | side | coords | debug uuid msb | debug uuid lsb | actions performed */
boiler server 399 4 2100 7406611159510238969 -6004429813246348525 set value in rebuild | getActualBlockState()
boiler server 400 4 2100 8627676692757629106 -9168228343733114012 set value in rebuild
tank server 399 5 2100 984907512996252097 -8261117959805136242 set value in rebuild // triggered rebuild
tank server 400 5 2100 5216046578638867465 -9161033129145849707 set value in rebuild

boiler client 399 4 2100 -777796711269906213 -6909084835805274391 called 2x in getActualBlockState() | read from stream
boiler client 400 4 2100 1875314197142261539 -4929778570773350453 called 2x in getActualBlockState() | read from stream
tank client 400 5 2100 -2113191912659205197 -4780008886862939808 read from stream
tank client 399 5 2100 -6765782617268665069 -6187872588524286999 read from stream

/* like above, but the UUID combined with the BlockPos is the important thing to look at.
boiler client 399 4 2100 -1982676738952378984 -7018763097901900451 getActualBlockState() new TE!!!!
boiler client 400 4 2100 1875314197142261539 -4929778570773350453 getActualBlockState() this one is the same

 

Keep in mind that each of the two sections were taken in a single update tick after completing the multiblock.

 

I seriously don't know what to do anymore. Hopefully someone here can figure it out.

Edited July 2, 2017 by BenignBanana
flipped debug notes to be more in execution order

[Choonster]

I don't fully understand your code, but could it be that the IBlockState at that position is changing, triggering a refresh of the TileEntity (as determined by FruityTileEntity#shouldRefresh)?

[BenignBanana]

9 hours ago, Choonster said:

I don't fully understand your code, but could it be that the IBlockState at that position is changing, triggering a refresh of the TileEntity (as determined by FruityTileEntity#shouldRefresh)?

I don't see what would set a new BlockState though, especially considering only one of the two TileEntityBoilers has this issue. I also don't override shouldRefresh and in the default TileEntity that just returns true if the Block changes (which it obviously doesn't). After doing a bunch more debugging with a clearer mind I noticed some other things. For one, I made a new world to exclude the possibility of a world corruption during development. This didn't help. I also added a third block to the structure and tracked the TileEntities over quite a few multiblock breaks/rebuilds. What I found is that it's not one specific TileEntity that gets refreshed, but they almost always refresh one at a time. There were two syncs where 2 changed at once, but on the vast majority it was just one, going highest X to lowest X in order.

 

I also let the BlockState refresh from just normal render refreshes and the TileEntities never changed, combined with the fact that this only happens on the client side it means the bug must be somewhere in the sync code. I pretty much straight ported that from OpenModsLib which was written for 1.7 (aka pre BlockStates), so there might be a sneaky bug in there I can't find. I hardly changed anything in the sync system, so it might be easier for you to understand the Java equivalent code here: https://github.com/OpenMods/OpenModsLib/tree/master/src/main/java/openmods/sync

 

I've been looking at this code for way too long and I might just be missing something really obvious. I'll keep looking but maybe you can figure something out.

[Choonster]

FruityTileEntity overrides TileEntity#shouldRefresh to return true when the IBlockState has changed, not when the Block has changed.

 

If you put a breakpoint in BlockBoiler#createNewTileEntity with the condition worldIn.isRemote (i.e. the method is being called on the logical client), can you see what's causing the new TileEntity to be created?

 

Side note: Don't use ITileEntityProvider, override Block#hasTileEntity(IBlockState) and Block#createTileEntity instead.

[BenignBanana]

Haha, like I said I've been looking at this code for way too long. That override was copied from OpenModsLib and I totally missed it when I looked through the file. Also a good idea to break in createTileEntity(), I'll try that and see what happens.

[BenignBanana]

Hmm, so the original issue may or may not still be there, but after removing that shouldRefresh override and defaulting it to the vanilla behaviour, the server side no longer gets the onBlockPlacedBy event, and onBlockAdded() doesn't seem to fire at all, period. My guess is that shouldRefresh has something to do with sending a block update to the server, but I can't find the relevant code. Any ideas?

[Draco18s]

Post your updated code.

[Choonster]

TileEntity#shouldRefresh has nothing to with whether Block#onBlockPlaced or Block#onBlockAdded are called, so I'm not sure why that's happening.

 

The default behaviour of TileEntity#shouldRefresh for non-vanilla TileEntities is actually the same as what you had in FruityTileEntity(return true when the IBlockState changes), so removing the override shouldn't have changed anything.

[BenignBanana]

 

41 minutes ago, Draco18s said:

Post your updated code.

I always forget to commit when I'm trying to debug stuff. It's updated now.

36 minutes ago, Choonster said:

TileEntity#shouldRefresh has nothing to with whether Block#onBlockPlaced or Block#onBlockAdded are called, so I'm not sure why that's happening.

 

The default behaviour of TileEntity#shouldRefresh for non-vanilla TileEntities is actually the same as what you had in FruityTileEntity(return true when the IBlockState changes), so removing the override shouldn't have changed anything.

It seems that after restarting the client the events fire once on the server side (including blockAdded), but after that it only fires client side and blockAdded stops firing altogether. I honestly don't have a clue what's going on. I don't just have a breakpoint in the events themselves, I have breakpoints on lots of spots down the chain and the only ones that are getting hit are onBlockPlacedBy and teh following world.isRemote check in BlockMultiblockPart. I can't figure out what's happening. I made sure nothing down the inheritance chain is overriding those functions either.

 

Here's the listing of the relevant functions with break points marked as a comment

    override fun onBlockPlacedBy(worldIn: World, pos: BlockPos, state: IBlockState, placer: EntityLivingBase, stack: ItemStack) {
        worldIn.setBlockState(pos, state.withProperty(FACING, placer.horizontalFacing.opposite), 2) //BP: breaks once with worldIn instanceof WorldClient
        placeEvent.fire(BlockPlacedEvent(worldIn, pos, state, placer, stack))
        super.onBlockPlacedBy(worldIn, pos, state, placer, stack)
    }

    override fun onBlockAdded(worldIn: World, pos: BlockPos, state: IBlockState) {
        addEvent.fire(BlockAddedEvent(worldIn, pos, state)) //BP: never triggers
        super.onBlockAdded(worldIn, pos, state)
    }

    init {
        placeEvent += {
            if(!it.world.isRemote) { //BP: Triggers once with it.world instanceof WorldClient
                val te = it.world.getTileEntity(it.pos) as TileEntityMultiblockPart
                te.rebuild(it.pos)
            }
        }
        breakEvent += {
            if(!it.world.isRemote) {
                val te = it.world.getTileEntity(it.pos) as TileEntityMultiblockPart
                if(te.multiblockId.value != -1) (it.world.getEntityByID(te.multiblockId.value) as? EntityMultiblock)?.destroy()
            }
        }
    }

This is the hardest to track down bug I've ever had. Seriously.

[BenignBanana]

No one?

[Draco18s]

Probably not because no one else uses Krolin...sorry, Kotlin to program mods.  The fact that onBlockAdded is never called is a complete mystery to us.

[BenignBanana]

2 hours ago, diesieben07 said:

It would be best if you could provide use with a working Git repository of your mod.

I posted both in the OP. It's completely up to date after draco reminded me to commit.

 

2 hours ago, Draco18s said:

Probably not because no one else uses Krolin...sorry, Kotlin to program mods.  The fact that onBlockAdded is never called is a complete mystery to us.

Well yeah. I know that Kotlin hasn't really caught on yet outside the Android community, but most of the code is similar enough to Java to be understandable. Either way most of the more complicated kotlin code doesn't really deal with the problem, it's just that very simple bit of code I posted above.

 

Also, I mostly asked "No one?" as a way to push the thread back up, since the conversation kind of stopped after yesterday afternoon and the thread was about to leave the first page when I checked back today.

 

Sorry if I'm being a bit pushy, this is just a really big issue that prevents me from further working on my mod and despite my 8 years Java experience I cannot for the life of me figure this out by myself. I hate asking others to help me debug my own code, I only posted here because I'm pretty desperate.

[jeffryfisher]

If your TE is being replaced when you don't want it replaced, then the logic in your shouldRefresh method is probably faulty (or not being reached, resulting in default behavior, which replaces the TE for every state change).

 

Make sure you have overridden shouldRefresh, and make sure that its logic does replace the TE when it needs to be cleaned up (e.g. when the block class changes) but not when you want to keep your TE during a state change. Step through it in the debugger to watch it work.

[BenignBanana]

1 hour ago, jeffryfisher said:

If your TE is being replaced when you don't want it replaced, then the logic in your shouldRefresh method is probably faulty (or not being reached, resulting in default behavior, which replaces the TE for every state change).

 

Make sure you have overridden shouldRefresh, and make sure that its logic does replace the TE when it needs to be cleaned up (e.g. when the block class changes) but not when you want to keep your TE during a state change. Step through it in the debugger to watch it work.

I totally misread that vanilla code, I thought if it ISN'T vanilla it checks if the block changed, otherwise if the state changed. Seems like its the other way around. Derp. Anyways, still have the problem of onBlockPlacedBy not being called server side. I'm moving my way through all the internal Minecraft/Forge code atm to see why this isn't working. Hopefully I can find something.

Edit: Is there some kind of Discord or something for Forge development help? I don't want to spam this forum with updates on the debugging.

Edited July 3, 2017 by BenignBanana

[BenignBanana]

Alright I finally figured out what was going on with the events not being called server side. Turns out in this new world, unlike before, the multiblock parts had different Y coordinates instead of different X coordinates, which caused an infinite loop in my rebuild function (due to a mistake of mine), but I couldn't catch this in step through because the logic was in a lambda delegate (which is executed in a separate context, hence no stepping). As a result the server got frozen and any further events were no longer sent. I thought the whole debugger hanging up was an issue because of spending too long in paused mode (which happens fairly frequently), but after a few restarts I noticed it always happened in the same spot, allowing me to spot the issue.

I learned my lesson: Always replace delegates with regular for loops while debugging. The issue with TEs being replaced is also fixed thanks to jeffryfisher's help.

 

The question about a discord or something still stands though, I'd much rather ask for help in a chat program if it's something that won't be useful to see for anyone else in the future.