Network Rewrite

[ElvishJerricco]

Closes #30

• [x] Re-implement several interfaces on several classes.
  • [x] On cable parts.
  • [x] On modem parts.
  • [x] On intermediate parts.
  • [x] On wireless bridges.
  • [ ] Wireless bridges for turtles
  • [x] On modem blocks.
  • [x] On vanilla cables.
• [x] Remove a ton of functionality that is now implemented more simply in NetworkController
• [x] Make BasicModem implement INetworkedPeripheral calls.
• [x] Instantiate a network when a node is placed down.
• [x] Wireless bridge bindings should provide nodes, not positions of nodes.
• [x] Remove INetworkController argument from INetworkNode.receivePacket

Additional features we should add at some point (added by @SquidDev).

• [ ] Replace set of connections with a more efficient data structure. Ideally we should be able to find connections to/from any node without having to iterate through the list. A Map<INetworkNode, Set<INetworkNode>> would work, but it does mean a two way mapping is harder to ensure.
• [ ] Bring back the unit tests. I deleted them in 3bc51af1a10666891fe4d72177e37fa15e373ca0 but it might be worth bringing them back in a different form to ensure that everything works correctly.
• [ ] Optimise the network code. Currently removing a node recreates the entire network. This could be happening several hundred times on a chunk unload.
• [ ] Cables cache connections
• [ ] Send packets directly rather than on the update tick.

Fixes #35 and closes #30.

[ElvishJerricco]

Finally starting to make some headway on the code for this. Not compiling currently (changing the methods in INetworkNode, and still need to implement all the changes). But I think I've got a good plan for network changes.

First of all, the NetworkController class won't at all care about world positions and the like. Only INetworkNodes. So INetworkNode implementations have to implement getConnectedNodes(), which nicely removes the need for getExtraNodes(). Difference is that a cable now has to explicitly state that it connects to which cables on its sides. Network helper functions can make this easy.

Second, network modification. There are four modes of network modification I can think of.

• Disconnect
  • Ex: Put a cover between cables.
  • Call the network controller with INetworkController.breakConnection(INetworkNode node1, INetworkNode node2).
  • Network controller starts with node1, scans the network to gather the new network.
  • During this scan, if node2 is found, cut immediately.
  • This means the network didn't end up changed and no work needs doing.
  • Find any nodes in the old network no longer in the new network.
  • Remove these nodes from the network.
  • Create a new network starting at node2 and allow it to populate itself.
• Connect
  • Ex: Remove a cover.
  • Call the network controller with INetworkController.formConnection(INetworkNode existingNode, INetworkNode newNode).
  • If newNode is already on the network, no work needs doing. Cut.
  • Add newNode to the network and start a scan at newNode.
  • Assimilate nodes by removing them from their old network and adding them to this one.
• Node removed
  • Ex: Break a cable block.
  • Call the network controller with INetworkController.removeNode(INetworkNode node).
  • Use node.getConnectedNodes() to find nodes not destroyed.
  • If there is only one, cut. No network change.
  • Perform a network scan for each one of these nodes to form all the newly severed networks.
  • After scanning one network, remove any nodes already found in a network from the list of nodes to start scanning from.
  • If there's only one found network, no network change has been made.
  • Otherwise, create new network controllers for each of the new networks.
• Node added
  • Ex: Place a cable block.
  • Call the network controller with INetworkController.addNode(INetworkNode node).
  • Add the node to the network, and start a network scan at that node.
  • Should be similar to formConnection().

There's one really interesting consequence of this. Let me preface with: I don't think we should plan to implement this next idea any time soon.

We don't have to treat cables as a grid of nodes. We can actually just treat a segment of cables that doesn't branch at all as one node basically. Just kinda interesting. Potential future optimization.

Anyway, after any of these operations, check the old list of peripherals against the new list of peripherals. Based on any changes found, inform modems, peripherals, etc. This does mean that any networks spawned as a result of these operations will have to be aware of the list of peripherals they used to have before severing.

[SquidDev]

Sounds good. My one issue with getConnectedNodes is that every node has to scan the world six times to find them. This shouldn't be a problem it just seems slightly annoying. How will disconnect work between two multipart cables though - both cables have to be notified that a cover has been removed and so update their getConnectedNodes list, maybe I'm just misunderstanding.

We'll also have to take care that a provider may return a node that represents the same position but is a different object (if someone wanted to have redstone blocks as nodes they would return a new node instance every time we request it). This could slow down searching slightly as I guess we will have to check equality linearly rather than using a HashMap/HashSet/etc... as we can't rely on getHashCode. There might be a way to get around it but I don't know.

Another thing to remember is that we have to store distance from one node to another (for network packets) so we will still have to have methods to traverse the network normally.

[ElvishJerricco]

It will be slightly annoying to have to search all six sides in the getConnectedNodes thing, but I think that can be mitigated by a simple helper function.

Disconnect between two multipart cables is just an ordinary disconnect. If I put a cover inside one multipart to disconnect from the other, one will call breakConnection. At this point, the cables simply employ their own logic to stop rendering towards each other. We might want a subclass of INetworkNode like IWorldNetworkNode, whose purpose is to be a network node that indicates if cables should render toward them in a direction. Node providers would return these instead I guess.

Providers that are providing a new instance of INetworkNode for every time we request a node would be wrongly implemented if they didn't override equals and getHashCode.

And yea I forgot about packets remembering distance. But I think that's a lower priority at the moment. We'll get there when we get there.

EDIT: Accidentally hit tab then enter before finishing.

[ElvishJerricco]

Apologies for the slow progress. I've had a busy week. But I'm definitely getting somewhere with this. Not going to push until all the INetworkNode changes are implemented across the board (don't want to push code that doesn't compile). But there's an interesting difficulty.

Should networks remember which nodes attach which nodes? Or should they just repeatedly use getConnectedNodes to figure out which nodes connect which?

I think I should put the effort into making it remember which nodes connect which, because this way nodes like the modem block don't have to be able to tell the network that they connect to adjacent cables. Instead, we just have to know that the cable said it connects to the modem block. Keeps strange code out of the modem block node.

Also, I think I have a good way to do the packet transmitting. In INetworkNode, there will be getDistanceToNode(node), which will return the distance between the node and the parameter. It will be guaranteed to only be called by passing a node that could be gotten from getConnectedNodes, so that the node can return a value that it can easily determine (for cable blocks, it'll be 1 to adjacent cables, and 0 to multipart nodes in the block space for example). This means wireless bridges will still be able to accurately represent distance.

[SquidDev]

Don't worry about slow progress, I'm so thankful of all the work you are doing on this. If there is anything I can help, with I'm happy to.

Hmmmm, I really don't know. Ideally INetworkNode would be as easy to implement as possible, but also we need to cope with annoying things such as the distance between nodes. I just don't see a better way though. Uggghr.

[ElvishJerricco]

It sure would be nice if the norm in MC modding was Scala. Traits would be sweet.

[SquidDev]

I seem to remember this might make its way into Forge at some point. Its effectively mixins for Java. The wiki describes it better. So you can do:

@Mixin(EntityPlayer.class)
public abstract class MixinEntityPlayer extends Entity implements LivingThing, Leveller {

    @Shadow
    private int level;

    @Override
    public void setLevel(int newLevel) {
        // Refers to the shadow field above, but will refer
        // to the REAL field when the mixin is applied
        this.level = newLevel;
    }
}

where @Shadow is sort of like @Stub - it targets the mixin's level property instead.

[ElvishJerricco]

Interesting. Although honestly, that just seems like a less capable (albeit, more streamlined) version of the transformation library in CC-Tweaks. And I don't see a way to use it to implement essentially default methods for interfaces. Although actually, I believe Mojang said they plan on shipping a JVM with MC, guaranteeing a certain java version. When they get there, we can start using Java 8 features.

[SquidDev]

Well, we could add Mixins to CCTweaks :stuck_out_tongue:. They are packaging Java 8 with Windows/Mac launchers, but not with FTP/Curse/Tekkit/etc... launchers. 60% use Java 8 but that really isn't enough, maybe another year :cry:.

An alternative would be a compile time mixin ability. Or I guess we could rewrite in Scala.

[ElvishJerricco]

Ugh. One of these days I'll write a Lambda and mean it... =P

[SquidDev]

I really don't like Java's lambdas. I'm so used to .Net's LINQ which is much better and more flexible. But anyway, enough of Java rants...

[ElvishJerricco]

Yea back to the matter at hand,

I'm writing a utility class to help track connections. Basically, UnorderedPair<X, Y> is a class that has two final instance variables. Its equals and hashCode methods are overridden so that new UnorderedPair<Integer, String>(1, "hey").equals(new UnorderedPair<String, Integer>("hey", 1)) == true. Idea being the network controller can have a HashSet<UnorderedPair<INetworkNode, INeworkNode>> of pairs representing connections made.

[ElvishJerricco]

I think the biggest issue at hand is determining to add a new network to the world. When a cable is placed down, we want it so that the networks that the node might become a part of reach out to it and add it, instead of the node reaching out and adding itself. This way the node doesn't have to know every possible way it might be connected to a network. For example, someone might add a node that connects to distant nodes that don't even know they're being connected to this way. In this case, it's up to that strange kind of node to add the distant node, instead of being up to the distant node. So when the distant node is added to the world, it can't know which networks it'll be a part of.

Point is, if a node doesn't know what networks it'll be a part of, when does it know to create its own network for itself?

[SquidDev]

So a similar problem would be covers. If you put the connection code inside the placed node, it needs to check if the other node can accept connections on that face, and if the neighbour handles it then it needs to check if the placed node can accept connections. Neither way is nice.

I'm going to have to go and think about this....

[ElvishJerricco]

Well covers are pretty simple. The cable being covered knows what's on that side and can the do the appropriate breakConnection call. Or at the very least, when onNeighborChanged is called, it can find out what it can't connect to anymore and call breakConnection for that as well.

Perhaps nodes should create a network that just contains that node when they're added. Then any networks that want that node can assimilate that network. And if that doesn't happen, it has the network it needs.

[ElvishJerricco]

In fact, because I'm changing to tracking connections in the network controller, getConnectedNodes isn't even necessary. Instead, nodes will simply be added by something, instead of the network reaching out to find them.

[ElvishJerricco]

Anyway I think the only nodes that will actually need to create a network are modems. The only time a network's services are needed are when a modem is active. So we can go from there I think.

[ElvishJerricco]

Also, why does all our code use distanceTravelled variables as integers? Shouldn't they be doubles?

[SquidDev]

Were they not originally integers? I think the original network visitor code used integers. Doubles only makes sense when you need to travel diagonally - so for wireless networks but for a wired network you can only travel a whole block so an integer seems to make more sense.

[ElvishJerricco]

But with the concept of the network API, we have no reason to assume that we will only travel in non-diagonal lines. Wireless bridges, for example, should add a transmission distance that has to be a double.

[SquidDev]

Hmmm, hadn't thought about that... I guess we can no longer consider them exclusively as 'wired' networks and yeah, a double is fine.

[ElvishJerricco]

Alright. I'm at a point of API-level stability I think. Do you want me to push my changes? Here's what's left:

EDIT: Moved list to OP

The volume of each of these things is pretty huge, but since I'm at a point where the API isn't changing, I think we can work together on the remainder of this.

[ElvishJerricco]

Also worth noting that it totally doesn't compile yet, because of the need to re-implement so many interfaces.

[SquidDev]

Go for it!

[SquidDev]

Sorry, only just got round to pulling. Is it safe to remove the INetworkNode.lock() method? Presumably we can now lock on the entire network rather than each node's lock object.

Just wondering why receivePacket takes a INetworkController object, will that not be identical to getAttachedNetwork?

Could networkInvalidated take two maps - added and removed peripheral maps instead of a map of old peripherals? I say this because the list of old peripherals is only useful if you need to compare it with the new one.

Thoughts?

[ElvishJerricco]

I believe it would be safe to remove the lock method.

You're right, receivePacket probably doesn't need the INetworkController argument. My mistake.

networkInvalidated passes the old list in just because it's easier to calculate, but perhaps that would be better.

[ElvishJerricco]

BTW, I'm working on BasicModem right now, so I've got that one covered.

[SquidDev]

I'm having a look at doing TileCable, though I can't guarantee anything. On the case of attachToNetwork, should I throw an exception if I already have a network, fail silently or succeed?

[ElvishJerricco]

Well theoretically, the NetworkController class should never ever call attachToNetwork without first calling detachFromNetwork, except sometimes when getAttachedNetwork returns null. What do you think should be the policy on this?

[ElvishJerricco]

Also, TileCable would be so much better / easier if it used BasicModem.

[SquidDev]

I'm think fail fast and hard is better, if we are trying to attach twice then something has gone wrong somewhere and we need to know about it as soon as possible.

I guess. I'm stuck between 'change all the things' and backwards compatibility. Looking at TileCable that will probably be the hardest (and so probably last) to do as it does pretty much everything - connections, modems, etc... Maybe instead of 'patching' it, we should just rewrite the entire thing.

[SquidDev]

Also, wondering if INetworkAccess should use the same method names as INetworkController, though obviously we can't inherit from it?

[ElvishJerricco]

That's not a bad idea. But ultimately I don't think it's super important.

I've got BasicModem migrated. I'll move onto it's subclasses if you want to tackle making INetworkedPeripheral and INetworkAccess work.

[ElvishJerricco]

Modems proved to be surprisingly simple. Just need to update nodes I guess.

[ElvishJerricco]

I don't think we want the BasicModem implementing INetworkAccess. The network controller has to be able to call attachToNetwork and detachFromNetwork with the access object, which it has no way to know is the modem. I think it'd be better if the network controller performed all of the INetworkedPeripheral operations and provided its own access objects.

[SquidDev]

The one issue is that the network controller needs to know where a packet is transmitted from transmitPacket(INetworkNode, Packet) so we need a custom INetworkAccess that knows what node it is. I guess we can have a simple NodeNetworkAccess which holds a node and a controller, but then BasicModem would need to store an instance of that instead which isn't ideal. Thoughts?

[ElvishJerricco]

My thought was that NetworkController would have a Map<String, NodeNetworkAccess> attachedNetworkedNodes map instance variable. In the packet sending code, whenever it gets to a node, it checks if the node has any networked peripherals and sends the remembered NodeNetworkAccess

[ElvishJerricco]

To expand on why I think the INetworkAccess should be handled by the controller: This way, INetworkNodes don't have to implement any INetworkAccess stuff for peripherals they attach. The network handles that for them, which is nice, because it removes unnecessary boiler plate and implementation from the nodes.

[SquidDev]

Sorry, I'm in the middle of exam season so just haven't had a proper chance to look at things yet.

I'm probably being really dumb but for Map<String, NodeNetworkAccess>, what would the String key be? I guess the peripheral name, but then we might have multiple INetworkAccess instances for one node.

[ElvishJerricco]

Yea it'd be the peripheral name. Perhaps they key would be better as the node that provided the peripheral?

[ElvishJerricco]

I gave it a look to see if I could do it easily before pushing all my changes over the last day, and it's a little weird. It'd definitely just be easier if the modem took care of it, i think.

So yea, I guess we'll just have the modem handle networked peripherals.

[ElvishJerricco]

Issue is that it's not always the attached network controller that calls detachFromNetwork, so it won't always be sane where the INetworkedPeripheral calls go if they're in NetworkController. Just means it's easier to put them in the appropriate modem methods. (Although, this is another instance where Java 8 default interface methods would be great. We could implement methods in the INetworkNode interface to do this job on its own).

[ElvishJerricco]

Does FMP not provide a way to see when a part is added to the tile? I know there's onPartChanged, but that's called in more circumstances than just when a part is added, and it doesn't make any distinction. How do we determine when a part is added? We need to be able to respond to this so that CablePart can add that part to the network if its a node.

[SquidDev]

No, don't think so. You can listen for remove by checking if the TMultiPart parameter of onPartChanged is still in the multipart.

[ElvishJerricco]

Well the solution I used was just adding connections between the cable and every part in the tile. The network properly handles trying to add an already existing connection, so it's fine.

[ElvishJerricco]

INetworkedPeripherals! We're definitely getting somewhere here.

[ElvishJerricco]

Wireless bridges! Got the bindings reworked. Am I incorrect to handle connection forming / breaking in the NetworkBindings class?

[ElvishJerricco]

As for TileCables, I think we have an opportunity to abstract cable logic a bit here. Instead of making TileCable an IWorldNetworkNode, we can make it an IWorldNetworkNodeHost, doing the same to PartCable. The node they provide is something similar to BasicModem, but for cables instead of modems. It'll handle the logic for connections and forming / breaking them. And TileCable can just keep an instance of SinglePeripheralModem. And it calls formConnection on the network controller to put that modem on the network.

[SquidDev]

Gosh you've been busy! Thanks! The NetworksBindings looks good. :+1:

I had a look at abstracting some code out in #32 but I thought that at the time there was no point. Now nodes are doing much more work I think we should abstract away as much as code as possible.

I have a wee bit more time now if there is anything left to work on?

[SquidDev]

Not relevant at all, but there have been 137 commits, 10,000 additions and 6,000 deletions since the last release. Wow.