diff --git a/projects/common/src/main/java/dan200/computercraft/impl/network/wired/InvariantChecker.java b/projects/common/src/main/java/dan200/computercraft/impl/network/wired/InvariantChecker.java index ea69bbbc4..0e627f7dc 100644 --- a/projects/common/src/main/java/dan200/computercraft/impl/network/wired/InvariantChecker.java +++ b/projects/common/src/main/java/dan200/computercraft/impl/network/wired/InvariantChecker.java @@ -4,45 +4,66 @@ package dan200.computercraft.impl.network.wired; +import org.jetbrains.annotations.Contract; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import javax.annotation.Nullable; + /** - * Verifies certain elements of a network are "well formed". + * Verifies certain elements of a network are well-formed. *

- * This adds substantial overhead to network modification, and so should only be enabled - * in a development environment. + * This adds substantial overhead to network modification, and so is only enabled when assertions are enabled. */ -public final class InvariantChecker { +final class InvariantChecker { private static final Logger LOG = LoggerFactory.getLogger(InvariantChecker.class); - private static final boolean ENABLED = false; private InvariantChecker() { } - public static void checkNode(WiredNodeImpl node) { - if (!ENABLED) return; + static void checkNode(WiredNodeImpl node) { + assert checkNodeImpl(node) : "Node invariants failed. See logs."; + } - var network = node.network; - if (network == null) { - LOG.error("Node's network is null", new Exception()); - return; + private static boolean checkNodeImpl(WiredNodeImpl node) { + var okay = true; + + if (node.currentSet != null) { + okay = false; + LOG.error("{}: currentSet was not cleared.", node); } - if (network.nodes == null || !network.nodes.contains(node)) { - LOG.error("Node's network does not contain node", new Exception()); + var network = makeNullable(node.network); + if (network == null) { + okay = false; + LOG.error("{}: Node's network is null.", node); + } else if (makeNullable(network.nodes) == null || !network.nodes.contains(node)) { + okay = false; + LOG.error("{}: Node's network does not contain node.", node); } for (var neighbour : node.neighbours) { if (!neighbour.neighbours.contains(node)) { - LOG.error("Neighbour is missing node", new Exception()); + okay = false; + LOG.error("{}: Neighbour {}'s neighbour set does not contain origianl node.", node, neighbour); } } + + return okay; } - public static void checkNetwork(WiredNetworkImpl network) { - if (!ENABLED) return; + static void checkNetwork(WiredNetworkImpl network) { + assert checkNetworkImpl(network) : "Network invariants failed. See logs."; + } - for (var node : network.nodes) checkNode(node); + private static boolean checkNetworkImpl(WiredNetworkImpl network) { + var okay = true; + for (var node : network.nodes) okay &= checkNodeImpl(node); + return okay; + } + + @Contract("") + private static @Nullable T makeNullable(T object) { + return object; } } diff --git a/projects/common/src/main/java/dan200/computercraft/impl/network/wired/NodeSet.java b/projects/common/src/main/java/dan200/computercraft/impl/network/wired/NodeSet.java new file mode 100644 index 000000000..9d3f0808e --- /dev/null +++ b/projects/common/src/main/java/dan200/computercraft/impl/network/wired/NodeSet.java @@ -0,0 +1,100 @@ +// SPDX-FileCopyrightText: 2024 The CC: Tweaked Developers +// +// SPDX-License-Identifier: MPL-2.0 + +package dan200.computercraft.impl.network.wired; + +import dan200.computercraft.api.network.wired.WiredNode; + +import javax.annotation.Nullable; +import java.util.Objects; + +/** + * A disjoint-set/union-find of {@link WiredNodeImpl}s. + *

+ * Rather than actually maintaining a list of included nodes, wired nodes store {@linkplain WiredNodeImpl#currentSet the + * set they're part of}. This means that we can only have one disjoint-set at once, but that is not a problem in + * practice. + * + * @see WiredNodeImpl#currentSet + * @see WiredNetworkImpl#remove(WiredNode) + * @see Disjoint-set data structure + */ +class NodeSet { + private NodeSet parent = this; + private int size = 1; + private @Nullable WiredNetworkImpl network; + + private boolean isRoot() { + return parent == this; + } + + /** + * Resolve this union, finding the root {@link NodeSet}. + * + * @return The root union. + */ + NodeSet find() { + var self = this; + while (!self.isRoot()) self = self.parent = self.parent.parent; + return self; + } + + /** + * Get the size of this node set. + * + * @return The size of the set. + */ + int size() { + return find().size; + } + + /** + * Add a node to this {@link NodeSet}. + * + * @param node The node to add to the set. + */ + void addNode(WiredNodeImpl node) { + if (!isRoot()) throw new IllegalStateException("Cannot grow a non-root set."); + if (node.currentSet != null) throw new IllegalArgumentException("Node is already in a set."); + + node.currentSet = this; + size++; + } + + /** + * Merge two nodes sets together. + * + * @param left The first union. + * @param right The second union. + * @return The union which was subsumed. + */ + public static NodeSet merge(NodeSet left, NodeSet right) { + if (!left.isRoot() || !right.isRoot()) throw new IllegalArgumentException("Cannot union a non-root set."); + if (left == right) throw new IllegalArgumentException("Cannot merge a node into itself."); + + return left.size >= right.size ? mergeInto(left, right) : mergeInto(right, left); + } + + private static NodeSet mergeInto(NodeSet root, NodeSet child) { + assert root.size > child.size; + child.parent = root; + root.size += child.size; + return child; + } + + void setNetwork(WiredNetworkImpl network) { + if (!isRoot()) throw new IllegalStateException("Set is not the root."); + if (this.network != null) throw new IllegalStateException("Set already has a network."); + this.network = network; + } + + /** + * Get the associated network. + * + * @return The associated network. + */ + WiredNetworkImpl network() { + return Objects.requireNonNull(find().network); + } +} diff --git a/projects/common/src/main/java/dan200/computercraft/impl/network/wired/WiredNetworkImpl.java b/projects/common/src/main/java/dan200/computercraft/impl/network/wired/WiredNetworkImpl.java index ef8527304..c297b7294 100644 --- a/projects/common/src/main/java/dan200/computercraft/impl/network/wired/WiredNetworkImpl.java +++ b/projects/common/src/main/java/dan200/computercraft/impl/network/wired/WiredNetworkImpl.java @@ -8,6 +8,7 @@ import dan200.computercraft.api.network.Packet; import dan200.computercraft.api.network.wired.WiredNetwork; import dan200.computercraft.api.network.wired.WiredNode; import dan200.computercraft.api.peripheral.IPeripheral; +import dan200.computercraft.core.util.Nullability; import java.util.*; import java.util.concurrent.locks.ReadWriteLock; @@ -187,10 +188,76 @@ final class WiredNetworkImpl implements WiredNetwork { return true; } - var reachable = reachableNodes(neighbours.iterator().next()); + assert neighbours.size() >= 2 : "Must have more than one neighbour."; + + /* + Otherwise we need to find all sets of connected nodes within the graph, and split them off into their own + networks. + + With our current graph representation[^1], this requires a traversal of the graph, taking O(|V| + |E)) + time, which can get quite expensive for large graphs. We try to avoid this traversal where possible, by + optimising for the case where the graph remains fully connected after removing this node, for instance, + removing "A" here: + + A---B B + | | => | + C---D C---D + + We observe that these sorts of loops tend to be local, and so try to identify them as quickly as possible. + To do this, we do a standard breadth-first traversal of the graph starting at the neighbours of the + removed node, building sets of connected nodes. + + If, at any point, all nodes visited so far are connected to each other, then we know all remaining nodes + will also be connected. This allows us to abort our traversal of the graph, and just remove the node (much + like we do in the single neighbour case above). + + Otherwise, we then just create a new network for each disjoint set of connected nodes. + + {^1]: + There are efficient (near-logarithmic) algorithms for this (e.g. https://arxiv.org/pdf/1609.05867.pdf), + but they are significantly more complex to implement. + */ + + // Create a new set of nodes for each neighbour, and add them to our queue of nodes to visit. + List queue = new ArrayList<>(); + Set nodeSets = new HashSet<>(neighbours.size()); + for (var neighbour : neighbours) { + nodeSets.add(neighbour.currentSet = new NodeSet()); + queue.add(neighbour); + } + + // Perform a breadth-first search of the graph, starting from the neighbours. + graphSearch: + for (var i = 0; i < queue.size(); i++) { + var enqueuedNode = queue.get(i); + for (var neighbour : enqueuedNode.neighbours) { + var nodeSet = Nullability.assertNonNull(enqueuedNode.currentSet).find(); + + // The neighbour has no set and so has not been visited yet. Add it to the current set and enqueue + // it to be visited. + if (neighbour.currentSet == null) { + nodeSet.addNode(neighbour); + queue.add(neighbour); + continue; + } + + // Otherwise, take the union of the two nodes' sets if needed. If we've only got a single node set + // left, then we know the whole graph is network is connected (even if not all nodes have been + // visited) and so can abort early. + var neighbourSet = neighbour.currentSet.find(); + if (nodeSet != neighbourSet) { + var removed = nodeSets.remove(NodeSet.merge(nodeSet, neighbourSet)); + assert removed : "Merged set should have been "; + if (nodeSets.size() == 1) break graphSearch; + } + } + } + + // If we have a single subset, then all nodes are reachable - just clear the set and exit. + if (nodeSets.size() == 1) { + assert nodeSets.iterator().next().size() == queue.size(); + for (var neighbour : queue) neighbour.currentSet = null; - // If all nodes are reachable then exit. - if (reachable.size() == nodes.size()) { // Broadcast our simple peripheral changes removeSingleNode(wired, wiredNetwork); InvariantChecker.checkNode(wired); @@ -198,43 +265,46 @@ final class WiredNetworkImpl implements WiredNetwork { return true; } - // A split may cause 2..neighbours.size() separate networks, so we - // iterate through our neighbour list, generating child networks. - neighbours.removeAll(reachable); - var maximals = new ArrayList(neighbours.size() + 1); - maximals.add(wiredNetwork); - maximals.add(new WiredNetworkImpl(reachable)); + assert queue.size() == nodes.size() : "Expected queue to contain all nodes."; - while (!neighbours.isEmpty()) { - reachable = reachableNodes(neighbours.iterator().next()); - neighbours.removeAll(reachable); - maximals.add(new WiredNetworkImpl(reachable)); + // Otherwise we need to create our new networks. + var networks = new ArrayList(1 + nodeSets.size()); + // Add the network we've created for the removed node. + networks.add(wiredNetwork); + // And then create a new network for each disjoint subset. + for (var set : nodeSets) { + var network = new WiredNetworkImpl(new HashSet<>(set.size())); + set.setNetwork(network); + networks.add(network); } - for (var network : maximals) network.lock.writeLock().lock(); + for (var network : networks) network.lock.writeLock().lock(); try { // We special case the original node: detaching all peripherals when needed. wired.network = wiredNetwork; wired.peripherals = Map.of(); + wired.neighbours.clear(); - // Ensure every network is finalised - for (var network : maximals) { - for (var child : network.nodes) { - child.network = network; - network.peripherals.putAll(child.peripherals); - } + // Add all nodes to their appropriate network. + for (var child : queue) { + var network = Nullability.assertNonNull(child.currentSet).network(); + child.currentSet = null; + + child.network = network; + network.nodes.add(child); + network.peripherals.putAll(child.peripherals); } - for (var network : maximals) InvariantChecker.checkNetwork(network); + for (var network : networks) InvariantChecker.checkNetwork(network); InvariantChecker.checkNode(wired); // Then broadcast network changes once all nodes are finalised - for (var network : maximals) { + for (var network : networks) { WiredNetworkChangeImpl.changeOf(peripherals, network.peripherals).broadcast(network.nodes); } } finally { - for (var network : maximals) network.lock.writeLock().unlock(); + for (var network : networks) network.lock.writeLock().unlock(); } nodes.clear(); @@ -373,22 +443,4 @@ final class WiredNetworkImpl implements WiredNetwork { throw new IllegalArgumentException("Unknown implementation of IWiredNode: " + node); } } - - private static Set reachableNodes(WiredNodeImpl start) { - Queue enqueued = new ArrayDeque<>(); - var reachable = new HashSet(); - - reachable.add(start); - enqueued.add(start); - - WiredNodeImpl node; - while ((node = enqueued.poll()) != null) { - for (var neighbour : node.neighbours) { - // Otherwise attempt to enqueue this neighbour as well. - if (reachable.add(neighbour)) enqueued.add(neighbour); - } - } - - return reachable; - } } diff --git a/projects/common/src/main/java/dan200/computercraft/impl/network/wired/WiredNodeImpl.java b/projects/common/src/main/java/dan200/computercraft/impl/network/wired/WiredNodeImpl.java index d402053cf..27bed7a3a 100644 --- a/projects/common/src/main/java/dan200/computercraft/impl/network/wired/WiredNodeImpl.java +++ b/projects/common/src/main/java/dan200/computercraft/impl/network/wired/WiredNodeImpl.java @@ -27,6 +27,14 @@ public final class WiredNodeImpl implements WiredNode { final HashSet neighbours = new HashSet<>(); volatile WiredNetworkImpl network; + /** + * A temporary field used when checking network connectivity. + * + * @see WiredNetworkImpl#remove(WiredNode) + */ + @Nullable + NodeSet currentSet; + public WiredNodeImpl(WiredElement element) { this.element = element; network = new WiredNetworkImpl(this);