SpaceWalkerRS / alternate-current

An efficient and non-locational redstone dust implementation
MIT License
203 stars 12 forks source link

0-tick block conveyor broke with AC #22

Open RinixGG opened 1 year ago

RinixGG commented 1 year ago

Minecraft 1.19.2, Quilt 0.18.2

After installing AC this 0-tick conveyor broke down изображение изображение

SpaceWalkerRS commented 1 year ago

Thank you for the report! It looks like you are using this smart piston design, which is unfortunately locational, so even in vanilla it will break in some locations. I would suggest switching to this design instead. It is not locational and works with AC as well.

I have previously considered changing AC's update order to fix this specific smart piston design, but have always been hesitant as changing update order to fix one thing will inevitably break something else. What I'm now considering is making multiple update order settings available that you can switch between with a command. Something to look out for in a future update.

RinixGG commented 1 year ago

Thank you for the report! It looks like you are using this smart piston design, which is unfortunately locational, so even in vanilla it will break in some locations. I would suggest switching to this design instead. It is not locational and works with AC as well.

I have previously considered changing AC's update order to fix this specific smart piston design, but have always been hesitant as changing update order to fix one thing will inevitably break something else. What I'm now considering is making multiple update order settings available that you can switch between with a command. Something to look out for in a future update.

Oh, thank you for advice. Recently I have issue with this design in creative world and can't understand what's wrong. tysm!

Artoria2e5 commented 1 year ago

it might be a good idea to have some issue tags at the point so people know why at a glance. For example, these smart pistons can use the tag "directional circuit". The TNT one can use "update count". There was a closed one that can use "locational circuit" or something.

paperky commented 9 months ago

I can confirm that this is still present with the listed smart piston design not working. Another potential fix is this pusher, for space constrained applications, however I don't think this works in Vanilla in the same orientations: ( If you need it shorter, you can replace the block above the piston with a target block, and lower the top-right polished diorite with dust on top of it)

alternative piston layout

However, in the ilmango video linked earlier, mango mentions the following (timestamp link)

So basically, the idea is now if you have a piston on the bottom and the top, it should always work. Another idea would be to put a piston here in the back, but as it turns out, this is not necessary. I tried it with 1,000 of those smart pistons in all 4 directions so we got 250 of those smart pistons in every direction and it always worked; so, it's enough to have a piston at the top and bottom in order to guarantee that this design always works.

Mango then describes how lag can be reduced by removing any of the auxiliary pistons which do not fire, which would cause locationality. While n=250 is not every location, it's a somewhat thorough test, and is at least a testament to this not being as bad as many other locational piston devices. This video is also prior to a large number of significant redstone changes, being as of writing about 5 years old.

@SpaceWalkerRS Can you go into detail over the redstone update order that has been affected, and link to the file in the repo? I'd also like to know of any examples of these zero tick block conveyors that are locational in vanilla. I'm not certain it can't happen, but it is rare in my experience. Typically, these pushers are used in tree farms which are already directional due to the nature of sapling growth, so a directional circuit doesn't typically cause problems: rotating the farm would already break fundamental circuits.

SpaceWalkerRS commented 9 months ago

While Ilmango's video looks convincing, there is a hidden assumption behind the test that he did: that the locationality is uniformly distributed. If that were the case, then a sample size of 1000 would tell you the failure rate is less than 1 in 1000, and you'd safely bet on it being fully reliable. But locationality is not uniformly distributed, and so this sample size of 1000 is quite meaningless.

The source of locationality is this method in RedStoneWireBlock, called from its neighborChanged implementation:

    private void updatePowerStrength(Level level, BlockPos pos, BlockState state) {
        int targetStrength = this.calculateTargetStrength(level, pos);
        if (state.getValue(POWER) != targetStrength) {
            if (level.getBlockState(pos) == state) {
                level.setBlock(pos, state.setValue(POWER, targetStrength), UPDATE_CLIENTS);
            }
            HashSet<BlockPos> notifiers = Sets.newHashSet();
            set.add(pos);
            for (Direction dir : Direction.values()) {
                set.add(pos.relative(dir));
            }
            for (BlockPos notifier : notifiers) {
                level.updateNeighborsAt(notifier, this);
            }
        }
    }

The first few lines deal with updating the power level of the wire, but it's the last few lines that give rise to locationality, and it's the code that deals with updating neighboring blocks of the wire.

HashSet<BlockPos> notifiers = Sets.newHashSet();
set.add(pos);
 for (Direction dir : Direction.values()) {
    set.add(pos.relative(dir));
}
for (BlockPos notifier : notifiers) {
    level.updateNeighborsAt(notifier, this);
}

And in Level, updateNeighborsAt looks like this:

    public void updateNeighborsAt(BlockPos pos, Block block) {
        this.neighborChanged(pos.west(), block, pos);
        this.neighborChanged(pos.east(), block, pos);
        this.neighborChanged(pos.below(), block, pos);
        this.neighborChanged(pos.above(), block, pos);
        this.neighborChanged(pos.north(), block, pos);
        this.neighborChanged(pos.south(), block, pos);
    }

The culprit here is the hash set. First seven BlockPoss are added to it, that of the wire's own position, and those of its six direct neighbors. These are what we call the 'notifiers'. Then it iterates over these notifiers, and calls level.updateNeighborsAt on each of them, which will cause block updates at the six direct neighbors of the notifier.

Thus we are at the mercy of the hash set's iterator() implementation as well BlockPos' hashCode implementation, that is what determines the update order of any given position. And unfortunately that means it can change based on the Java version and vendor you use. The same circuitry built in the same exact coordinates could work differently on different computers. Earthcomputer once did a survey of the update order across an entire Minecraft world, and some update orders are exceedingly rare. But because of the non-uniform distribution, a small local test of 1000 cannot tell you much about if a circuit is reliable.

As for Alternate Current, the relevant code is in this method. While changing the update order to fix this is simple, I've been hesitant because fixing the update order for this circuit will break other circuits. So for the time being I've chosen to keep it the same so at least circuits built with Alternate Current do not randomly break with the next update.

One solution I see is that I build multiple update orders into Alternate Current (or even a fully customizable update order through commands), so people can choose the one they like best.

paperky commented 9 months ago

I appreciate this wealth of information! To see if I understand the issue: (is this why the hash map is referenced so frequently?) It is undefined for what order the hash set's default iterator() (used with for-each type loops) will iterate through the elements relative to the order they were added to the hash set, so different implementations of the JVM-provided(?) hash set may cause different behavior. Additionally, due to the hash of a notifier changing with the location it represents, using a hash set inherently causes locationality. AC fixes this by entirely doing away with the hash set and instead intentionally enqueuing and dequeuing notifiers (or an equivalent) with a priority queue (for direction preference) and a simple, faster queue (for when performance matters more than order, or when order doesn't matter) Is the above correct?

I re-read the README and realized that I'd missed quite a lot of useful information, especially the intuitive "power flows in the direction it was already coming" part; honestly quite a nice QOL improvement.

As a sidenote, I appreciate the stability of AC that comes from things like these:

I have decided to keep a directional element in ambiguous cases, rather than to introduce randomness I've chosen to keep it the same so at least circuits built with Alternate Current do not randomly break with the next update Avoiding

  • breaking changes
  • introducing randomness are admirable qualities of a mod like this, especially one released as open source.

A question about a comment in WireHandler.java

This part of the algorithm was heavily inspired by theosib's 'RedstoneWireTurbo', which you can read more about in theosib's comment on Mojira here or by checking out its implementation in carpet mod

Does this mean that the changes relevant to specifically these piston mechanisms are also found in Carpet's fastRedstoneDust implementation? I've been unable to test (yet) on a server running AC, but the following circuit at this location behaves differently depending on whether fastRedstoneDust (frd) is enabled or disabled: (the cobblestone is top slabs, the repeater is to remove player tick effects): pistonDemo

Consistent with the idea of intuitive "flow-forward" mechanics, with frd on, the piston at the "end" of the redstone line is activated first. With frd off, the piston nearer the bottom of the screenshot fires first in this specific configuration. Am I understanding the intent and implementation of AC correctly, and does this match your expectations if AC was running?

Looking at this issue, there's an example of a zero tick generator which is reliable in AC, but from my testing it does not seem to work in Carpet using fastRedstoneDust, so there appears to be an implementation difference not encountered by the setup I created. Do you know what this might be?

Screenshot taken using: Minecraft Version: 1.20.4 (Fabric version 0.15.2) Modded with Fabric API, carpet, minihud, malilib Operating System: Windows 10 (amd64) version 10.0 Java Version: 17.0.2, Oracle Corporation Java VM Version: OpenJDK 64-Bit Server VM (mixed mode, sharing), Oracle Corporation

(Please let me know if there's a better place for me to continue this conversation)

SpaceWalkerRS commented 9 months ago

Additionally, due to the hash of a notifier changing with the location it represents, using a hash set inherently causes locationality. AC fixes this by entirely doing away with the hash set and instead intentionally enqueuing and dequeuing notifiers (or an equivalent) with a priority queue (for direction preference) and a simple, faster queue (for when performance matters more than order, or when order doesn't matter) Is the above correct?

That's precisely correct! This is the core issue behind locationality. The different hash set implementations baed on the Java vendor is just salt in the wound, as it were. It means you cannot even rely on sending a world save and expect it to work the same on another computer (though it plenty cases it will turn out it does, as there aren't that many different Java vendors with wildly different hash set implementations).

As a sidenote, I appreciate the stability of AC that comes from things like these: [...] are admirable qualities of a mod like this, especially one released as open source.

Thank you, that's very kind.

A question about a comment in WireHandler.java

This part of the algorithm was heavily inspired by theosib's 'RedstoneWireTurbo', which you can read more about in theosib's comment on Mojira here or by checking out its implementation in carpet mod

Does this mean that the changes relevant to specifically these piston mechanisms are also found in Carpet's fastRedstoneDust implementation?

I wasn't able to take anything directly from RedstoneWireTurbo, as I don't really understand its algorithm. I did try to decipher it when I started working on AC to get some ideas, but I didn't get much further than a few ideas that I got from its documentation (like that it was a BFS algorithm) and I decided to re-invent the wheel instead and start from scratch. But one other nugget of information I kept in mind was that it tried to determine the direction of information flow, and use that to derive the power propapgation and update order. And so I took that idea and developed it further once I had something working with AC. I don't know the specifics of RedstoneWireTurbo, so I cannot explain how it differs from AC's implementation, but undoubtedly there will be a different propagation order, and a different block update order.

In the case of the smart piston design (the one in Ilmango's video), there is only one piece of dust involved, so propagation order is irrelevant. The block update order is a bit arbitrary, but I settled on something that I thought was a natural extension of the default shape update order in Vanilla (-X+X-Z+Z-Y+Y). As for RedstoneWireTurbo, I can't be certain, but this method defines the order of neighbors around a wire (with the flow direction not yet applied), and I think that will be the block update order around a single wire as well.

Consistent with the idea of intuitive "flow-forward" mechanics, with frd on, the piston at the "end" of the redstone line is activated first. With frd off, the piston nearer the bottom of the screenshot fires first in this specific configuration. Am I understanding the intent and implementation of AC correctly, and does this match your expectations if AC was running?

The intuitive explanation is a little more abstract than the actual implementation. The idea is that power propagates outward from power sources. For example, if you have a long redstone line that is powered at both ends, and you remove power at one end, then the wires closer to the still powered end will decrease in power first, since they're closer to the power source. This is perhaps not intuitive, as you might expect changes to radiate outward from the point where you update the line. To get a little more technical, wires are update in order of their new power level. All the wires that goto power 0 are additionally sorted based on their prior power level (this makes it so networks "power on" in the same order as they "power off").

Looking at https://github.com/SpaceWalkerRS/alternate-current/issues/18, there's an example of a zero tick generator which is reliable in AC, but from my testing it does not seem to work in Carpet using fastRedstoneDust, so there appears to be an implementation difference not encountered by the setup I created. Do you know what this might be?

Similar to the smart piston design, this depends entirely on the block update order that is used. For AC, that is defined here, and for RedstoneWireTurbo, that is defined here.

tesseract-two commented 9 months ago

smart-piston Here's a more compact design that works with AC.

SpaceWalkerRS commented 9 months ago

I might be wrong, but I think that design is directional with AC - though I'm unable to test it at this moment

tesseract-two commented 9 months ago

Oops yup, it only works facing east/west.

SpaceWalkerRS commented 5 months ago

I've released a new beta for 1.8.0 that adds a new feature to resolve this issue. Changing the update order to vertical_first_outward or vertical_first_inward will make this smart piston design work. Let me know if you encounter issues with this beta or if new problems pop up.