Merge EventHooks into CommonHooks

[Technici4n]

The split is a bit artificial. Might as well have all the "misc hooks" in a single class.

Draft until #588 is merged.

[neoforged-pr-publishing[bot]]

• [x] Publish PR to GitHub Packages

Last commit published: f48f0a227d9e7001c8206cd85341ec0fc131a0f8.

PR Publishing

The artifacts published by this PR:

• :package: net.neoforged:neoforge:20.5.17-beta-pr-875-merge-event-common-hooks
• :package: net.neoforged:testframework:20.5.17-beta-pr-875-merge-event-common-hooks

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 #875' // https://github.com/neoforged/NeoForge/pull/875
        url 'https://prmaven.neoforged.net/NeoForge/pr875'
        content {
            includeModule('net.neoforged', 'neoforge')
            includeModule('net.neoforged', 'testframework')
        }
    }
}

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-pr875.
On Powershell you will need to remove the -L flag from the curl invocation.

mkdir NeoForge-pr875
cd NeoForge-pr875
curl -L https://prmaven.neoforged.net/NeoForge/pr875/net/neoforged/neoforge/20.5.17-beta-pr-875-merge-event-common-hooks/mdk-pr875.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.

[TelepathicGrunt]

You still looking to do this PR? It'll need to be updated to 1.21 and merge conflicts resolved

[Technici4n]

Feel free to do it! It's not high on my priority list. In any case, we should wait for the next release cycle I think.

[TelepathicGrunt]

Gonna label this for 1.21.1 for now. If 1.21.1 is non-breaking mc version, we can keep pushing this down the road till we know for sure what mc version is super breaking that this PR would be a good candidate for

[KnightMiner]

If this is definitely getting merged next breaking cycle, its probably worth adding documentation to EventHooks suggesting new PRs place hooks in common to minimize effort to move this later. Deprecation is probably the wrong approach as callers cannot change what they do, just PRs

[HenryLoenwind]

This can be implemented non-breakingly by moving all methods over and replacing them with extends CommonHooks. Those methods are static but not final, so the INVOKESTATIC will happily pick them up from the new superclass. Preview: https://github.com/HenryLoenwind/NeoForge/commit/286d60cf6fe2dc814338fee9dbea8a4bfc6d702d (compiles but completely untested.)

However, this will give a class with 2656 lines, which may be a bit unwieldy. CommonHooks already is more of a CommonMergeConflict. It may be more prudent to move the event hooks into the actual event classes instead of throwing them all into one big unstructured pile. (This can also be done non-breakingly by leaving behind a bouncer method, which IDE refactoring can do automatically.)

[Caltinor]

I agree with Henry on wanting to avoid a behemoth class which will result in a lot of merge conflicts, however, I also recognize the need to consolidate our hooks into one place. What if instead of consolidating them into a single class, we consolidated them into a single package? (net.neoforged.neoforge.patchhooks?) We could then have multiple classes in the package to compartmentalize related methods. Also, we use the word "common" a lot with sidedness so getting away from "Common"Hooks is probably good for semantic consistency.

[marchermans]

This split was done by design..... In the past we explicitly had a difference here because of optifine. The hooks in this class where there to indicate that they are needed for the event system.

I am pretty sure we did not really enforce that so it can be reverted, but it is not arbitrary.

[neoforged-automation[bot]]

@Technici4n, this pull request has conflicts, please resolve them for this PR to move forward.