Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conditional mods & event bus subscribers #212

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

wired-tomato
Copy link

@wired-tomato wired-tomato commented Nov 12, 2024

Conditional class loading described in #201

@DependsOn("modid")
@Mod("testmod")
//class loaded if modid is present
public class TestMod {
    public TestMod() { }
}
@DependsOn({"modid", "modid2"})
@EventBusSubscriber(modid = "testmod")
//class loaded if modid and modid2 are present
public class TestModEvents {
    public void configEvent(ModConfigEvent event) {}
}

@neoforged-pr-publishing
Copy link

  • Publish PR to GitHub Packages

@tmvkrpxl0
Copy link

tmvkrpxl0 commented Nov 12, 2024

Can you use @dependency without wrapping it in @ChainDependency? I'm quite sure most use-cases would be just depending on one single modid.

@tmvkrpxl0
Copy link

tmvkrpxl0 commented Nov 12, 2024

Uh oh I accidentally mentioned someone

@shartte
Copy link
Contributor

shartte commented Nov 12, 2024

I don't like how this is modeled. dependencies is not the right term to do conditional loading, and I think the annotation nesting soup is getting a bit out of hand...
It'll be some time until I can take a proper look at the impl too, but I think I'd prefer repeatable annotations separate from the top-level annotation used.

@Gaming32
Copy link
Contributor

Looking at the beginning of the impl, it seems you can use dependencies = @ChainDependency(@Dependency("modid")). Though that's still too verbose in my opinion.

@wired-tomato
Copy link
Author

wired-tomato commented Nov 12, 2024

Would a model like this be preferable?

@DependsOn(value = "neoforge", condition = Condition.ALL_PRESENT, operator = Operator.AND)
@DependsOn(value = {"othermod", "othermod2"}, condition = Condition.AT_LEAST_ONE_PRESENT, operator = Operator.OR)
@DependsOn("othermod3")
@Mod("testmod")
public class TestMod {
    public TestMod() {
        //called when neoforge and one of othermod or othermod2 are present or when othermod3 is present
    }
}

The least verbose one dependency would be @DependsOn("dependency")
This model also avoids changing the definitions of @Mod and @EventBusSubscriber

@shartte
Copy link
Contributor

shartte commented Nov 12, 2024

Why do you need such a complex expression language beyond simple AND/OR which could be expressed through a repeatable annotation / value array attribute?

@Technici4n
Copy link
Member

I think that OR conditions are quite suspicious if the goal is to avoid crashes because of missing classes.

@wired-tomato
Copy link
Author

The new model is comprised of AND/ORs and their respective negations
ORs are included because there are cases in which multiple mods provide the same public API
Negations are included because there are cases in which mod conflictions end up changing certain behaviors

@DependsOn("neoforge")
@DependsOn(value = { "othermod", "othermod2" }, condition = DependsOn.Operation.OR)
@DependsOn("othermod3")
@DependsOn.List({
        @DependsOn("neoforge"),
        @DependsOn(value = { "othermod", "othermod2" }, condition = DependsOn.Operation.OR),
        @DependsOn("othermod3")
})

Both of the preceding annotations will behave the same

Refer to the @DependsOn annotation class for further details

@Technici4n
Copy link
Member

Technici4n commented Nov 13, 2024

The main goal of checking for dependencies in the annotation like this, is to prevent accidentally loading code that references nonexisting classes. If you add NOT or OR dependencies, that flies out of the window, and you might as well just add the following code instead of writing convoluted annotations:

if (ModList.get().isModLoaded("a") && !ModList.get().isModLoaded("b")) {
    bus.register(AButNotBEventHandler.class);
}

@wired-tomato
Copy link
Author

Removed ORs and NOTs, @DependsOn now only takes an array of modids that must all be present for the class to be loaded

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants