Effects api 2

[paulevsGitch]

A replacement for PR #117 since the old branch was damaged by multiple changes.

[paulevsGitch]

I redesigned API a bit, now instead of using fixed time effects you can specify the time when adding effect to entity, similar to modern (probably)

[calmilamsy]

Otherwise, I'm happy to see this merged.

[mineLdiver]

Might cause some load order issues because InitEvent is called very early on, not sure though.

[paulevsGitch]

What kind of issues?

[mineLdiver]

I'm not exactly sure since this is a rather open situation, but there's this piece of documentation from the deprecated PostInitEvent that was replaced with POST_INIT_PHASE:

Minecraft classes can not be referenced during this event.

The reason is that InitEvent is posted on Fabric's PreLaunchEntrypoint, so, before the game is actually started by Fabric Loader.
This could lead to potential very hard to debug issues if an effect class references some parts of Minecraft in its static initializer, causing Minecraft classes to load prematurely.
I'm not completely sure what'd be a good alternative to InitEvent though. Maybe Fabric's own ModInitializer? This one is initialized after most of the game's setup has been done.

[calmilamsy]

Might be an idea to add a "stapi init last step" event.

[calmilamsy]

Literally just before initializationfinished.

[mineLdiver]

No point in such an event really when ModInitializer is already a thing,

[paulevsGitch]

if an effect class references some parts of Minecraft in its static initializer

Static initialiser will be not called until at least one method of target class is called, and lambda is not that method, so any static block will be created only when the first instance of effect will appear. You can easily check that by adding static block to any effect like this:

static {
	System.out.println("\n\nEFFECT\n\n");
}

And until at least one effect instance is created that will be zero:

So I don't see any problems here at all

[mineLdiver]

The event listener could also be poorly coded, and it's not really obvious that you shouldn't reference a Minecraft class from an event listener that registers an in-game object. Moving it to a later point in the init sequence is simple and foolproof enough.

[paulevsGitch]

This is not different from other registry events, you also should not call most of game classes in them since otherwise you will just break the game.

The event listener could also be poorly coded

This is a problem of specific listener implementation, not an API. If I will call Minecraft client on server that will be my fault that I did that, not an API that allowed me to do that.

I guess that the simplest solution is just directly specify in Javadocs that game classes should be not called in this event to prevent issues

[mineLdiver]

This still doesn't address the problem of game content (entity effects) being registered before the game even starts initializing. It likely won't cause actual crashes, but can cause confusion.

[mineLdiver]

I think I'll take over from here.

A few things I want to do:

1. Change fastutil's Pair to DFU's Pair.
2. Make the effects registry synchronized and use effect raw IDs in the packets.
3. Change tabs to spaces.
4. Change InitEvent to ModInitializer.
5. Make StAPI's internal listeners private.

If you have no objections to this, I'll start working.

I'd also like to discuss potentially introducing EntityEffectType, similar to PacketType. The problem is that EntityEffectFactory isn't really extendable since it's just an interface, so introducing EntityEffectType would future-proof the API design and make more sense for the registry entry type. It might not be required right now, but it's not a bad thing to have.
I can implement it myself if you're not against it. And, again, this is up for discussion, you're the author of the pull request after all.

[mineLdiver]

Found a bug - StationEffectEntity#removeAllEffects doesn't invoke EntityEffect#onRemoved.

Gonna fix it.

[mineLdiver]

Forgot that fastutil also has pairs for primitives. Using that instead of DFU's pair.

[mineLdiver]

I think I've figured out a typesafe way to have generic factories while not being too complicated for simple effects.

public class TestPlayerEffect extends EntityEffect<PlayerEntity, TestPlayerEffect> {
    public static final EntityEffect.Type<PlayerEntity, TestPlayerEffect> TYPE = Type
            .builder(TestPlayerEffect::new, entity -> entity instanceof PlayerEntity player ? player : null)
            .build();

    private int originalHealth;
    
    protected TestPlayerEffect(PlayerEntity entity, int ticks) {
        super(entity, ticks);
    }
    
    // ...

The instanceof check lambda is optional, there's another builder method which only takes the factory, in which case all generic types default to basic Entity and don't allow factories which take a subclass of Entity.

In the type class there will be a tryCreate method like this:

public Optional<EFFECT> tryCreate(Entity entity, int ticks) {
    return Optional
            .ofNullable(entityFilter.apply(entity))
            .map(filteredEntity -> factory.create(filteredEntity, ticks));
}

entityFilter being the optional instanceof check (or Function#identity if the former isn't provided).

Overall, this should make for an easy to use, typesafe, reflectionless, and foolproof API. Thoughts?

[mineLdiver]

Three bugs:

1. Metadata doesn't synchronize between server and client. Rejoining a server with an effect causes the client-side metadata to revert to default values, causing undefined behavior, for example TestPlayerEffect setting player's health to 0 client-side on getting removed, showing the respawn screen, while the server sets it back to the original health from the saved NBT value, putting the player in a glitched state.
2. SendAllEffectsPacket is never sent.
3. SendAllEffectsPacket is incorrectly marked as serverBound, though expecting client-side on apply and being sent from the server.

Working on it.

[mineLdiver]

Correction: SendAllEffectsPacket isn't never sent, rather it's only sent to other players when you join with an effect, or to you when you join a server with a player that has an effect. Which then causes bad packet ID on the receiving clients because of the wrong marking.

[mineLdiver]

Nevermind above correction, it's supposed to work like that, I didn't fully understand its purpose by that time.
However, this unveiled a different issue - all packets are incorrectly marked as serverBound while expecting client on apply.