Event subscribtions in complete class hierarchy

[rubensworks]

While writing some event listener abstractions I noticed that the annotations @SubscribeEvent and @Mod.EventHandler are only checked inside the class of the object registered in the event bus. I was wondering if it would be somehow possible to allow for these annotations on all methods in the complete class hierarchy of these registered event listeners?

[LexManos]

Forge's event bus does check parents for @SubscribeEvent. Guava's does not.

[rubensworks]

Would it be possible to surround the loop at https://github.com/MinecraftForge/FML/blob/master/src/main/java/net/minecraftforge/fml/common/FMLModContainer.java#L319 with something like for(Class<?> clazz = mainClazz.getClass(); clazz != null; clazz = clazz.getSuperclass())?

This should enable parents to be checked for annotated methods as well. It might however be required for additional check to be done for overridden methods.

[LexManos]

I'm trying to think of why, forcing the main mod class to be a simple implementation is part of the design. What are you trying to do that would handle the state events with inherited code?

[rubensworks]

Currently I am implementing an abstraction for two different mods (Let's call them ModA and ModB). These two mods have a main @Mod class which inherit from a ModBase class. The reason for having this ModBase is to abstract away some common logic that is called for the events FMLServerStartedEvent and FMLServerStoppingEvent. But the problem I am facing now is that even when I annotate the event listeners for these with @Mod.EventHandler, they are not called because the ModA and ModB classes are registered as @Mod class instead of the ModBase class.

The current solution for this is inserting methods like the following in ModA and ModB's classes:

@Mod.EventHandler
@Override
public void onServerStarted(FMLServerStartedEvent event) {
    super.onServerStarted(event);
}

This is however not a very clean solution.

I think other library mods would also benefit from this increase in abstraction flexibility, when for example they would want to enforce event registration in some superclass method that is marked as final.

[RainWarrior]

You can always make the actual annotated base method call another (abstract) base method, which would be correctly resolved in subclasses :P

[rubensworks]

@RainWarrior The actual annotated base method would exist inside the ModBase class then if I understand correctly? This is actually what I attempted at first (and still hope to achieve), but extra annotated methods inside the subclasses are currently required for making the event listeners detectable by FML. The earlier mentioned loop for checking all parents would enable this wanted behavior.

[heldplayer]

Would be nice if FML checked parent classes. Right now I have an ASM transformer that generates the missing methods so that I don't have to write methods that simply call their supers.

[LexManos]

-.- I don't like "Add this or i'll coremod shit" threats -.-

[RainWarrior]

@rubensworks

@Mod
abstract class Parent
{
    @Mod.EventHandler
    public final void onServerStartedImpl(FMLServerStartedEvent event)
    {
        onServerStarted(event);
    }

    protected abstract void onServerStarted(FMLServerStartedEvent event);
}

class Child extends Parent
{
    @Override
    protected abstract void onServerStarted(FMLServerStartedEvent event)
    {
        // do stuff
    }
}

EDIT: Ah, I misunderstood the problem - this will work the other way around

[LexManos]

How about this, think your request through. Provide a compelling argument for it. Provide a functional PR that DOESN'T break mods. {Protip its not just what you've said} And don't come here threatening to coremod shit because you think you know better -.-

[cpw]

Hello. I designed this system 3 years ago. It doesn't propagate to parents because annotations don't propagate to parents. Java annotations are not, by default, inherited by subclasses. There's all sorts of good reasons for this - I suggest you go read the massive debate the JCP had back in about 2001 or so around this topic (yes, I am 100% aware there are mechanisms in the Annotation framework for this - they were added as a compromise).

The system is implemented such that it is familiar to Java developers who use annotations already. The triviality of the hack you complain about is just that- trivial. The amount of unexpected fallout - HUGE. In summary, I see no way you can have your cake and eat it too. I would suggest you accept how it is here, or find an alternative modding platform for minecraft.

By the way - if you coremod this in, I highly anticipate a huge amount of fallout - so, if you ever expect to play well in the Forge modding community, I wouldn't do that.

[rubensworks]

Ok, I have my answer. I wasn't aware of the fact that annotations aren't supposed to propagate to their parents, so I completely accept the design decision for doing it like this.

@LexManos @cpw I have the feeling that my request was offensive to you, this was not my intention at all. I simply wanted to bring up an issue I though could be improved upon. I have nothing more than respect for the platform you are providing.

And regarding coremods (and reflection for that matter), up until now I always preferred not to use this, because IMO they are very bad for mod interoperability if not done with care. So I'll keep using my current solution.

If one day this subject would be brought up again, I'd be happy to write up a PR for this. But for now, I'll close this issue.

[cpw]

@rubensworks you only "crossed the line" with your silly suggestion of coremodding this. @LexManos and I really have a love/hate relationship with coremods, and coremodding such a critical behaviour is bound to have really odd sideeffects across a large swath of the modding community. So we obviously got a little defensive when that was suggested..

Protip: If you don't like something, don't threaten to coremod it your way

[rubensworks]

This was probably a misunderstanding then, I haven't mentioned wanting to coremod anything for this (tbh, I have never even worked with ASM transformers). If I am correct, it was @heldplayer suggesting the use of ASM transformers. Trust me, I don't like coremods any more than you do ;)

[heldplayer]

To clarify, the ASM transformer only runs on classes extending my base mod class, and is used to prevent methods with only super calls. There is no way that I would be touching other mods' mod classes for such a thing.

However, take for example this example case: ModBase.java

public class ModBase {
    // Methods and fields that mods need

    @EventHandler
    public void preInit(FMLPreInitializationEvent event) {
        // Initialize and set up some values
    }

    public abstract String getSpecificThing();
}

ModTest.java

@Mod(modid = "inheritTest")
public class ModTest extends ModBase {
    @EventHandler
    public void init(FMLInitializationEvent event) {
        // Use methods and fields from parent class
    }

    @Override
    public String getSpecificThing() {
        return "this thing";
    }
}

In this case, you'd want the ModTest class to have the preInit method be executed so that it can use whatever it makes ready for use in its init method. However, to achieve this effect you would have to write the ModTest class to be:

@Mod(modid = "inheritTest")
public class ModTest extends ModBase {
    @Override
    @EventHandler
    public void preInit(FMLPreInitializationEvent event) {
        super.preInit(event);
    }

    @EventHandler
    public void init(FMLInitializationEvent event) {
        // Use methods and fields from parent class
    }

    @Override
    public String getSpecificThing() {
        return "this thing";
    }
}

The method "getSpecificThing" gets used by the parent class to prepare the values it needs and can be/do anything.

You would however also want to have the ability to override the methods to use a custom implementation. For example:

@Mod(modid = "inheritTest")
public class ModTest extends ModBase {
    @Override
    @EventHandler
    public void preInit(FMLPreInitializationEvent event) {
        // Do other cool stuff and possibly call the super function
    }

    @EventHandler
    public void init(FMLInitializationEvent event) {
        // Use methods and fields from parent class
    }

    @Override
    public String getSpecificThing() {
        return "this thing";
    }
}

[LexManos]

And what is wrong with doing standard java practices in this? You're saving yourself 4 lines of code by using ASM black magic. Do not do that. Annotations do not propagate upwards. Classes do not have the methods of their parents.

[heldplayer]

Indeed, annotations don't propagate upwards. However if class A has an annotated method, and class B has a different annotated method (it doesn't override the method from class A), wouldn't it make sense that when checking methods for annotations on class B the ones from class A that aren't overridden get included?

[LexManos]

No, that's the entire point of Annotations not propagating upwards.