Loonium with data-driven structure loot #4639

TheRealWormbo · 2024-05-10T19:48:22Z

Implementation for structure detection and structure-specific loot tables and mob spawning pools, customizable via data packs.

structure-specific loot tables are identified by the structure ID (e.g. for minecraft:pillager_outpost the custom Loonium loot table is located at data/botania/loot_tables/loonium/minecraft/pillager_outpost.json), with a default loot table corresponding to dungeon loot (located at data/botania/loot_tables/loonium/default.json)
- default loot tables are created with Datagen
- loot tables usually just reference original chest or entity loot tables related to each structure, so Loonium loot can be modified separately from the loot found in those structures
structure-specific Loonium configurations are identified the same way (e.g. for minecraft:pillager_outpost the custom configuration is located at data/minecraft/config/loonium/pillager_outpost.json) with a default configuration (located at data/botania/config/loonium/default.json) acting as fallback
- default configurations are created via Datagen
- configurations can reference a parent configuration and only override individual aspects of the referenced configuration
- configurations consist of mana cost to spawn a mob, how many mobs of spawnable types are allowed around the flower, whether the Loonium must be placed in the overall bounding box or an individual piece of the structure, the weighted list of mobs to spawn, a list of potion effects to apply to spawned mobs, and a list of attribute modifiers to apply to the mobs
  - attribute and effect lists can be overridden for individual mobs
  - custom NBT data can be defined for individual mobs (used to make a Creeper spawn charged)
  - equipment loot tables can be specified for each spawnable mob (this will be a vanilla feature in later Minecraft versions, although the implementation here is slightly tweaked from what Trial Chambers use)
expanded wand HUD to indicate whether a Loonium generates structure-specific loot
added lexicon mention of the structure-specific behavior
fixed the mob location selection logic to prevent mobs from immediately starting to suffocate
added a new challenge advancement for killing every mob type in the structure configurations added with this change
mobs spawned by the Loonium are put onto a unique team so they don't just start attacking each other (because there are zoglins in some of the mob spawn pools; Heisei Dream may need a future upgrade to support overriding teams allegiances)

TheRealWormbo · 2024-05-10T19:49:24Z

Supersedes #4313, but is only loosely based on that PR.

Loonium figures out which structures(s) it was placed in and uses a corresponding list of loot tables with the id `botania:loonium/<structure_namespace>/<structure_path>`. If the flower is placed in overlapping pieces of multiple structures with configured loot tables, it randomly chooses one of the tables whenever it spawns a mob. The Loonium drop capability is now attached to all mob entities, in preparation of customizable mob pools. Default equipment slot drops are turned off. TODO: Define loot tables for relevant vanilla structures. TODO: Define structure configuration data (JSON, likely separate registry) for the spawnable mob pool and whether the flower must be placed within a structure piece or anywhere within the structure's overall bounding box, maybe even the amount of mana per spawned mob. TODO: Move default configuration to a default structure configuration entry.

(can't be detected reliably)

- tweak ancient table selection city weights - fix reference to elder guardian drops loot table - remove type specification on Loonium loot tables, as it caused validation warnings

(includes a generic reloadable config data manager, which currently only covers Loonium configurations)

(covers e.g. the skeleton from a spider jockey or the bonus chicken for some baby zombies)

(such as skeletons from a spider jockey or chickens from a zombie jockey, provided they are in the spawned mobs list)

- move inner config classes to own files - add builders where appropriate - simplify optional field definitions in codecs where possible, and remove constructors with Optional arguments

Equipment tables are not vanilla yet, so they currently use the "selector" loot table type and are applied manually by the Loonium. Nested loot table definitions also currently need to be loot table references, since embedded tables as pool entries are not supported by vanilla yet either.

- mobs are spawned on a unique team, which means even Loonium-spawned zoglins don't attack other Loonium-spawned mobs - new challenge advancement "King" requires killing each of the currently 22 different mobs the Loonium spawns in different structures

artemisSystem · 2024-08-04T18:26:54Z

Writing this comment as i'm going through the review:

for minecraft:pillager_outpost the custom configuration is located at data/minecraft/config/loonium/pillager_outpost.json

Why not data/botania/config/loonium/minecraft/pillager_outpost.json? The current location doesn't actually specify botania anywhere (only botania thing is the loonium folder), which maybe doesn't matter that much, but seems off to me at least. Changing it to my suggestion would also make it more consistent with the loot table locations.

Actually, what if we made the loot table specifiable in the config? Then, if you're just referencing something, you can point to it directly without creating a new loot table file, and if you want to make something custom, you can reference any custom loot table. Then the config becomes the only "magic" resource location, instead of having two. Thoughts?

(this will be a vanilla feature in later Minecraft versions, although the implementation here is slightly tweaked from what Trial Chambers use)

What's different? Will/should our implementation change to match more closely with vanilla in 1.21? Or will we be able to reuse vanilla code?

whether the Loonium must be placed in the overall bounding box or an individual piece of the structure

Realizing on a second read that this means any individual piece of a structure, right? Vs the cuboid bounding box that encompasses the whole structure. But the way i first interpreted this made me think: Is it possible to detect which structure piece you're in? I guess it would depend on the structure type, i don't know too much about structures, but there's at least jigsaw placement with pieces. Are there other, non-jigsaw ways to make a structure? Anyway, different configs for individual structure pieces would be cool to have, though maybe that could be an additional change later.

artemisSystem · 2024-08-04T19:08:38Z

Xplat/src/main/java/vazkii/botania/data/LooniumStructureLootProvider.java

+ Also, archaeology loot tables need to be handled carefully, due to their limited loot pool options.
+ */
+
+ tables.put(getLootId(BuiltinStructures.ANCIENT_CITY),


This could use a helper function, probably. Nicer to author and edit. Maybe an interface like:

put(tables, BuiltinStructures.ANCIENT_CITY, LootTableReference.lootTableReference(BuiltInLootTables.ANCIENT_CITY).setWeight(4), LootTableReference.lootTableReference(BuiltInLootTables.ANCIENT_CITY_ICE_BOX).setWeight(1) )

I could do this myself, sounds like a fun refactor :3

artemisSystem · 2024-08-04T19:14:50Z

Xplat/src/main/java/vazkii/botania/data/LooniumStructureLootProvider.java

+ )
+ );
+
+ var output = new ArrayList<CompletableFuture<?>>(tables.size());


I think it's cleanest if this is it's own function. Or maybe this stays in the run function, and a separate function is made that handles creating and populating tables. Again, i can handle this

artemisSystem · 2024-08-04T19:21:49Z

Xplat/src/main/java/vazkii/botania/data/LooniumStructureLootProvider.java

+
+ @NotNull
+ private static LootTable.Builder buildOceanRuinLootTable(ResourceLocation archaeology) {
+ // Note: since the Loonium does not supply a location, treasure maps will roll as empty maps


Could we make it supply a location? would be cool

We could supply the Loonium's (effective) location, or the mob's death location. However, generating a treasure map can come with a significant performance impact, and the usefulness of the generated would probably depend on how much you can manipulate the location.

Using the mob's death location sounds fun, then you could bring them with you places to roll treasure maps for that area

That sounds interesting. I'm just a little worried about the performance impact of frequently generating maps, because locating structures can take several seconds. Also, the loot table rolling needs to be moved from mob spawn to mob death.

artemisSystem · 2024-08-05T15:02:09Z

Xplat/src/main/java/vazkii/botania/data/LooniumStructureLootProvider.java

@@ -179,7 +179,7 @@ public CompletableFuture<?> run(@NotNull CachedOutput cache) {
 for (var e : tables.entrySet()) {
 Path path = pathProvider.json(e.getKey());
 LootTable.Builder builder = e.getValue();
- LootTable lootTable = builder.setParamSet(LootContextParamSets.EMPTY).build();
+ LootTable lootTable = builder.setParamSet(LootContextParamSets.ALL_PARAMS).build();


Shouldn't this ideally be changed to the context parameters we actually supply/use?

IIRC there's validation happening when the loot table is written, loaded, or evaluated. And while you don't need to set any parameters, having them not defined in the first place can lead to warnings or errors if the type of the referenced loot requires them.
The Loonium loot is generated in an empty context (it could probably be a different context that provides the flower location and mob reference somehow, but that could influence the type of items you get, e.g. due to cooking food drops if the mob was on fire, or whether it was a player kill with or without looting), but there are references to e.g. entity, chest, or archaeology type loot tables, which may have different parameter sets.

Long story short: This commit changed it from the actually supplied set (empty) to the widest available set (all params) because something didn't work as expected.

artemisSystem · 2024-08-05T16:49:56Z

Xplat/src/main/java/vazkii/botania/common/block/flower/functional/LooniumBlockEntity.java

@@ -262,7 +266,7 @@ public void readFromPacketNBT(CompoundTag cmp) {
 super.readFromPacketNBT(cmp);
 if (cmp.contains(TAG_LOOT_TABLE)) {
 var lootTableString = cmp.getString(TAG_LOOT_TABLE);


This is just a String right? should just say String instead of var

artemisSystem · 2024-08-05T16:51:06Z

Xplat/src/main/java/vazkii/botania/common/block/flower/functional/LooniumBlockEntity.java

+ attuneType = "multiple_structure_drops";
+ }
+ String attuned = I18n.get("botaniamisc.loonium." + attuneType).formatted(flower.lootTables.length);
+ int filterWidth = mc.font.width(attuned);


filter? is this copied from hopperhock?

I think it is, oops.

artemisSystem · 2024-08-05T16:55:25Z

Xplat/src/main/resources/assets/botania/lang/en_us.json

@@ -132,6 +132,9 @@
 "botaniamisc.rannuncarpus.state_sensitive": "Match Exact State",
 "botaniamisc.rannuncarpus.state_insensitive": "Match Block Only",
 "botaniamisc.lokiRingLimitReached": "Selection limit reached",
+ "botaniamisc.loonium.generic_drops": "Not attuned",
+ "botaniamisc.loonium.structure_drops": "Attuned to structure",


Do we want to say which structure it's attuned to if it's only one? The name could be specified in the config maybe. Another reason to require config i guess. It would result in twice the files together with my other idea (config references a loot table, and we have loot table functions that limit to one drop), but maybe not a huge issue? It feels cleaner and the most customizable

That part changed in a later commit to "Generic loot", "Structure loot", or "Custom loot" (if the Loonium BE NBT is edited to have a custom loot table assigned), but either way, structures do not have display names. The only thing we could show is the structure resource location.

artemisSystem · 2024-08-05T17:00:45Z

Xplat/src/main/java/vazkii/botania/api/configdata/LooniumStructureConfiguration.java

+ Codec.list(MobEffectToApply.CODEC).fieldOf("effectsToApply").forGetter(o -> o.effectsToApply)
+ ).apply(instance, LooniumStructureConfiguration::new)
+ );
+ public static final Codec<LooniumStructureConfiguration> OPTIONAL_CODEC = RecordCodecBuilder.create(


I think i'd call this one WITH_PARENT_CODEC or something, to more clearly get across the difference

That was the first shot at implementing the codec logic. Turns out I didn't need the second one.

artemisSystem · 2024-08-05T17:04:11Z

Xplat/src/main/java/vazkii/botania/api/configdata/LooniumStructureConfiguration.java

+public class LooniumStructureConfiguration {
+ public static final Codec<LooniumStructureConfiguration> CODEC = RecordCodecBuilder.create(
+ instance -> instance.group(
+ ExtraCodecs.POSITIVE_INT.fieldOf("manaCost").forGetter(o -> o.manaCost),


I think i'd prefer this to be NON_NEGATIVE_INT, for more customizability

Should work without further modifications, as far as I can tell.

artemisSystem · 2024-08-05T17:12:48Z

Xplat/src/main/java/vazkii/botania/api/configdata/LooniumStructureConfiguration.java

+ ExtraCodecs.validate(Codec.INT,
+ duration -> duration > 0
+ ? DataResult.success(duration)
+ : DataResult.error(() -> "Invalid effect duration"))


Why not ExtraCodecs.POSITIVE_INT?

I can't remember if that works with the -1 (infinite) default value. Maybe it does.

The current one also doesn't work with -1, does it?

Fire resistance is an infinite effect for most mobs, so it should be working in the current state.

artemisSystem · 2024-08-05T18:34:17Z

Xplat/src/main/java/vazkii/botania/common/config/ConfigDataManager.java

+ return true;
+ }
+ var parentConfig = map.get(parent);
+ return parentConfig != null && findTopmostParent(map, parent, parentConfig.parent, visitedEntries);


From what i can see, specifying a missing ID as the parent, will lead to the Ignoring Loonium structure configuration(s) without top-most parent error message. Or is that caught earlier? If not, maybe we should have an error specifically for that also

The config loading logic should be validating that any specified parent ID actually exists.

artemisSystem · 2024-08-05T18:44:04Z

Xplat/src/main/java/vazkii/botania/api/BotaniaAPI.java

@@ -33,6 +33,7 @@
 import vazkii.botania.api.corporea.CorporeaNodeDetector;
 import vazkii.botania.api.internal.DummyManaNetwork;
 import vazkii.botania.api.internal.ManaNetwork;
+import vazkii.botania.common.config.ConfigDataManager;


(forgive me if this changes in a later commit)

I don't think you can import a non-api class in the api. Someone depending on just the API wouldn't be able to call this method anyway.

Also, i'm not entirely clear on the purpose of this method. From what i can tell, the api impl starts with an instance of the config manager, and another instance is registered as a listener from the class itself. Then, whenever data is reloaded, setConfigData is called with that instance, which has the correct data. But, the instance stays there and doesn't need to be set again on next reload, so this all just seems a bit weird.

Ah, right. Probably need to extract an API interface for the ConfigDataManager to use in those methods.

The way data pack reloading (for vanilla, and also for this) works is as follows: The game starts without any loaded data, which is an empty accessible instance of the data manager. When data packs are reloaded, a new instance (not yet accessible outside the loading logic) is created, and when loading is done, that instance replaces the previous accessible one.

I'm not sure if there's any better place to put this, but BotaniaAPI seemed like a good place for the method that returns the currently valid config data instance. I'm also not quite sure if the setter should be publicly accessible, but I couldn't figure out a clean way to set the instance to return.

Xplat/src/main/java/vazkii/botania/common/block/flower/functional/LooniumBlockEntity.java

artemisSystem · 2024-08-05T19:13:36Z

Xplat/src/main/java/vazkii/botania/common/block/flower/functional/LooniumBlockEntity.java

+ if (e != mob && e instanceof Mob otherMob) {
+ // prevent armor/weapon drops on player kill, also no nautilus shells from drowned:
+ Arrays.stream(EquipmentSlot.values()).forEach(slot -> otherMob.setDropChance(slot, 0));
+ LooniumComponent otherLooniumComponent = XplatAbstractions.INSTANCE.looniumComponent(otherMob);


This doesn't apply to the root mob anymore, does it?

In case of mob stacks (e.g. chicken jockey, or spider jockey), this prevents secondary mobs from dropping anything. In a chicken jockey, the "root vehicle" is a chicken with a jockey flag set, while in a spider jockey the "root vehicle" is the spawned spider, but it has a skeleton as passenger. So no random feathers, bones, or armor.

In later versions you will see an expansion of that piece of code to cover various edge cases, such as not spawning pillagers as captains, or actually assigning loot drops to known mob types.

artemisSystem · 2024-08-11T14:55:49Z

Xplat/src/main/java/vazkii/botania/common/block/flower/functional/LooniumBlockEntity.java

+ var key = structureMap.firstKey();
+ detectedStructures = Object2BooleanMaps.singleton(key, structureMap.getBoolean(key));
+ } else {
+ detectedStructures = new Object2BooleanArrayMap<>(structureMap);


Is this check necessary? Can't you just use the constructor like this in the empty and singleton cases too?

I guess it could be considered premature optimization. The same would be true for the parts where that map is serialized to/from NBT.

artemisSystem · 2024-08-11T15:00:53Z

Xplat/src/main/resources/assets/botania/lang/en_us.json

- "botaniamisc.loonium.multiple_structure_drops": "Attuned to %d structures",
+ "botaniamisc.loonium.generic_loot": "Generic loot",
+ "botaniamisc.loonium.structure_loot": "Structure loot",
+ "botaniamisc.loonium.custom_loot": "Customized loot",


I think i don't actually like this. It's not really relevant to the player (in my eyes at least), whether the loot is from a structure or set via NBT. I think i liked "Attuned" better. Maybe it could be "Attuned to <structure>", (grabbing the structure name from the config (only if we make the config mandatory)) and you could choose your own string for that if you're setting the mobs with nbt.

I don't think structures have a translated name by default. Displaying the structure resource location feels wrong.

Yeah, which is why the name would need to be specified in the config

Config is server side, the display name would need to be in a language file (i.e. resource pack) on the client.

artemisSystem · 2024-08-11T15:07:28Z

Xplat/src/main/java/vazkii/botania/common/block/flower/functional/LooniumBlockEntity.java

- consumer.accept(comp.getDrop());
- }
+ if (comp != null)
+ if (!comp.getDrop().isEmpty() || comp.isDropNothing()) {


This if only fails if the component is set to drop an empty stack, without dropNothing being set, right? Does that ever happen? To me, it looks like the case when the set drop is empty works like if dropNothing is set anyway, so maybe we don't need dropNothing at all and can just set the component with an empty stack?

The component should always be attached to mobs on Fabric, so we need to somehow specify whether the mob was spawned by a Loonium or not. (I believe on NeoForge the data attachment can be created later on-demand, but the CCA logic for Fabric runs at the end of the entity's constructor.)

Maybe a better way to structure the data would be to have an "is Loonium-spawned" field that specifies that the toDrop item stack should be used, even if it's empty.

artemisSystem · 2024-08-11T15:11:50Z

Xplat/src/main/java/vazkii/botania/common/block/flower/functional/LooniumBlockEntity.java

@@ -198,10 +216,16 @@ public void tickFlower() {
 world.addFreshEntity(mob);
 mob.spawnAnim();

- addMana(-COST);
+ addMana(-DEFAULT_COST);


Oh, this needs to use the cost from the config, doesn't it?

That's changed in a later commit. One thing that doesn't change later, though, is the mana capacity. That is currently fixed at the default cost, so technically no structure could use a higher mana cost.

- var -> explicit type, unless it's obvious from a constructor call - extracted API interface for ConfigDataManager - replace "drop nothing" flag in LooniumComponent with "override drop"; mobs will drop whatever is defined as "to drop" when that flag is set - don't bother creating specialized map types for detected structures - adjust variable names in renderHUD code - replaced some manually validated attribute CODECs with corresponding predefined ExtraCodecs variants - allow configured mana cost for a structure to be zero - add default translation for botania:loonium_offhand_equipment tag

TheRealWormbo mentioned this pull request May 13, 2024

Unobtainable odds and ends in Garden of Glass #3101

Open

16 tasks

TheRealWormbo force-pushed the 1.20-loonium-structures branch 2 times, most recently from 8813fdc to 272d340 Compare May 18, 2024 17:27

TheRealWormbo marked this pull request as draft May 20, 2024 19:13

TheRealWormbo force-pushed the 1.20-loonium-structures branch 4 times, most recently from 0819447 to 2fed3ca Compare May 24, 2024 21:55

TheRealWormbo marked this pull request as ready for review May 26, 2024 10:43

TheRealWormbo force-pushed the 1.20-loonium-structures branch 2 times, most recently from 7344086 to a088bac Compare June 19, 2024 22:46

TheRealWormbo force-pushed the 1.20-loonium-structures branch from a088bac to ca9e6a4 Compare June 30, 2024 08:02

TheRealWormbo added 17 commits July 4, 2024 20:51

Datagen for Loonium loot tables

d3b2ab5

Remove igloo bottom from loot structures

7cd3bfa

(can't be detected reliably)

Adjust loot table definitions

5d5e216

- tweak ancient table selection city weights - fix reference to elder guardian drops loot table - remove type specification on Loonium loot tables, as it caused validation warnings

Update lexicon description of Loonium

420f919

Expand wand HUD to indicate Loonium structure detection

2807ba6

Datagen for structure-specific Loonium configurations

37d1686

Datagen for and loading of structure-specific Loonium configurations

39db350

(includes a generic reloadable config data manager, which currently only covers Loonium configurations)

Loonium uses structure configs and loot

3086c97

Remove unintended non-Loonium drops from additional mobs

40c8422

(covers e.g. the skeleton from a spider jockey or the bonus chicken for some baby zombies)

Limit number of spawned mobs around Loonium

1f52560

Loonium-spawned mobs have no instant despawn range

1f2ec79

Fix structure configuration lookup

5d4214d

Apply bonuses to additionally spawned Loonium mobs

18658f4

(such as skeletons from a spider jockey or chickens from a zombie jockey, provided they are in the spawned mobs list)

General Loonium structure config cleanup

6903b3d

- move inner config classes to own files - add builders where appropriate - simplify optional field definitions in codecs where possible, and remove constructors with Optional arguments

Fix spawned mobs suffocating and add spawn effect to flower

afca0bf

TheRealWormbo added 3 commits July 4, 2024 20:52

Actually consume the configured amount of mana

20814da

Hint at Loonium item exclusion list in lexicon description

1c516fd

TheRealWormbo force-pushed the 1.20-loonium-structures branch from ca9e6a4 to 1c516fd Compare July 4, 2024 18:55

TheRealWormbo assigned artemisSystem Jul 4, 2024

artemisSystem reviewed Aug 4, 2024

View reviewed changes

artemisSystem reviewed Aug 5, 2024

View reviewed changes

Xplat/src/main/java/vazkii/botania/common/block/flower/functional/LooniumBlockEntity.java Show resolved Hide resolved

artemisSystem reviewed Aug 5, 2024

View reviewed changes

Xplat/src/main/java/vazkii/botania/common/block/flower/functional/LooniumBlockEntity.java Show resolved Hide resolved

artemisSystem reviewed Aug 5, 2024

View reviewed changes

artemisSystem reviewed Aug 11, 2024

View reviewed changes

Loonium with data-driven structure loot #4639

Are you sure you want to change the base?

Loonium with data-driven structure loot #4639

Conversation

TheRealWormbo commented May 10, 2024 • edited Loading

TheRealWormbo commented May 10, 2024

artemisSystem commented Aug 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artemisSystem Aug 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artemisSystem Aug 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheRealWormbo commented May 10, 2024 •

edited

Loading

artemisSystem Aug 11, 2024 •

edited

Loading

artemisSystem Aug 11, 2024 •

edited

Loading