Modifying loot tables

[Juuxel]

Adds an event that can be used to modify loot tables when they're loaded.

Edit: Tagging issue #7.

[asiekierka]

I'm not perfectly happy with it being geared only for additions. We need to evaluate use-cases for modifications and replacements, if any (removals are probably impossible).

[asiekierka]

I think an event might be the correct approach here for Fabric, possibly with a helper method to load loot tables from a different JSON file and a helper to merge them.

We rejected JSONs for block/item properties - I think we should do likewise here and leave it to a J-in-J.

[Juuxel]

I'm not perfectly happy with it being geared only for additions. We need to evaluate use-cases for modifications and replacements, if any (removals are probably impossible).

Additions are the easiest to implement. Modification would require some way to identify the loot pools you want to modify or remove (probably through converting the loot table into a map, and looking for certain values).

I think an event might be the correct approach here for Fabric

I'll add an event for adding loot pools. Should I use the event system from #74 if/when that is merged?

[Juuxel]

Changed the loot pool adders to an event system in 502b678.

[Juuxel]

I've updated this to use the 0.2.0 event API.

[asiekierka]

Oh, no, one issue here.

Mojang intends LootPools etc. to be immutable. Notice the Builder pattern, and the finalities. Perhaps, instead of setters, we'd be better off with (a) getters backed by Arrays.asList, and (b) new builders which allow using existing loot pools and not just builders? That, or examining the API space again

[Juuxel]

Using extended builders to do immutable transformations would also work, and allowing to mutate existing loot tables can be a bit dangerous (as they're intended to be final).

[Juuxel]

So, my new approach is making two new builders for LootPools and LootSuppliers and making the event take the extended LootSupplier builder. The original LootSupplier is copied to the builder and replaced by the result of the event handlers. This doesn't currently have a way to remove LootPools or other values from the builders. I'm not sure if that is needed / wanted.

The extended builders can be made from existing loot pools / suppliers and they add helper methods that can be used to add entries / functions / pools directly without using their builders.

I'm not sure if the extended LootPool builder is needed as I've added the withPool method taking a pool directly, not a LootPool.Builder.

[LemmaEOF]

@Juuxel any new developments? This is something I feel should get in pretty soon.

[Juuxel]

Is removing loot pools or other loot table components needed? If not, I think this PR is ready to be reviewed or merged. (@Boundarybreaker)

[LemmaEOF]

It might be helpful, but it's not really necessary for a first release.

[Juuxel]

It can easily be added, but I think this is ready for review. We can add it later.

[asiekierka]

Well, we can easily replace loot tables for now, anyway.

One thing which concerns me about the implementation is that what I wanted to promote (and would presumably be simpler, and more adaptable by other datapack devs/modpack devs) is allowing the appendment of one loot table JSON to an existing loot table. This doesn't seem to add a trivial way to do so, does it?

[Juuxel]

I can add a simple helper to do that. I just need to deserialize a loot table or an array of pools. Loot tables / suppliers have a type, so I'm thinking of a LootSupplier merge(LootSupplier a, LootSupplier b) (name can be changed of course), where the loot supplier type is taken from a.

Edit: Nevermind. I think I'll add FabricLootSupplierBuilder.copyFrom(LootSupplier supplier).

[Juuxel]

Where should I add the loot table deserialization helper? Should I make a utility class for it, or include it in FabricLootSupplierBuilder as a static?

[asiekierka]

Utility class (not necessarily exposed in API, depending on usefulness) + copyFrom

[Juuxel]

I added a separate LootUtils utility class with some comments on it under a TODO.

[Juuxel]

I moved the LootUtilities class from impl to api and added some more helper methods for deserializing, while making the Gson instance private.

I think this is ready to be merged.

[LemmaEOF]

This should probably be merged ASAP before 1.14 full release on Tuesday, if nobody has any more objections to it.

[asiekierka]

Speaking of ASAP...

[asiekierka]

Doing some minor tweaks to this, and then I'll make a new PR.