The Issue

Running the /setblock command to destroy an existing MultipartBlock removes the block's parts without ever invoking the onRemoved method on them, meaning that they are not given a chance to clean up any associated resources.

Currently onRemoved only seems to detect when a part is broken by the player. Things like World.setBlockState don't seem to have any effect.

The Cause

The /setblock command calls World.setBlockState which calls World.removeBlockEntity which removes the block entity without allowing it to invoke parts' onRemoved methods. It is possible to listen for the INVALIDATE event. The problem with the INVALIDATE event is that it is also called when the world saves, meaning editing the world will cause a deadlock.

Potential Solution

A potential solution would be to mixin into something in the World.removeBlockEntity process and call a method on parts to allow them to clean up when they're removed.

Notes

It would likely be a good idea to differentiate whether a part is being removed naturally or unnaturally, so that parts can know whether to d survival-like things like how containers drop their contained items.

I can probably add the fix to this to #54, as I need this in order to really test #54 anyways.

I've been thinking, it would probably be best to always call onRemoved and then have another method (with a name like onBroken) that is only called when the part is removed naturally (i.e. by a player, block-breaker, etc.). onRemoved vs. onBroken will be very much like the current onAdded vs. onPlaced.

WorldChunk.removeBlockEntity just before the BlockEntity.markRemoved call looks like a good place for the mixin for this.

@Inject(method = "removeBlockEntity", at = @At(value = "INVOKE", target = "Lnet/minecraft/block/entity/BlockEntity;markRemoved()V"), locals = LocalCapture.CAPTURE_FAILHARD)

It's sad that we can't just use markRemoved directly, but it looks like it gets called in all kinds of situations where we don't want to actually consider ourselves removed from the world (like when unloading the world).

Oops, I just noticed there's already an onBreak method. I imagine that's probably good enough.

This does however make me wonder if the REMOVED event is useful as-is or if it should be replaced by simply making sure onRemoved is called in all scenarios and having a separate STATE_REPLACED event.

I noticed there is a REMOVED event, however it gets fired when a part is added as well. This is likely as an effect of setting CAN_EMIT_REDSTONE which in turn updates the blockstate which then causes onStateReplaced to be called, firing the REMOVED event.

Furthermore, the REMOVED event doesn't get fired when a Create contraption is created because Create contraptions make sure to remove the blockentity via World.removeBlockEntity before actually setting the blockstate.

Mixing into WorldChunk.removeBlockEntity still feels like the most reliable way to make sure parts know when they are being removed.

The REMOVED event probably shouldn't be fired if the blockstate changes - that feels like an LMP bug, and one that could be fixed fairly easily.

Does the INVALIDATE event get fired when a create contraption is created? I assume we wouldn't want to remove the parts since they aren't actually getting "removed", just moved from the world to a contraption.

The INVALIDATE event does get called when a Create contraption is created, but it also gets called when unloading the world.

Maybe there should be a new event, specifically for when WorldChunk.removeBlockEntity is called that isn't fired the other times BlockEntity.markRemoved is called?

From conversations with AlexIIL on discord, the fact that onRemoved is not called for BlockState changes seems intentional. This is because, from what I can tell, methods on AbstractPart are for when something happens that directly, only affects that part, where as events are fired for things that affect the whole block.

I will close this issue and look for another way of updating GraphLib when a part is potentially removed, possibly using the INVALIDATE event.