Move certain events to the newly introduced ServerPlayerEvent

[Spinoscythe]

Some modders in the community noticed that some PlayerEvent events, like PlayerEvent.PlayerLoggedInEvent, only ever pass in a ServerPlayer instance. However, since PlayerEvent is a Player type, you have to cast the player back to ServerPlayer.

It would be worth switching to a <? extends Player> generic type for player events, but we don't support generic events anymore. So this seems the most ideal.

Some modders = @LeScooter. ty for the idea :)
Also @Technici4n approves this idea.

[neoforged-pr-publishing]

• Publish PR to GitHub Packages

[Shadows-of-Fire]

This change doesn't really make sense. If we're going to make breaking changes to these events, they should be moved to their own individual files. There isn't any reason for ServerPlayerEvent to exist.

[Spinoscythe]

This change doesn't really make sense. If we're going to make breaking changes to these events, they should be moved to their own individual files. There isn't any reason for ServerPlayerEvent to exist.

I think having ServerPlayerEvent saves time from casting getEntity in every event to ServerPlayer

[neoforged-compatibility-checks]

@Spinoscythe, this PR introduces breaking changes.
Fortunately, this project is currently accepting breaking changes, but if they are not intentional, please revert them.
Last checked commit: 288e628cf04d3d30572cd5df12fa6c4fc6b675a0.

Compatibility checks

neoforge (:neoforge)

• net/neoforged/neoforge/event/entity/player/PlayerEvent$PlayerLoggedInEvent
  • ❗ API class no longer exists
• net/neoforged/neoforge/event/entity/player/PlayerEvent$PlayerRespawnEvent
  • ❗ API class no longer exists
• net/neoforged/neoforge/event/entity/player/AdvancementEvent$AdvancementProgressEvent
  • <init>(Lnet/minecraft/world/entity/player/Player;Lnet/minecraft/advancements/AdvancementHolder;Lnet/minecraft/advancements/AdvancementProgress;Ljava/lang/String;Lnet/neoforged/neoforge/event/entity/player/AdvancementEvent$AdvancementProgressEvent$ProgressType;)V: ❗ API method was removed
• net/neoforged/neoforge/event/entity/player/PlayerEvent$PlayerLoggedOutEvent
  • ❗ API class no longer exists
• net/neoforged/neoforge/event/entity/player/PlayerContainerEvent$Close
  • <init>(Lnet/minecraft/world/entity/player/Player;Lnet/minecraft/world/inventory/AbstractContainerMenu;)V: ❗ API method was removed
• net/neoforged/neoforge/event/entity/player/PlayerSetSpawnEvent
  • <init>(Lnet/minecraft/world/entity/player/Player;Lnet/minecraft/resources/ResourceKey;Lnet/minecraft/core/BlockPos;Z)V: ❗ API method was removed
• net/neoforged/neoforge/event/entity/player/PlayerEvent$PlayerChangeGameModeEvent
  • ❗ API class no longer exists
• net/neoforged/neoforge/event/entity/player/PlayerContainerEvent
  • ❗ Class was made abstract
  • <init>(Lnet/minecraft/world/entity/player/Player;Lnet/minecraft/world/inventory/AbstractContainerMenu;)V: ❗ API method was removed
• net/neoforged/neoforge/event/entity/player/PlayerContainerEvent$Open
  • <init>(Lnet/minecraft/world/entity/player/Player;Lnet/minecraft/world/inventory/AbstractContainerMenu;)V: ❗ API method was removed
• net/neoforged/neoforge/event/entity/player/PlayerEvent$PlayerChangedDimensionEvent
  • ❗ API class no longer exists
• net/neoforged/neoforge/event/entity/player/AdvancementEvent
  • <init>(Lnet/minecraft/world/entity/player/Player;Lnet/minecraft/advancements/AdvancementHolder;)V: ❗ API method was removed
• net/neoforged/neoforge/common/NeoForgeEventHandler
  • playerLogin(Lnet/neoforged/neoforge/event/entity/player/PlayerEvent$PlayerLoggedInEvent;)V: ⚠ API method was removed
• net/neoforged/neoforge/common/CommonHooks
  • onChangeGameType(Lnet/minecraft/world/entity/player/Player;Lnet/minecraft/world/level/GameType;Lnet/minecraft/world/level/GameType;)Lnet/minecraft/world/level/GameType;: ❗ API method was removed
• net/neoforged/neoforge/event/EventHooks
  • onAdvancementProgressedEvent(Lnet/minecraft/world/entity/player/Player;Lnet/minecraft/advancements/AdvancementHolder;Lnet/minecraft/advancements/AdvancementProgress;Ljava/lang/String;Lnet/neoforged/neoforge/event/entity/player/AdvancementEvent$AdvancementProgressEvent$ProgressType;)V: ⚠ API method was removed
  • onAdvancementEarnedEvent(Lnet/minecraft/world/entity/player/Player;Lnet/minecraft/advancements/AdvancementHolder;)V: ⚠ API method was removed
  • onPlayerSpawnSet(Lnet/minecraft/world/entity/player/Player;Lnet/minecraft/resources/ResourceKey;Lnet/minecraft/core/BlockPos;Z)Z: ❗ API method was removed
  • firePlayerChangedDimensionEvent(Lnet/minecraft/world/entity/player/Player;Lnet/minecraft/resources/ResourceKey;Lnet/minecraft/resources/ResourceKey;)V: ❗ API method was removed
  • firePlayerLoggedIn(Lnet/minecraft/world/entity/player/Player;)V: ❗ API method was removed
  • firePlayerLoggedOut(Lnet/minecraft/world/entity/player/Player;)V: ❗ API method was removed
• net/neoforged/neoforge/event/entity/player/AdvancementEvent$AdvancementEarnEvent
  • <init>(Lnet/minecraft/world/entity/player/Player;Lnet/minecraft/advancements/AdvancementHolder;)V: ❗ API method was removed

[Caltinor]

This change doesn't really make sense. If we're going to make breaking changes to these events, they should be moved to their own individual files. There isn't any reason for ServerPlayerEvent to exist.

I think having ServerPlayerEvent saves time from casting getEntity in every event to ServerPlayer

I noticed a code format difference. In subclasses of LivingEvent which return a specific entity type (namely PlayerEvent and AnimalTameEvent), they each take the specific entity class for their event class name (Player and Animal respectively), pass that into super but also store it locally and pass it back via the getEntity override.

public class ExampleEntityEvent extends LivingEvent {
  private final ExampleEntity example;
  
  public ExampleEntityEvent(ExampleEntity example) {
    super(example);
    this.example = example;
  }

  @Override
  public ExampleEntity getEntity() {return example;}
}

This implementation in contrast just casts the super invocation. Whether this or the other classes change, I think we should be consistent.

[TelepathicGrunt]

When you can, please implement Caltinor's suggestion. It would remove the need for the casting in the server player events

[Caltinor]

I think this c7cf777 is the opposite of what @TelepathicGrunt wanted. They wanted to remove the casts, from how I read it.

[TelepathicGrunt]

Yeah. Less casting. Not more

[Shadows-of-Fire]

Per a conversation in the NF discord:

Introducing a generic to EntityEvent allows us to streamline more than just the server player events:
We'd be able to drop LivingEvent, ItemEvent, and PlayerEvent just by having that generic there. We introduce some redundancy where we may end up having EntityEvent<Entity> (which would be a nontrivial number of cases), but there's also a benefit: Authors of events that are children of EntityEvent<T> should now be thinking about what the correct T is for the situation (so we don't end up with events that are too lenient as we have now).

Based on that, this PR is not something we want to proceed with at this time, and the refactor triggered by switching to EntityEvent<T> is large enough it warrants its own new PR.