Skip to content

Conditional mods & event bus subscribers#212

Open
wired-tomato wants to merge 4 commits into
neoforged:mainfrom
wired-tomato:conditional-class-loading
Open

Conditional mods & event bus subscribers#212
wired-tomato wants to merge 4 commits into
neoforged:mainfrom
wired-tomato:conditional-class-loading

Conversation

@wired-tomato

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

Copy link
Copy Markdown

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
Copy Markdown
  • Publish PR to GitHub Packages

@tmvkrpxl0

tmvkrpxl0 commented Nov 12, 2024

Copy link
Copy Markdown

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

tmvkrpxl0 commented Nov 12, 2024

Copy link
Copy Markdown

Uh oh I accidentally mentioned someone

@shartte

shartte commented Nov 12, 2024

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

wired-tomato commented Nov 12, 2024

Copy link
Copy Markdown
Author

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

shartte commented Nov 12, 2024

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Member

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

@wired-tomato

Copy link
Copy Markdown
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

Technici4n commented Nov 13, 2024

Copy link
Copy Markdown
Member

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
Copy Markdown
Author

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

@neoforged-automation neoforged-automation Bot added the needs rebase This Pull Request needs to be rebased before being merged label Jun 18, 2025
@neoforged-automation

Copy link
Copy Markdown

@wired-tomato, this pull request has conflicts, please resolve them for this PR to move forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs rebase This Pull Request needs to be rebased before being merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants