Double Chest Problem

[theoriginalbit]

A bug has been discovered with double chests through push in OpenPeripheral. reported here. I feel that it's probably with this line but I'm not too sure hence posting this issue, instead of just fixing it myself.

Example setup of how I see the problem occurring. Assume these chests are setup east/west. Assuming that push is invoked on the purple chest (since that's where the modem is) pushing an item west, the blue chest will be the target instead of the red chest being the target.

[lyqyd]

Looks like line 60 is probably problematic too. I'd think that the second comparison would need to be between thisInventory and otherInventory, if that sort of comparsion works at all, rather than between otherInventory and target.

[theoriginalbit]

I'm thinking that that comparison wouldn't work at all anyway, especially with double chests, since the doubleChestFix returns a new instance of a Double Chest Inventory, meaning that the Inventory's returned from the Purple and Blue chests would be different even though they're the same double chest.

[lyqyd]

Yeah, looks like that's the case.

I don't much see the utility of the doubleChestFix code, since the getInventory method on the chests themselves seems to work fine?

[theoriginalbit]

yeah I'm not too sure. I vaguely remember @nevercast stating to me why it was needed at some point, perhaps he can shed some light on the situation and why the getInventory wasn't just used.

[nevercast]

I don't believe getInventory did then what it does now. Back then the two parts of the chest were separate inventories, or at least that is how I remember it. You had to solve which one was left and right so to speak so that you would know which inventory was at the top of the gui and which at the bottom. Then we would reconstruct a pseudo-gui that would act as a proxy between the two real gui. This is how I remember it anyway, but it was years/months/a year/long time/some time/galaxy far far away, ago

[nevercast]

I can see how the screenshot you have there would certainly cause an issue, I never tested the large chests long-ways. It was always a turtle in front pulling from all slots and putting back. Clearly I didn't cover all the use cases

[boq]

I have no idea how to fix it in clean way. But this actually may be working as expected, since this operation always works on single block (not multiblocks, even as primitive as large chests). If we add code to handle long chest, what will be our behaviour if there are two viable inventories on longer side, like this:

 II
CXX

(C - computer, XX - double chest, I - individual (single-block) inventory).

Current code may behave in somehow confusing way, but actually may be most logical solution.

[nevercast]

I think inventories need to be abstracted to support a for example an origin and dimensions, so the smallest x,y,z would be the origin, and dimensions would extend +x,+y,+z.

Then adjacent inventories would be calculated based on the dimensions of the interacted inventory.

This would fix the original issue's problem, but complicates what @boq said. Perhaps the block that the computer is connected to is the origin, and when looking for an adjacent inventory, you would add the dimension of that block to the query.

I know how I would code this so that both @boq's case and @theoriginalbit's case would work. However I do not have a development environment setup.

Basically, have a way to query the dimensions of an inventory in each direction relative to the target block of the operation, so that the offset positions in each direction are multiplied by that dimension.

[nevercast]

Pseudo code!

function getDimensions( targetBlock ) {
   //         E.G. return Vec3(1,1,-2) for a large chest in the -z direction
   return CalculatedSizeRelativeToTarget( targetBlock )
}

function getOffsetBlock( targetPosition, relativeDimensions, directionVector) {
    offsetBlockPosition = targetPosition + relativeDimensions * directionVector
    return World.getBlock( offsetBlockPosition );
}

function getAdjacentBlock( targetBlock, direction) {
   targetBlockDimensions = getDimensions( targetBlock )
   // Convert the direction enum to a vector such as (0,1,0) for Up
   directionVector = direction.getVector();
   otherBlock = getOffsetBlock( targetBlock.position, targetBlockDimensions, directionVector);
   return otherBlock
}

function pullFrom( connectedBlockTile, direction, slotInfo, amount ) {
   otherBlock = getAdjacentBlock(connectedBlockTile, direction)
   return otherBlock.transferTo(connectedBlockTile, slotInfo, amount)
}

[nevercast]

Err this would not work perfectly I don't think, now that I'm mapping all the operations on paper. It would cause an issue when you wish to move in the opposite direction of the large chest

Top down view:

  C
OILLA

Telling the computer C to transfer from the large chest LL to the small chest I, would likely cause an issue where either it will try to interact with O instead of I. or in the right circumstances of negatives, it would try to interact with A

[boq]

Meh, don't bother. This hack for specific pseudo-multiblock is already bad. As well as assuming in code that is handling specific interface (IInventory), that it may have completely unrelated properties like position. Might as well be reproduced with other multiblocks that can have multiple TEs pointing indirectly to some inventory (forestry farms?).

[theoriginalbit]

forgive my ignorance, but could not using the TE's getInventory help solve this? keep moving in the particular desired direction until the inventory from the source and the inventory from the target don't match? since the inventory returned from getInventory would match when coming from the same TE (right?)

[boq]

No. First of all, there is no getInventory(), TEs implement IInventory, therefore it's always "one block" == "one inventory". Multiblock inventories don't exists from interface point of view. Also, we are creating special wrappers (provided by vanilla) to handle large chests, but they are created on block basis, so they are never same objects.

This code is almost identical to vanilla.

[nevercast]

Then I think the solution to this is, "Minecraft is a block game, and a double chest, so it's two blocks, moving an item long ways is going to put it in to the other half of the same chest. so don't do it that way"

I mean it's no different to opening a large chest in real life and moving an item half the width of the chest long ways, when you let go, it ends up in the same chest.

Unless someone has a brilliant solution to this, that is not too difficult and will work for all TE's; we might have to leave this one as 'Intended operation'

[boq]

Agreed. No point in hacking around that, if there is no way to do it for every other multiblock structure (or at least good portion of it). I personally would even remove double chest fix (so it actually moves items from one TE to other TE), but at this point it's legacy behaviour, so it probably has to stay.

Closing this issue, tempted to mark it as "wontfix".