Peripheral Network Access

[ElvishJerricco]

Closes #17

• [x] Network invalidation hooks
• [x] Packet handling
• [x] INetworkAccess for peripherals attached directly to computers.
• [x] Work with non-multipart cables.
• [x] Work with full block modems

[ElvishJerricco]

I'd like INetworkAccess to extend INetwork, but I don't want an API interface to extend an unpublished CC interface. Should I just have INetworkAccess implement the same named methods as INetwork?

[ElvishJerricco]

And oh gosh I didn't realize what you had been doing in the api-rewrites branch... These might be slightly difficult to merge.

[ElvishJerricco]

Working out some crashes that I ignorantly created, and now this is happening. HOW??

[SquidDev]

Awesome! I wouldn't worry about my api rewrites branch - I planned to merge it after this anyway as I knew this would create conflicts.

I don't think it should all the methods of INetwork - we don't need to know where the packet is being sent from for instance. I feel we should keep the signatures as similar to INetworkNode: transmitPacket(Packet) rather than sending the individual arguments. I don't know if it should be getConnectedPeripherals/getPeripherals rather than peripheralsByName though as they aren't exactly similar - one applies to the node, one applies to the network.

For INetworkedPeripheral do we want to have a networkInvalidated(INetworkAccess) method which is called when the network is invalidated? Peripherals might do their own caching of the network and they need to know about it.

I also think INetworkAccess should have an invalidateNetwork() method if it changes something and needs to tell people, though I'm not 100% sure that it should take this form.

My one worry about deferring peripheral finding is that finding them could happen on the Lua thread instead, which shouldn't be a problem but is something to be aware of.

[ElvishJerricco]

It's peripheralsByName because the point of network access is to let the peripheral see the other peripheral objects on the same network.

I like the ideas for network invalidation though. Updated the OP with a todo list for this.

[ElvishJerricco]

What's the best way to deal with patching a private inner class? I need to patch PeripheralAPI$PeripheralWrapper for computer local networks, but both PeripheralAPI and PeripheralWrapper have so many private fields and methods, I'm not sure what the best way to do that is.

[SquidDev]

I've noticed some bugs with peripherals not being detached until toggling peripheral access - not removed on neighbour updates. I've added some debug blocks that test just log to console and it doesn't register it any more.

[ElvishJerricco]

So wait you're saying IPeripheral.detach isn't being called when a peripheral's block is broken?

[SquidDev]

I've just tested this, and it is the same on master as well, so it isn't an issue with this pull request. The actual issue is that the network invalidate isn't fired unless all peripherals are removed. This is an issue with this method. This is only a stupid issue with multi-peripheral blocks.

[ElvishJerricco]

So what's the fix? Also thoughts on my question from a couple of comments ago?

[SquidDev]

I guess more accurately the issue is that the only check we do on multi-peripheral modems to see if anything has changed is the enabled one. I'll probably add a custom change check to the multi-peripheral modem.

Sorry, I only just saw the previous question. I guess you'll need to override the attach and detach methods? So if you @MergeVisitor.Stub the m_peripheral, m_attached, m_mounts and m_fileSystem fields it should work. There really isn't a better way :frowning:.

[ElvishJerricco]

Alright. Well do I just make a class and write in the methods that need to be overwritten, then use a partial patcher to patch? That's what I did with CableBlockRenderingHandler, since that's a private inner class, but I'm not sure that's the best way to do it.

new ClassPartialPatcher(
    "dan200.computercraft.client.proxy.ComputerCraftProxyClient$CableBlockRenderingHandler",
    "org.squiddev.cctweaks.core.patch.CableBlockRenderingHandler_Patch"
)

[SquidDev]

It should be fine seeing as most of the code should be copied from the original file. If you don't need methods from the outer class everything should work. I'm surprised that the CableBlock renderer works seeing as static constructors aren't supported and you have a static field. I'll probably do an ASM dump on that class to see what happened.

I'm not 100% happy with my patch to multi-peripheral modems - I ensure the IDs were identical before and after - but there could be an edge case where the IDs and so the invalidated event isn't fired.

[ElvishJerricco]

Yea the whole CableBlockRenderingHandler thing needs a reimagination. It definitely won't work once obfuscated, ant its weird that the static field works.

And yea there should be a better fix for that.

[SquidDev]

I've tested it, it does work. Because it inherits from an FML class instead it isn't obsfucated - and so works normally. There are several things which probably will need rewriting some time in the future. They work fine, just not perfectly, and so aren't a priority.

[SquidDev]

It also seems that the network events are not always called on multipart modems or normal modems. You may be aware of this already?

[ElvishJerricco]

I am not. What events aren't getting called?

[SquidDev]

Sorry for the slow response, detach events were never being called - I could toggle the modem and detach was never called.

[ElvishJerricco]

Hm... I honestly have no idea why. Not able to get at the code at the moment though.

[SquidDev]

That is fine, I'll have a go at fixing it. Gotta do some revision first though.

[ElvishJerricco]

Is there anything else major you see that needs fixing before merging?

[SquidDev]

Sorry, Friday is 'work on other stuff' day. I'll have a look now though it looks good!

[ElvishJerricco]

I'll be at a big MTG tournament today so I probably won't be able to respond to anything for a while, but if you have some spare time, I sent you a question in a PM on CC forums.

[SquidDev]

I've just had a look at this (8 hours after I said I would :frowning: - sorry) and I've found some more bugs. Most of these are just me being pernickety so sorry.

When you break a block next to a multipart modem, it is not detached. I think this is because TileNetworkedModem listens to both block and tile change, whilst PartModem only listens to block change, so the tile is removed before we can attach to it. A similar bug is that attaching tiles to a full block after they are connected does not fire attachToNetwork, but does still invalidate the network and so does call attach(IComputerAccess).

I'm thinking maybe a better way would be to cache peripheral instances so it is easier to track when attach/detach events should be fired and we are not dependent on events as much. However I'm not sure how best to do this as we would have to compare peripherals with IPeripheral.equals(IPeripheral) so we'd want some sort of custom set - or force subclasses of BasicModem to return a fixed length array.

A more major bug is that normal wired modems do not get attachTo/detachFromNetwork events at all. TileCable doesn't use BasicModem at all so we might want to change that and get original modem methods to delegate to the BasicModem class.

[ElvishJerricco]

I thought I fixed the block breaking detach thing. I'll take another look.

And oh I thought TileCable used BasicModem. My bad.

[ElvishJerricco]

Definitely having some trouble here. It's painfully awkward how little happens on demand. Instead, a lot happens in the update method, leaving a tick where a lot of things aren't right. I'll figure it out and get it working despite this, but then after we merge #29, I want to take a look at rethinking some of the network code. In particular, network invalidation should be immediately followed with revalidation, rather than waiting a tick. There should be a revalidateNetwork method, where nodes clear caches and rediscover their peripherals, then there should be a networkRevalidated method, where nodes reach into the network to find the remote peripherals the rest of the network found in the revalidateNetwork call.

[SquidDev]

The update tick delay is strange. I'm guessing exists for the same reason that packets aren't sent until an update tick: to ensure thread safety whilst traversing the network. The network could be invalidate on the Lua thread which we need to be aware of. If we can get it to happen on the Lua thread however without problems and remove the need to an update method at all, that would be wonderful.

[ElvishJerricco]

Yea but like I said, this is an issue that would be more easily solved after #29 is done. I'll submit an issue, then I'll try to close this issue today. Then hopefully we can merge master into api-rewrites, and get that swiftly taken care of.

[SquidDev]

I'm wondering if something like AE has might be worth thinking about - caching the entire network instead. There could be a shared network object for every node - though this could be much more complicated today.

I've looked at providing snapshots and it is possible - I'm not sure if it is entirely needed once 0.2 is stable though.

[ElvishJerricco]

Caching the entire network is a huge pain in the butt. Basically requires building an entire API just for managing what's on the network. Granted, I'd love to write something like that, and it would be better. But it just adds yet another big step before .2 can be released. Also, it largely invalidates a lot of the work we've done, which is a major feelbad =P

[SquidDev]

I wasn't suggesting we implemented it any time soon. It would be one of those things that would be interesting to do in the very long run. I guess I probably should have planned the entire networking code's structure before I dived into it: we're paying the price for that now. :anguished:

[ElvishJerricco]

So getConnectedPeripherals is now cached, and everything seems to be working.

Is there anything left before merging?

[SquidDev]

I'll pull and test now. Good work!

[SquidDev]

Just tested and most things seem to work. :+1: However on modem blocks network events are not fired if the modem is active and a peripheral is placed down next to it, however attach(IComputerAccess) events are fired.

An even more minor problem is that detachFromNetwork is not called when broken. I don't know how to fix that without caching peripherals though.

[ElvishJerricco]

Well peripherals are cached now. It was an unfortunately necessary measure....

Under what circumstances does detachFromNetwork not get called? I tested breaking peripherals next to computers, next to modems of both PartModem and TileCable kinds, and next to modem blocks, and the all seemed to work.

[SquidDev]

Ah, I didn't see the caching. My fault. TileCable works perfectly, but detach is never called when I break a networked peripheral. I'll test again to check.

Edit: OK, PartModem also works fine. It seems to be just an issue with the modem block.

[ElvishJerricco]

Ok so the issues are

• [ ] TileCable doesn't call detach when the connected peripheral is broken.
• [x] Already active modem blocks don't call attachToNetwork when an additional peripheral is placed next to it.

Am I missing anything?

[ElvishJerricco]

I just did a test in vanilla CC, a peripheral attached via cable doesn't get detached when broken. Should we change this behavior?

[SquidDev]

There are a lot of bugs with the default TileCable implementation - if you break the peripheral (so the modem also breaks) and replace the modem, it is still on. I think we probably should fix it, but I'll also add it to my 'network bugs' topic on the forums.

[ElvishJerricco]

Well, for now, I say we table the issue of fixing faulty vanilla behavior for later. I think I'd rather merge in the API rewrites, then try to see if we can rewrite how much of the networking works. Clearing that up and getting around all the weird stuff and corner cases we currently have to deal with would make fixing the vanilla stuff easier.

[SquidDev]

I'll do a quick test now and then I think we are ready to merge?

[ElvishJerricco]

Agreed. Is there anything more to do with the api rewrites branch before trying to merge these changes into it?

[SquidDev]

Just tested. Works like a charm. :shipit:

I don't think there is anything else for the API rewrites branch. I'll considering adding a INetworkNodeHost and IPeripheralHost classes that return a Peripheral/Node on TEs instead of delegating every call to a modem or using IPeripheralTile. That, and OpenPeripheral integration, I'll probably do on separate branches though.

[ElvishJerricco]

Great. I'll pull this into master and see what I can do to merge master and api rewrites.

[SquidDev]

Yay! Merge conflicts!