Repeatable crash with Coal/Charcoal Piles #3330
Comments
|
I believe this is the same crash/issue that has been reported a few times such as in: #3261 (comment) |
|
Found a few additional Pile related bugs and their causes (and some potential fixes). These are related to the behavior of piles breaking as they are being built and a discrepancy on the size of a pile between the server and client. However are a bit of doozy to explain. When constructing a pile and ending up in any format of
This looks like intended behavior at first, however, that pile above does not collapse, it breaks. As such it leaves behind the a drop amount based on its fill, in essence duplicating some portion of its items. What is actually happening is the base pile [16] attempts to be filled by player interaction, is full so creates a new pile object above itself and hands off the TryPutItem into it creating a temp situation of:
Then it runs a TriggerPileChanged() but on the base [16] pile this in turn can end up creating (on the server at least) a very temp pile of
as 2 is removed from the base pile, creates a EntityFallingBlock above one of the horizontalface positions, it leaves the original pile at 14 and creates a blockEntity neighboring the small pile ontop.
The EntityFallingBlock then naturally falls, removes the block from the initialPosition and triggers OnNeighbourBlockChanged() on its neighbors, this including the small [2] size pile. Having a gander at BlockCoalPile.OnNeighbourBlockChanged() we see: public override void OnNeighbourBlockChange(IWorldAccessor world, BlockPos pos, BlockPos neibpos)
{
Block belowBlock = world.BlockAccessor.GetBlock(pos.DownCopy());
if (!belowBlock.CanAttachBlockAt(world.BlockAccessor, this, pos.DownCopy(), BlockFacing.UP))
{
world.BlockAccessor.BreakBlock(pos, null);
return;
}
...
}The size [2] pile checks whether or not it can be attached to the block below itself, if it cannot, it breaks itself. CanAttachBlockAt() for BlockCoalPile is as such: public override bool CanAttachBlockAt(IBlockAccessor blockAccessor, Block block, BlockPos pos, BlockFacing blockFace, Cuboidi attachmentArea = null)
{
BlockEntityCoalPile be = blockAccessor.GetBlockEntity(pos) as BlockEntityCoalPile;
if (be != null)
{
return be.OwnStackSize == be.MaxStackSize;
}
return base.CanAttachBlockAt(blockAccessor, block, pos, blockFace, attachmentArea);
}The belowBlock is size 14, its MaxStackSize is 16. 14 == 16 ->False The temp situation is now:
This is the final situation, with one caveat. This is the situation as seen by the server. The code for TriggerPileChanged, which consequently calls TryPartialCollapse() which reduces the pile size and creates the EntityFallingBlock, only executes its block when called by the server, the client gets just an empty return. void TriggerPileChanged()
{
if (Api.Side != EnumAppSide.Server) return;
...
foreach (var face in BlockFacing.HORIZONTALS)
{
...
if (Api.World.Rand.NextDouble() < collapsibleLayerCount / (float)maxSteepness)
{
if (TryPartialCollapse(npos.UpCopy(), 2)) return;
}
}
}This creates a discrepancy as on the clients end, the base pile never became [14]. As for the exact degree of why or when the discrepancy is resolved so that it becomes [16] again on the server im still wrapping my head around, presumably after OnPlayerInteract() on the BECoalPile resolves and outside the exact scope of the Piles themselves. You can confirm this behaivor by checking via debug statements the size of a pile when OnPlayerInteract() is called, after the base.OnPlayerInteract() is called, and after TriggerPileChanged() is called. And checking the size of pile and the pile below it when OnNeighbourBlockChanged() is called and passes the if-statement. Below in an example of done with debug statements inserted at the start of OnPlayerInteract(), after base.OnPlayerInteract() (in my case it was with a supplemental version within the same BlockEntityPile class I created but an exact copy of BlockEntityItemPile's, this was probably not needed just a fugue state coding moment), and after TriggerPileChanged().
The blockbreaking behaivor can really easily be seen in game, just by watching closely when making piles in creative or by engineering a few different structural examples:
In approaching the problem to "fix" it I think one of the easiest "solutions" is to not allow TriggerPileChanged() to occur on anything but the top most of any given "stack" of BlockPile entities, thus in the event of a server-client discrepancy or order of operations a pile cannot accidentally break all piles above it, as it is the topmost pile. This can be done in a few ways, recursively checking whether a pile attempting to call or would call TriggerPileChanged() as part of onPlayerInteract() has another pile above it, and executing it on that topmost pile, or as simple as not searching but just "if !aboveBlock is BlockEntityItemPile)" comes to mind and is something im still testing out personally. This change also makes pile laying and the collapse events more seamless and intuitive as if a player creates a large perfect stack of piles say 1 wide 5 tall (for whatever reason), a collapse would make the entire pile "shrink" and splurge out, rather than snap at the point of interaction from the bug and (at least to my view of the implementation) is more true to how piles were intended to be/made to collapse, always checking its relation to not just its neighbors but whether it is apart of a larger macro-pile (see TriggerPileChanged checking for a below pile to help weight the collapse chance). I also dont see much reason to ever consider a collapse event occurring, so long as there is a pile above. If a new pile is created above from an interaction, a collapse should start from that new pile, perhaps for sake of aesthetics and to make piles keep a similar degree of tendency to"sploot", a collapsing pile can "underflow" to its below parent, taking out some of its stackSize as well if it does not have enough by itself to collapse meaningfully or without error. That however would denote a whole slew of design changes to onTriggerCollapse() to consider. On a final note, I am unsure whether or not this is a bug or not or even just a personal circumstance, but I (think?) piles when falling are meant to have a rendered entity (showing the collapse in motion), appearing as a 2 Layer pile when rendered falling through the air to its final position, however I have not been able to visually see this in game, not with CoalPiles, my own custom piles, or even StonePiles which uses the same method for making the EntityFallingBlock and shares an idential OnTesselation() with VS and the one in my mod (it being a direct copy of VS's). Its a potential bug for another time and comment however. Apologies as well for the verboseness of these, this ended up being a far deeper rabbit hole in trying to fix one crash in my mod than I thought it would be and has sorta consumed my last couple days as a hyper-fixation. |
|
hi thanks a lot for the thorough report, but yes this is indeed a bit too verbose to me. I'll fix the stack sizing bug in your first post, but if there are more bugs, please open new issues for these Will be in rc5 |
Craluminum2413
added
status: resolved
Game Version
v1.19-rc1
Platform
Windows
Modded
Vanilla
SP/MP
Singleplayer
Description
Was working on my own Piles for a mod and verifying functionality for 1.19rc1 (Rot and Salt Piles) and came across an issue regarding GetSelectionBoxes and GetCollisionBoxes giving an Index out of bounds exception resulting in an immediate crash and consequent crashes when loading into the world for a variety initial call reasons (pathfinding, mouse-hovering, etc...) with the same above two methods giving Index out of bounds. After some extensive testing and digging, I found these crashes could also be repeated with Vanilla coal and charcoal piles due to a series of odd behavior and events due to TryPartialCollapse implementation.
How to reproduce
The issue occurs due to a BlockEntityPiles Layer property returning a negative value, such as -1 and then being used as an Index for the CollisionBoxesByFillLevel array.
The Layer property is calculated as such in BECoalPile:
public int Layers => inventory[0].StackSize == 1 ? 1 : inventory[0].StackSize / 2 ;Due to how PartialCollapses are written a pile can sometimes "collapse" and be left with a stacksize < 1.
This is due to a collapse always entailing a transfer of 2.
This can result in potential for a pile in the situation of:
[x] Represents a pileBlock, X being its stackSize.
For 0 and -1 stack sizes the pile is rendered but unselectable directly, only through its belowPile, if the pile gets interacted with to remove an item from it it will get replaced by an air block upon reaching a stackSize of 0.
However in an instance of managing to get a stackSize of -1, a player could accidentally engineer a situation where adding 1 item to its stack (thus creating a temporary stackSize of 0) and then resulting in a collapse situation would turn that pile into a stackSize of -2, its Layer property becoming -1, and thus resulting in a crash upon trying to use the above methods.
Screenshots
Repeatable setup (with some Rng hassle due to PartialCollapse rand) to generate the crash.
Initial setup
Attempt to create a 3 size pile on top of the 16, this may take a few goes due to early collapse
Right click the base 16 pile to remove 2 from the 3 size pile. Resulting in a bugged out -1 size Pile after it collapses.
Once in this situation attempting to add 1 to the 16 pile and getting a collapse to occur will crash the game with a log around the lines of what is attached
Logs
The text was updated successfully, but these errors were encountered: