MineMaarten / PneumaticCraft

PneumaticCraft source
Other
84 stars 51 forks source link

Pressure Tubes do not leak air in some circumstances #705

Closed Mordenkainen closed 9 years ago

Mordenkainen commented 9 years ago

This issue is happening in a mod I am working on. I understand that what I am doing is causing the issue, but thought I should report it because it is something that should be fairly easy to fix.

If you link an external IAirHandler to a Pressure Tubes IAirHandler using IAirHandler.createConnection, and the Pressure Tube is physically connected to one other IAirHandler the Pressure tube does not leak air when it is supposed to.

In the case described above, the pressure tube is "connected" to 2 IAirHandlers, but is only physically connected to 1 of them, causing the other end of the tube to be open and unconnected. Air does not leak from the tube in this case, but should.

This appears to be because TileEntityPressureTube only checks that the number of "connected" IAirHandlers == 1, not that it has only 1 physically connected IAirHandler: https://github.com/MineMaarten/PneumaticCraft/blob/master/src/pneumaticCraft/common/tileentity/TileEntityPressureTube.java#L134

Updating this to check that the tube has only 1 physically connected IAirHandler should resolve the issue.

It is possible that this issue could affect other blocks as well, but I have only observed it with the Tubes at this point.

MineMaarten commented 9 years ago

It sounds like you're implementing it wrongly. IAirHandlers will be connected automatically in normal circumstances, by looking at neighbors on block updates. IAirHandler#onNeighborChange() should be called when block updates happen to your block.

createConnection only should be used in very special circumstances, with your own IAirHandlers, not PneumaticCraft's (like tubes). For example, AE2's P2P tunnels connect normally to tubes, but are connected together using this createConnection. I will make this more clear in the javadoc though.

Mordenkainen commented 9 years ago

So in this case I do need to use createConnection.

I am adding PneumaticCraft support to Compact Machines (https://github.com/thraaawn/CompactMachines)

This mod adds a block which links to a room in another dimension. Each side of this block is connected to another block that is on a side of this room. Items, power, fluids, etc. can be transferred in/out via this connection. There is no physical connection or adjacency between the "block" and the "room", all the connections are virtual.

I added an IAirHandler to the "interior" connection in the room, but did not want to add an additional IAirHandler to the external block, because the block on the "inside" and the block on the "outside" are supposed to be the same block, doubling the air volume did not make sense. To make matters worse, there can be multiple "external" blocks for one room. If each had it's own IAirHandler, this would add a significant amount of storage volume to the tube network.

Instead I created a "dummy" IAirHandler in the outside block, with a volume of 1, that causes adjacent tubes to connect to the block but does not add any significant volume of air to the tube network. Then I monitor the onNeighborChange() and when I find an adjacent IAirHandler (or notice that one is no longer there), I use createConnection()/removeConnection() to create/destroy the connection from the adjacent IAirHandler to the "inside" IAirHandler.

I understand that this may not be quite the way the API was intended to be used, but I don't know if there is another way. It works, with the one issue that was detailed in this issue report.

If you could suggest a better way to implement this. I'm open to suggestions.

In any case, I still believe the pressure tube should have leaked in this case.

MineMaarten commented 9 years ago

Hmmmm....

I also don't see any other way to do this in this case... I'll fix this.

Mordenkainen commented 9 years ago

Thanks!

Like I said up front, I know that under normal circumstances this bug does not exist, and it is that I am doing something unusual that creates it.

I just didn't see any other way to properly integrate the two mods without doing it this way. Thanks for hearing me out!

As a side note, a couple of suggestions:

  1. A way to create an IAirHandler that just acts as a passthrough for another IAirHandler would have been nice in this situation. (or to "wrap" an IAirHandler in another object using delegates so you could do something like this yourself)
  2. A way to have a block that "connects" to adjacent Pneumatic blocks, but is not itself a Pneumatic block may be useful. This would allow a block to visually connect to tubes without being part of the tube network. Currently, if you do not return an IAirHandler, no connection is rendered.

If those two things were supported, it may make integrating PneumaticCraft with other mods that do not quite behave in the "expected" way easier.