jamesbirtles / ImmersiveIntegration

Integrates IE with other mods, as well as some useful addition to IE itself
http://www.curseforge.com/projects/232159/
21 stars 10 forks source link

Flawed AE2 integration #52

Closed yueh closed 8 years ago

yueh commented 8 years ago

Due to a random issue (proved to be unrelated), I glanced over your implementation for the AE2 integration and noticed af few issues with it.

First of all TileMETransformer.java#L44-L52 is flawed as calling IGridNode.updateState() on your neighbors is potentially a bad idea. It violates the contract which states, it should be called when your state changes, but does not mention you should or must call it on any of your neighbors. Doing so has a couple of issues, like it might not even be called on an IGridNode connected to your tile entity, causing a completely unrelated network to repath. Also it might cause a neighbor IGridHost to join a grid too early and in an incomplete state, resulting in random issues. Currently I do not know of any IGridHost implementation delaying their join, but that does not mean some addon might do it at one point. AE2 is perfectly fine should a IGridHost decides to only join a grid 5 minutes after it was placed into the world. Worst case is probably that an IGridNode with an undetermined state joins prematurely, crashes and then saves an unrecoverable state to disk without any chance to recover later. Probably very unlikely, but I would not be surprised, if one addon would do it at some point.

Secondly TileMETransformer.java#L54-L62 is a bit complex with manually creating a grid connection between the two parts. Changing TileMETransformer.java#L126-L128 to return an EnumSet containing UP for the lower and DOWN for the upper part and let AE2 handle the connection is probably easier.

Thirdly TileMEWireConnector.java#L49-L60 should not use an internal class. IAppEngApi.createGridConnection() will already take care of checking, if a connection already exists and throw an exception accordingly. Also just log the message and not the full stacktrace, so it is consistent with our logging. With rv3 calling GridNode.hasConnection() is even no longer possible.

Last but not least using WorldSettings in BlockMEWireConnector.java#L27, BlockMETransformer.java#L67 and BlockMETransformer.java#L74 is not ideal. Just use the registry provided by IAppEngApi, which would also allow you to support rv2 and rv3 without any change (and probably earlier ones). Otherwise it can break with any build of AE2, even stable ones as we do not guarantee any internal implementation to be stable.


Edit: Regarding 2. You could even consider reducing it to a single IGridNode in the bottom part and "connect" the wire directly to this one. Saving one IGridNode and having to deal with a multiblock with multiple world accessible ones.

jamesbirtles commented 8 years ago

Thanks for the advice, had no experience with AE prior to this so tbh I am happy it even worked, I will address these soon

yueh commented 8 years ago

If in doubt, just ask us. Probably easier then trying to figure it out.

AE2 has a pretty different design compared to other APIs like RF and we had a couple cases where devs still wanted to apply the concepts they know from these.

The basic concept is that a tile entity implementing IGridHost announces that it wants to be part of a grid and provides an IGridBlock to define its properties like on which sides it allows connections. IGridBlock even does not need to be something like a Block or TileEntity at all, a simple class implementing this interface is sufficient. The IGridNode is then the entry point for anything a grid provides. Everything else like actually forming connections is handled by AE2 itself (basically forming something like a large interdimensional multiblock).

Manually creating connections is only needed to bridge gapes, like the wires for IE. Due to this AE2 simply has nothing like canConnect, because this is basically fullfilled once one or more IGridHost exists in any dimension including itself. Thus always true, as it is a valid use case that a single IGridHost could create a connection between two or more sides to say tunnel an external grid/network without letting it join the same one as the tunnel itself.