[1.20.5] Refactor tick events into distinct subclasses

[Shadows-of-Fire]

What

This PR refactors TickEvent and all child classes into individual abstract classes with Pre/Post subclasses. The number of events being fired and the timing of the events is unchanged, but the notion of one event that fires twice per tick has been removed, and the events have been moved out of a single file.
Additionally, all the events have been documented.

The list of changes is:

• TickEvent.ServerTickEvent -> ServerTickEvent.Pre/Post
• TickEvent.ClientTickEvent -> ClientTickEvent.Pre/Post
• TickEvent.LevelTickEvent -> LevelTickEvent.Pre/Post
• TickEvent.PlayerTickEvent -> PlayerTickEvent.Pre/Post
• TickEvent.RenderTickEvent -> RenderFrameEvent.Pre/Post (naming changed here as this is unrelated to ticks).
• LivingEvent.LivingTickEvent -> EntityTickEvent.Pre/Post (Pre retains the cancellable nature of the original event) [expanded to all entities].

Why

These events are more than a decade old, have no documentation, and easily confuse users. On numerous occasions have I had to inform users that all tick events fire twice per tick.

I could simply document this in the classes, but this would not remedy the issue that all information in the base class TickEvent is redundant, with the exception of TickEvent#phase (which is just confusing).

Instead, this refactor makes it obvious what event you are subscribing to and when it will fire, entirely removing the possibility that work is erroneously performed twice per tick.

There even exists a bug in one of the legacy tests where we are running a tick handler twice per tick due to not checking TickEvent#phase:

NeoForge/tests/src/main/java/net/neoforged/neoforge/oldtest/entity/GravityAttributeTest.java

Lines 63 to 70 in 34f85e1

	public void worldTick(TickEvent.LevelTickEvent event) {
	if (!event.level.isClientSide) {
	if (ticks++ > 60) {
	ticks = 0;
	Level w = event.level;
	List<LivingEntity> list;
	if (w.isClientSide) {
	ClientLevel cw = (ClientLevel) w;

[neoforged-pr-publishing]

• Publish PR to GitHub Packages

Last commit published: 7dc32e7b3961b3c4ae1224486bd1f49f02646863.

PR Publishing

The artifacts published by this PR:

• 📦 net.neoforged:neoforge:20.5.19-beta-pr-542-tick-event-refactor
• 📦 net.neoforged:testframework:20.5.19-beta-pr-542-tick-event-refactor

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 #542' // https://github.com/neoforged/NeoForge/pull/542
        url 'https://prmaven.neoforged.net/NeoForge/pr542'
        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-pr542.
On Powershell you will need to remove the -L flag from the curl invocation.

mkdir NeoForge-pr542
cd NeoForge-pr542
curl -L https://prmaven.neoforged.net/NeoForge/pr542/net/neoforged/neoforge/20.5.19-beta-pr-542-tick-event-refactor/mdk-pr542.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.

[marchermans]

Small tiny suggestion.

[Fuzss]

Maybe split LevelTick into client & server variants? Would get rid of the unused hasTime parameter client side and could provide ClientLevel and ServerLevel directly.
Fabric Api has it implemented this way, too, which is much cleaner imo.

Also maybe include the Minecraft instance for ClientTick as it fires from there and devs are likely to use it? (also like Fabric Api)

[Technici4n]

@Fuzss I thought about that, however many methods are already called on both client and server side, so modders should be used to handling both cases in common paths. It is also quite easy to check with an instanceof which kind of object it is.

Regarding Minecraft, it's already easily available via Minecraft.getInstance(), so I'm not sure it's worth adding to every client event.

[Minecraftschurli]

please prepare a blogpost and the text for an announcement since this is a rather big breaking change

[Technici4n]

Do we want to keep this for 1.20.5? (IMO yes)

[embeddedt]

Don't really see a reason to hold it back myself; it's a fairly trivial refactor in comparison to everything else we've done.

[Technici4n]

It's more a question of timing for me - we tried to do the most impactful refactors as soon as possible. This is coming a bit late and will require many people to release another version of their mods.

[Shadows-of-Fire]

Updated to account for LivingTickEvent and fix some things as noted by review. Will update main post to reflect.

[Shadows-of-Fire]

Rebased and re-targeted to port/1.20.5

resolved

[Fuzss]

Since this PR now also touches upon LivingTickEvent maybe it is a great opportunity to move the hook(s) for that?

Firing at the head of LivingEntity::tick has never been ideal, since vanilla is very inconsistent concerning when a subclass of LivingEntity calls super in the tick method. Some subclasses run code before calling super, resulting in cancelling the event not fully cancelling the whole tick as would be expected (and as is indicated by the javadoc).

Even more subclasses run code after calling super, which the new Post event misses out on, making it not too useful as it only allows for wrapping tick code in LivingEntity.

Why not move the event hooks to the call sites of LivingEntity::tick? This would involve three locations:

• ClientLevel::tickNonPassenger
• ServerLevel::tickNonPassenger
• Entity:rideTick

This approach would be similar to how ScreenEvent$Render is handled as it faces a comparable issue in how the concerning method can easily be overridden in subclasses. (not true anymore actually due to the introduction of Screen::renderWithTooltip in 1.19.3 which calls Screen::render now and is final)

It's unlikely for modders to call LivingEntity::tick, and if they really have to the can still manually invoke both events.

[Fuzss]

Also moving the hooks for both LivingTickEvents as outlined in the last comment would remove the need for having a dedicated tick event for players.

[Shadows-of-Fire]

Looking into moving it. Potentially also dropping Entity#canUpdate and expanding LivingTickEvent to EntityTickEvent. Waiting on some input from others before executing that implementation though, since it's a functional change rather than organizational. See also #829

[Fuzss]

Ah yes, expanding the event to EntityTickEvent would be great, didn't even realize my proposed change would open up the event to all entities. Vaguely remember a couple of cases where a tick event for non-living entities would've been great to have (I think that was related to item entities).

Maybe revert the current LivingTickEvent changes for now and move everything to a dedicated PR?

[Shadows-of-Fire]

Updated to remove Entity#canUpdate and expand LivingTickEvent to EntityTickEvent. The cancellation semantics of EntityTickEvent$Pre are still present that replace canUpdate. The PlayerTickEvent is still potentially necessary due to the semantics of ServerPlayer#doTick as noted in the javadoc.

[Fuzss]

Yeah ServerPlayer::doTick definitely needs custom handling, missed that earlier. But why not just fire both EntityTickEvents for that as well (either in ServerPlayer::doTick or in ServerGamePacketListenerImpl::tick) instead of having PlayerTickEvent?

Just a bit weird at the moment, since EntityTickEvent works perfectly fine for all player instances but ServerPlayer.

[Technici4n]

Looks very nice.