DS3: Version 3.0.0

[nex3]

This is a major rewrite of the Dark Souls III apworld, as discussed initially in Marechal-L/Dark-Souls-III-Archipelago-client#17 and in the Discord channel. It integrates with a branch of @thefifthmatt's static DS3 randomizer to enable a broad swath of new capabilities and improved player experience.

What is this fixing or adding?

• Support for randomizing all item locations, not just unique items, without needing progressive pickup lists.

• Support for randomizing items in shops, starting loadouts, Path of the Dragon, and more.

• Built-in integration with the enemy randomizer, including consistent seeding for races and configuration via the web interface and the standard Archipelago YAML file.

• Support for the latest patch for Dark Souls III, 1.15.2.

• Optional smooth distribution for upgrade items, upgraded weapons, and soul items so you're more likely to see weaker items earlier and more powerful items later.

• More detailed location names that indicate where a location is, not just what it replaces.

• Other players' item names are visible in DS3.

• If you pick up items while offline, they'll still send once you reconnect.

How was this tested?

I've run several multiworlds with this version, as have a number of beta testers from the DS3 channel on the Discord. @Exempt-Medic has run many hundreds of randomized generations with it to flush out edge-case bugs in generation logic.

To test locally, take use the latest beta release as the client and follow the instructions it provides.

[PoryGone]

I was looking through things here, and this section of the README of the randomizer you're integrating with jumped out to me:

One assumes that if you've gotten this far in development, you have spoken with the authors of that randomizer which you forked, and have received direct permission to fork it and distribute it in this way. It would probably be best to surface that conversation publicly here and in the README of the fork, so that everyone is aware of it, and understands any limitations placed on it.

[PoryGone]

Additionally, you should document the reasoning behind the couple of core changes in this PR, as their relevance is unclear.

[nex3]

Re: licensing, @gracenotes (who maintains the static randomizer) is on board to upstream my changes, so there should be no issue there in the long term. (I believe, although Matt can correct me on this, that the reasoning behind it not being freely licensed is that it uses/modifies some existing libraries that never got real licenses and whose original authors are now incommunicado.)

I'm happy to explain any changes, but I'm not sure which you mean specifically. I think most of the changes here are pretty clearly driven by the new features I listed above.

[PoryGone]

Re: licensing, @gracenotes (who maintains the static randomizer) is on board to upstream my changes, so there should be no issue there in the long term. (I believe, although Matt can correct me on this, that the reasoning behind it not being freely licensed is that it uses/modifies some existing libraries that never got real licenses and whose original authors are now incommunicado.)

It's good that you've received permission from the developers of the immediate upstream you're using. That repository's usage of unlicensed code from developers that are not responsive is concerning, but I'll defer to the core maintainers about anything like that.

I'm happy to explain any changes, but I'm not sure which you mean specifically. I think most of the changes here are pretty clearly driven by the new features I listed above.

I'm sure the changes within the DS3 world are all straightforward to those familiar with the World. As someone who is not familiar with the game or its existing World, it is not at all clear what the changes to the core files are in service of, or what the purpose of them is.

Additionally, pinging @lordlou , as this PR changes code from the upstream randomizer that his World is built off of.

[Exempt-Medic]

Additionally, you should document the reasoning behind the couple of core changes in this PR, as their relevance is unclear.

The Generate.py changes are already in PR 2403.

I do think the flatten changes could use some explanation though. They are also probably meant to be used in Items.py though they are not

[nex3]

Oh, the changes outside of DS3! I understand now. Yes, as @Exempt-Medic says, the Utils.py changes are just pulling in https://github.com/ArchipelagoMW/Archipelago/pull/2403. The flatten() change is taking a utility function that was used by SM and pulling it into Utils.py so that other worlds can use it. (The reason DS3 doesn't currently is so that it could be bundled into a standalone apworld, but it should start using it before this lands.)

[PoryGone]

Making a Utility change for other Worlds to use does seem like it's out of scope for this PR, especially if it's not even being used yet. That could reasonably be a separate PR.

[nex3]

@PoryGone That's fair, although my other small separate-pr change #2403 has been languishing for six months so I'm a bit nervous about splitting stuff out. I'm happy to pull that out separately before this lands, though.

@lordlou Oh, I didn't realize that came from a separate source. I've reverted it to use its own flatten function.

@nicholassaylor I'd be thrilled to remove the deprecated options. I kept them because I wanted this to be as minimally disruptive as possible (which, to be fair, isn't that possible). But I would personally prefer to have them gone, so if that's the consensus here as well I'll remove them happily.

@qwint

Wanted to note that while the default exclude_locations populates correctly on Launcher's generate templates, the webhost export settings on defaults does not include it (on options page and weighted options page). I'm not sure how to make it function without that being part of the webhost rework though, but could be documented by the other text describing the defaults.

I had a PR that made this work, but I think it was superseded by the current ongoing webhost work. If this looks likely to land before the rework, I'll add a note.

[PoryGone]

Sneaking core changes through inside large World Content PRs isn't really a great solution. The one you linked to only made it to the core review stage two weeks ago, right around when the most recent release was finalized, and very little has been merged since then. I'm sure a core reviewer will get to it soon enough.

[qwint]

Just wanted to document my current state of review, all my comments were fully addressed/clarified except: I still dislike the Generate.py change being grouped in here as it has its own PR and from light testing this PR does not depend on it My comment on saving the itempool for reference later instead of searching through the multiworld.itempool multiple times in the world - https://github.com/ArchipelagoMW/Archipelago/pull/3128#discussion_r1564391725 - not sure if the response got lost in the rest of the noise. I assume you were getting those errors because you initialized the list in the world itself, not in init/generate_early which is a very common mistake, but I still could be missing something

[ScipioWright]

Would it be a good compromise to tag this with waiting on other, with that other PR needing to be merged in before this one is?

[nex3]

@PoryGone

Sneaking core changes through inside large World Content PRs isn't really a great solution.

The intention isn't to "sneak it in". This is just a PR that depends on another PR. Because this is large, I wanted to get the review started. I've filed #3167 for that, I'm sure it'll get in before this is ready.

@qwint

I still dislike the Generate.py change being grouped in here as it has its own PR and from light testing this PR does not depend on it

This does depend on it—it's required to set the default exclude_locations option to a list of location groups. Those changes are being separately reviewed in https://github.com/ArchipelagoMW/Archipelago/pull/2403, but they're here as well so this PR can be tested in isolation.

My comment on saving the itempool for reference later instead of searching through the multiworld.itempool multiple times in the world - https://github.com/ArchipelagoMW/Archipelago/pull/3128#discussion_r1564391725 - not sure if the response got lost in the rest of the noise. I assume you were getting those errors because you initialized the list in the world itself, not in init/generate_early which is a very common mistake, but I still could be missing something

Ah, initializing it in create_items() does work, thanks! Let me know if I missed any uses in 947d18c5e71172ba5515b8eed75e02b08a92ed68.

[Princesseuh]

Played a run using the latest beta, worked great! Just two things I encountered, apologies if already well known.

• Generating a yaml using the webhost in the fork didn't work, it generated [] somewhere it should've generated {}
• Path of the Dragon was randomized into the main shop in Firelink, I bought it, but I didn't receive the emote.

Everything else worked perfectly fine and it was fun, awesome work!

[ScipioWright]

This PR should probably receive waiting-on: other

It already has waiting-on: other

[nicholassaylor]

This PR should probably receive waiting-on: other

It already has waiting-on: other

My bad, missed it from before

[nicholassaylor]

Since this PR no longer uses the flatten function, do the Utils.py changes need to be in this PR? It seems to make more sense to remove the Utils.py changes and since #2403 got merged, that would mean that this PR isn't blocked anymore.

[qwint]

Skimmed the changes since I last approved and everything looked reasonable, and double checked my last blockers were resolved

[NewSoupVi]

Exempt-Medic has made me aware of the issue with local shop items not being able to be sent to the server.

I think that's kind of bad, I think I would like you to investigate one of the following solutions:

1. Change all local shop items to events in post_fill by setting their address to None (Drawback: The multitracker now "spoils" which shop slots have local items)
2. Some sort of button in the client that "pseudo releases" all local shop item locations, after victory is achieved, or... something like that.
3. If all else fails, rip out shop rando until a client side solution is found

A room that has !release turned off should still be able to theoretically achieve 100% locations if possible.

(I'm aware of missable locations in DS3, which I'm also not entirely happy about, but at least you could make a new save for those if you really want 100%. With this shop issue, it seems that there is absolutely 0 ways to get some locations to be marked on the server)

[nex3]

Option 1 seems like the best compromise if you consider the status quo unacceptable, although I'm not entirely sure how to make that work while still passing the real item IDs to the offline randomizer.

Another option could be to have a "foreign shop items only" option. It couldn't be on by default because it intrinsically wouldn't work in a single-game randomizer, but it would fix multiworld cases. I really do want to get full support for proper shop item detection at some point, so hopefully we can find an acceptable compromise to get this landed in the meantime.

[nex3]

@NewSoupVi I've been mulling this over and over in my head and I finally cracked it. Local shop items are now properly marked as checked in the latest beta!

[NewSoupVi]

I think I will merge this soon, but under the "condition" that if https://github.com/ArchipelagoMW/Archipelago/pull/3739 gets in, remaining_fill is investigated for the post-fill smoothing algorithm. I don't want y'all to have to wait for that, but I think it should use that. (Which as I understand, Exempt-Medic already has a PR open for, to the DS3 AP repo, so that should be good.)