Simplified usage of GatherDataEvent

[BarionLP]

A while ago i showed a helper class to reduce the boilerplate of adding DataProviders to a DataGenerator during GatherDataEvent on discord.

This PR aims to implement those helper methods directly into the GatherDataEvent as Minecraft moves more and more to an data driven approach leading to more and more DataProviders.

NeoForge datagen without helper:

DataGenerator gen = event.getGenerator();
PackOutput packOutput = gen.getPackOutput();
CompletableFuture<HolderLookup.Provider> lookupProvider = event.getLookupProvider();

ExistingFileHelper existingFileHelper = event.getExistingFileHelper();
gen.addProvider(event.includeServer(), new NeoForgeAdvancementProvider(packOutput, lookupProvider, existingFileHelper));
NeoForgeBlockTagsProvider blockTags = new NeoForgeBlockTagsProvider(packOutput, lookupProvider, existingFileHelper);
gen.addProvider(event.includeServer(), blockTags);
gen.addProvider(event.includeServer(), new NeoForgeItemTagsProvider(packOutput, lookupProvider, blockTags.contentsGetter(), existingFileHelper));
gen.addProvider(event.includeServer(), new NeoForgeEntityTypeTagsProvider(packOutput, lookupProvider, existingFileHelper));
gen.addProvider(event.includeServer(), new NeoForgeFluidTagsProvider(packOutput, lookupProvider, existingFileHelper));
gen.addProvider(event.includeServer(), new NeoForgeEnchantmentTagsProvider(packOutput, lookupProvider, existingFileHelper));
gen.addProvider(event.includeServer(), new NeoForgeRecipeProvider(packOutput, lookupProvider));
gen.addProvider(event.includeServer(), new NeoForgeLootTableProvider(packOutput, lookupProvider));
gen.addProvider(event.includeServer(), new NeoForgeBiomeTagsProvider(packOutput, lookupProvider, existingFileHelper));
gen.addProvider(event.includeServer(), new NeoForgeStructureTagsProvider(packOutput, lookupProvider, existingFileHelper));
gen.addProvider(event.includeServer(), new NeoForgeDamageTypeTagsProvider(packOutput, lookupProvider, existingFileHelper));
gen.addProvider(event.includeServer(), new NeoForgeRegistryOrderReportProvider(packOutput));
gen.addProvider(event.includeServer(), new NeoForgeDataMapsProvider(packOutput, lookupProvider));

gen.addProvider(event.includeClient(), new NeoForgeSpriteSourceProvider(packOutput, lookupProvider, existingFileHelper));
gen.addProvider(event.includeClient(), new VanillaSoundDefinitionsProvider(packOutput, existingFileHelper));
gen.addProvider(event.includeClient(), new NeoForgeLanguageProvider(packOutput));

NeoForge DataGen with helper:

var includeServer = event.includeServer();
var includeClient = event.includeClient();

event.createProvider(includeServer, NeoForgeAdvancementProvider::new);
event.createBlockAndItemTags(NeoForgeBlockTagsProvider::new, NeoForgeItemTagsProvider::new);
event.createProvider(includeServer, NeoForgeEntityTypeTagsProvider::new);
event.createProvider(includeServer, NeoForgeFluidTagsProvider::new);
event.createProvider(includeServer, NeoForgeEnchantmentTagsProvider::new);
event.createProvider(includeServer, NeoForgeRecipeProvider::new);
event.createProvider(includeServer, NeoForgeLootTableProvider::new);
event.createProvider(includeServer, NeoForgeBiomeTagsProvider::new);
event.createProvider(includeServer, NeoForgeStructureTagsProvider::new);
event.createProvider(includeServer, NeoForgeDamageTypeTagsProvider::new);
event.createProvider(includeServer, NeoForgeRegistryOrderReportProvider::new);
event.createProvider(includeServer, NeoForgeDataMapsProvider::new);

event.createProvider(includeClient, NeoForgeSpriteSourceProvider::new);
event.createProvider(includeClient, VanillaSoundDefinitionsProvider::new);
event.createProvider(includeClient, NeoForgeLanguageProvider::new);

I also showed a simplification for the LootTableProvider which I would contribute in the future

event.addLootTables(builder -> builder
        .AddBlockProvider(ModBlockLootSubProvider::new)
        .AddEntityProvider(ModEntityLootSubProvider::new)
        .AddChestProvider(ModChestLootSubProvider::new)
);

I thought about removing the need to define when to run which provider as every provider type is either server or client.
This could be done through a maker on the base providers or through pattern matching in the addProvider method.
Both approaches are not perfect and @Technici4n pointed out the code saved is not worth the effort but i do think it could greatly help beginners.

my original implementation: https://github.com/Ametrin-Studios/Ametrin/blob/master/Ametrin/src/main/java/com/ametrinstudios/ametrin/data/DataProviderHelper.java

[CLAassistant]

All committers have signed the CLA.

[neoforged-pr-publishing]

• Publish PR to GitHub Packages

[KnightMiner]

As one comment, the "complicated" datagen that is simplified by the helpers only really applies to vanilla or Forge datagen, I've got quite a few custom data generators that need more arguments, such as requiring instances of one being passed to the other. It make me wonder if this is the best approach for adding such helpers, or if it wouldn't be better to do in some way that can be extended to custom data generators from mods.

As another comment, it looks probably 10x cleaner if you make a local variable for event.includeServer() and event.includeClient()

[BarionLP]

As another comment, it looks probably 10x cleaner if you make a local variable for event.includeServer() and event.includeClient()

just left that as it was, I'll change it

[BarionLP]

As one comment, the "complicated" datagen that is simplified by the helpers only really applies to vanilla or Forge datagen, I've got quite a few custom data generators that need more arguments, such as requiring instances of one being passed to the other. It make me wonder if this is the best approach for adding such helpers, or if it wouldn't be better to do in some way that can be extended to custom data generators from mods.

this should cover like 95% of all cases but you are correct.
Originally i wanted to write a full DI system but quickly realized that this would be an insane overkill but it might be an option if neoforge already has one that could be used here.
Of course I could make addBlockAndItemTags more generic solving at least the case with one dependency...
but going further this will quickly explode in combinatorial complexity (which might be a general problem because theoretically you could reorder the parameters of the constructors).

In case someone has an idea for a better approach let me know.
Although I don't think this has to cover every scenario, advanced users with special cases still can use the traditional way.

[BarionLP]

I could make addProvider return the added provider.
this would allow you to do this

var example = event.addProvider(include, ExampleProvider::new);
event.addProvider(include, (...) -> new OtherProvider(..., example, ...);

[BarionLP]

I could make addProvider return the added provider. this would allow you to do this

var example = event.addProvider(include, ExampleProvider::new);
event.addProvider(include, (...) -> new OtherProvider(..., example, ...);

this would introduce generics (because the return type has to match the input type) which indeed leads to ambiguous calls. to fix this I would have to rename builder methods to createProvider because somehow java cannot differentiate between passing an concrete instance or a functionalInterface but can differentiate different interfaces

[Shadows-of-Fire]

The idea is good; I would like to see some streamlining in the data provider registration process (especially with the requirement that registry set builders be registered first and then that object propagated downwards);

But, I would also like to explore some alternatives and assess some design considerations on what type of utility is the best from a user-facing perspective. Externally, I've developed an alternative solution to this: the DataGenBuilder class, which has the following example usage¹:

@SubscribeEvent
public void data(GatherDataEvent event) {
    DataGenBuilder.create(MODID, "minecraft")
        .registry(Registries.ENCHANTMENT, ApothEnchantmentProvider::bootstrap)
        .provider(LootProvider::create)
        .provider(EnchTagsProvider::new)
        .provider(EnchRecipeProvider::new)
        .build(event);
}

This current structure has some limitations (it makes no differentiation between includeServer and includeClient, but that's not too difficult to add).

Between the two solutions, I think I find myself leaning towards DataGenBuilder; it is more generic with room for extension by mods (as compared to the solution with methods on the Event object), and it handles RegistrySetBuilder objects (which your solution does not even allow for, since the objects are all built directly with config.lookupProvider).

Footnotes

1. Source ↩

[Shadows-of-Fire]

Given these general usages, it might make more sense to have additional createServerProvider and createClientProvider methods instead of requiring the caller to pass boolean run.

The current interface-based definition structure means this would lead to mass method overload though, so I'm not sure how clean that would be.

[BarionLP]

Given these general usages, it might make more sense to have additional createServerProvider and createClientProvider methods instead of requiring the caller to pass boolean run.

great idea, but it would tripple the method count (we still need one with parameter for dynamic scenarios or when using includeDev/includeReports). do we want this? I also talked about automatically detecting when to run what via some sort of marker in the provider class or pattern matching in addProvider.

The current interface-based definition structure means this would lead to mass method overload though, so I'm not sure how clean that would be.

yes, i don't like that too but i couldn't come up with a better option.
I did not want to use generic interfaces (like BiFunction) because I find them hard to read (BiFunction<PackOutput, CompletableFuture<HolderLookup.Provider>, T>). DataProviderFromOutputLookupFileHelper<T> is only slightly better though. I decided not to use DataProvider.Factory<T> for consistency but i could swap it to remove one interface.
As I mentioned before somewhere an alternative would be a depency injection system but I feel like that would be overkill. This would also fix cases where users reorder their parameters. I might consider it if there are other usecases for such a system. Alternativly we could use a library. (or does neo already has one?)

[BarionLP]

@Shadows-of-Fire ?

[BarionLP]

As @KnightMiner pointed out there are scenarios where data providers depend on other providers to run (e.g. item tag provider). I'm not sure how this would be possible with your DataGenBuilder.
for custom RegistrySetBuilder objects a builder probably is the better option.
I feel like the optimal solution is a combination of both approaches. I'm unsure how that should look though.

[Shadows-of-Fire]

I'm curious to hear Knight's opinion on the builder approach. While true it does not supply an OOTB solution for chaining additional generators (nor custom generators), the fact that it is an extensible object means modders can add those utilities themselves if necessary or desired.

[BarionLP]

While true it does not supply an OOTB solution for chaining additional generators (nor custom generators), the fact that it is an extensible object means modders can add those utilities themselves if necessary or desired.

I think this can be built on top of this pr once it is merged. I have to admit that I don't feel confident enough to built something like this on my own.

[neoforged-automation]

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