DS3: Dark Souls 3 Major Update/Refactor

[Zunawe]

Co-authored by @Br00ty. @Marechal-L has seen this branch before but hasn't given it a proper review as far as I know.

What is this fixing or adding?

This is a significant overhaul to the versatility of the DS3 implementation. The original goals were to split locations up into categories so that players can tune the size of their item pool, and to allow for a more diverse pool of items that includes things like enemy drops, shop items, or boss soul weapons. But the scope did creep as more stuff became possible.

• Adds a new set of options (enable_weapon_locations, enable_shield_locations, etc...) which toggle specific categories of locations.
• Adds some new regular locations.
• Adds many new "progressive consumable" locations (firebombs, gems, moss clumps, etc...).
• Adds new pool_type option. When set to various, most items in the game can appear in the pool. There will be the same proportion per category, so if your randomized locations originally contained 10 rings, 10 randomly chosen rings will be put into the pool. Previously, items were only pulled directly from the locations that were randomized (or, some locations misrepresented their default item to replace some uninteresting items with interesting ones, but this was still static).
• Adds new guaranteed_items option. This is an ItemDict that forces items into the pool if they aren't already there. Setting pool_type to various makes the set of possible items much larger than the set of randomized locations, so this allows you to make sure certain items show up, and it lets you add multiple copies. Particularly useful for people who like spellcasting builds, since the number of randomized spells is comparatively small, and you currently end up instead just relying on finding tomes to learn the same spells from NPCs as you would in vanilla.
  • Guaranteed items will first be added until the size of the item pool matches the number of locations, then they will begin replacing items in the same category, then other non-progression items.
  • Nothing currently prevents players from using this option to add multiple copies of progression items like cinders. This doesn't cause any hard problems (generation works correctly, and if players receive multiple copies of such items, the extra copies drop on the ground), but it does give more or less full control over a player's item pool barring removing progression items.
• Adds new randomize_infusion option which randomizes the infusion of infusible weapons and shields when the player receives them.
• "Progressive" items like Titanite Shards and Estus Shards which used to define one unique item for every copy (Titanite Shard #21) are now only defined once (Titanite Shard).
• Changes the behavior of late_basin_of_vows. Instead of locking the Basin of Vows to the Pontiff fight, it instead logically locks Lothric Castle and the Dancer fight behind the Small Lothric Banner, meaning you won't be forced to fight the Dancer or explore Lothric Castle without having access to the Undead Settlement and beyond.
• Adds new late_dlc option which logically locks the DLC behind Small Doll, meaning you won't be forced to go into the DLC without having access to Irithyll, the Irithill Dungeon, and Anor Londo.
• Renames a couple options and changes many display names and descriptions for consistency.
• Generally cleans up the code to hopefully be more readable and easier to follow.
• Should address Tests: add a test for worlds to not modify the itempool after create_items #1460.
• As a side effect of these changes, some locations have had their ids changed, and virtually all items have had their ids changed.

This PR requires a minor change to the mod/client to add the new consumables, hence the bump of required_client_version. Those changes have been made, but it's up to @Marechal-L to add whatever else they're working on and then to make a new release. If you run a game with the current version of the mod, those specific consumables will only work the first time, but everything else will work as expected including new items, locations, and infusions.

How was this tested?

A formal beta test on the AP discord and many outside tests by myself and @Br00ty in other multiworlds.

[Berserker66]

Current status is that @Marechal-L is the maintainer of DS3, and this needs their approval.

[Marechal-L]

Nice refactoring with a cleanup of the existing tables and a tons of options added by using DS3ItemCategory.
I will definitely approve this PR and apply the requested changes to the client.

My main concern is the shift of items id for players having a previous save to continue after the update, I don't remember if this is handled by the server or the client.

[Zunawe]

My main concern is the shift of items id for players having a previous save to continue after the update, I don't remember if this is handled by the server or the client.

Someone can correct me if I'm wrong, but

1. Existing games keep the data package in the multidata, so the item ids for those games would remain unchanged. Only newly generated games would have the new item ids.
2. The current client is entirely reliant on slot data to correlate item ids with DS3 item values anyway. So even if the item ids somehow changed for an in-progress save file between two different instances of connecting to the server, as long as the slot data was correct, everything would be fine.

[black-sliver]

If the client uses a apclientpp version that supports "checksummed data package", then it will download the correct data package from the server - i.e. the IDs and strings used to roll the game. The client still has to properly map the IDs to in-game items. IdR how that's handled by the DS3 mod, but if slot data has archipelago ID -> in-game ID (as explained above), that would be fine.

[black-sliver]

I would personally go for IntEnum here (and for LocationCategory), but it's up to you. Either will work.

[Zunawe]

Changed in cd371d2

Can I ask what the purpose is? I suppose was working under the assumption that IntEnum was for when you needed your values to also be ints, and Enum was sort of the "default".

[black-sliver]

That assumption is correct, however IntEnums are guaranteed to only have integer values, which is cleaner imo if you go with discrete integer values (i.e. you don't use auto()). Unless you serialize or de-serialize it, or have special pseudo-flags, they behave the same.

[black-sliver]

This will break existing yamls. Is that fine by everyone?

[Zunawe]

My opinion is that the majority of people will be remaking their yamls anyway, so it seems like the right opportunity to make these setting names more consistent and intuitive from a native English speaker's perspective.

[black-sliver]

Thanks!

Marechal-L and br00ty gave the OK, code looks good.