Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix redstone neighbor update order differing from vanilla #1866

Open
wants to merge 2 commits into
base: 1.21.x
Choose a base branch
from

Conversation

embeddedt
Copy link
Member

@embeddedt embeddedt commented Jan 18, 2025

Fixes #1863. We now update in the horizontal plane order, and then the vertical plane order. Technically we still emit updates on the vertical axis, while vanilla does not, but this should still be much closer to vanilla behavior. We also don't send vertical updates to vanilla comparators anymore.

Suggestions would be appreciated on where to put NEIGHBOR_UPDATE_LIST.

Most credit goes to 2No2Name for the code analysis.

@embeddedt embeddedt added bug A bug or error 1.21.4 Targeted at Minecraft 1.21.4 labels Jan 18, 2025
@neoforged-pr-publishing
Copy link

neoforged-pr-publishing bot commented Jan 18, 2025

  • Publish PR to GitHub Packages

Last commit published: a24773c091c0641cbfdf204b29ae3e38d8511475.

PR Publishing

The artifacts published by this PR:

Repository Declaration

In order to use the artifacts published by the PR, add the following repository to your buildscript:

repositories {
    maven {
        name 'Maven for PR #1866' // https://github.com/neoforged/NeoForge/pull/1866
        url 'https://prmaven.neoforged.net/NeoForge/pr1866'
        content {
            includeModule('net.neoforged', 'testframework')
            includeModule('net.neoforged', 'neoforge')
        }
    }
}

MDK installation

In order to setup a MDK using the latest PR version, run the following commands in a terminal.
The script works on both *nix and Windows as long as you have the JDK bin folder on the path.
The script will clone the MDK in a folder named NeoForge-pr1866.
On Powershell you will need to remove the -L flag from the curl invocation.

mkdir NeoForge-pr1866
cd NeoForge-pr1866
curl -L https://prmaven.neoforged.net/NeoForge/pr1866/net/neoforged/neoforge/21.4.66-beta-pr-1866-neighbor-update-order/mdk-pr1866.zip -o mdk.zip
jar xf mdk.zip
rm mdk.zip || del mdk.zip

To test a production environment, you can download the installer from here.

Copy link
Member

@Technici4n Technici4n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this is a fine place to have the constant. Can you add a comment along the lines of // Neo: Also send update to vertical directions?

@2No2Name
Copy link

2No2Name commented Jan 18, 2025

The issue also includes the vertical updates existing, which is detectable and different from vanilla. I think in some reasonable setups this might occur accidentally, breaking contraptions. (Comparator update detector above or below some other inventories.)

This setup updates the detector with neoforge, but not in vanilla.
2025-01-18_11 49 21

For some reason placing the comparator directly on the hopper doesn't trigger the behavior, I don't understand why.

@2No2Name
Copy link

For reasoning on what's happening and what I suggest to do: #1863 (comment)

@embeddedt
Copy link
Member Author

PR has been updated, as per feedback, to exclude vanilla comparators from the vertical updates.

@embeddedt embeddedt requested a review from Technici4n January 18, 2025 15:19
@2No2Name
Copy link

2No2Name commented Jan 18, 2025

For me it is confusing that these two different update methods exist, meaning
blockstate.onNeighborChange(this, blockpos, p_46718_); and
this.neighborChanged(blockstate, blockpos, p_46719_, null, false);
What's the reasoning for these being different calls?

blockstate.onNeighborChange seems to a neoforge method that by default only comparators implement.
The second method seems to be the vanilla method

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.21.4 Targeted at Minecraft 1.21.4 bug A bug or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Comparator update order different from vanilla
3 participants