Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DS3: Added 'Early Banner' Setting #2199

Merged
merged 31 commits into from
Nov 25, 2023
Merged

Conversation

Br00ty
Copy link
Contributor

@Br00ty Br00ty commented Sep 20, 2023

What is this fixing or adding?

This adds a new setting called "Early Small Lothric Banner". This was inspired by the SM setting "Early Morphball" to help get people out of BK mode faster in DS3. When enabled, the lothric banner will be placed in an early sphere (sphere 1).

How was this tested?

Generated a bunch of seeds with multiple games, with the setting toggled "on" and "off" to make sure that the banner really did show up in Sphere 1 when enabled. Played a bit of a seed just to get the banner.

If this makes graphical changes, please attach screenshots.

@Br00ty
Copy link
Contributor Author

Br00ty commented Sep 20, 2023

tagging @Marechal-L so they can not only see it, but give a thumbs up if this is fine with them.

Copy link
Collaborator

@ScipioWright ScipioWright left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generated a few times and checked spoiler log, it did appear to function properly.

@ThePhar ThePhar added the is: enhancement Issues requesting new features or pull requests implementing new features. label Oct 16, 2023
@Br00ty
Copy link
Contributor Author

Br00ty commented Oct 28, 2023

@Marechal-L if you agree with these changes, would you give it an approval?

@nex3
Copy link
Contributor

nex3 commented Oct 29, 2023

Sphere 1 can be very large in some games—would it make sense for this setting to also force the banner to appear in DS3 itself?

@@ -93,6 +93,8 @@ def generate_early(self):
self.enabled_location_categories.add(DS3LocationCategory.HEALTH)
if self.multiworld.enable_progressive_locations[self.player] == Toggle.option_true:
self.enabled_location_categories.add(DS3LocationCategory.PROGRESSIVE_ITEM)
if self.multiworld.early_banner[self.player] == Toggle.option_true:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be conditional on MISC locations being randomized?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not dealing with anything other than the Banner for this PR. this PR's intentions were not to go any deeper than to solve one singular issue, and that is having a Sphere 1 banner across worlds.

I'd say if thats something that you would like to see when you start implementing your take on the client, that would be the time to create a seperate PR along with your changes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean for the banner specifically—since the banner location is only randomized at all if enable_misc_locations is set, you'll be requesting early assignment for an item that's not in the item pool at all when it's not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got what you mean now, corrected it to be under KEY locations, as that is where the banner resides, is under KEYS not MISC

@Br00ty
Copy link
Contributor Author

Br00ty commented Oct 30, 2023

Sphere 1 can be very large in some games—would it make sense for this setting to also force the banner to appear in DS3 itself?

That can be done by the player already using plando settings, or just forcing there Banner to be local via YAML. that is not the goal of this PR

@nex3
Copy link
Contributor

nex3 commented Oct 31, 2023

Users could also use plando to force the banner into sphere 1, right? It seems to me that if we're creating a more user-friendly option to effectively say "don't BK me on Small Lothric Banner", it would make sense to guarantee that the banner is in the DS3 notion of "the early game" rather than lost in the overworld of some huge game.

@Br00ty
Copy link
Contributor Author

Br00ty commented Nov 1, 2023

@nex3 people would plando their Banner into sphere 1 of THEIR world. that is not the intention of this. this is what i talked about in the mod group with Zunawe, Marech, and myself. This was also already discussed in the Async i ran at the beginning of the latest ap update. there are plenty of games with big sphere 1’s and it’s more enjoyable to have items randomized throughout worlds than to have it always be in your world. if the user wants it in there world, they can specify that in there YAML already.

@Br00ty
Copy link
Contributor Author

Br00ty commented Nov 4, 2023

Okay, to accommodate @nex3 request, I have changed the setting around to make it so the user can now choose whether the Small Lothric Banner is placed in an early sphere in their world, or across all worlds.

@Br00ty
Copy link
Contributor Author

Br00ty commented Nov 16, 2023

just changed some words around to use global/local instead to match other games that use similar

worlds/dark_souls_3/__init__.py Outdated Show resolved Hide resolved
@Br00ty
Copy link
Contributor Author

Br00ty commented Nov 25, 2023

@ThePhar i commited the change you requested

@black-sliver black-sliver requested a review from ThePhar November 25, 2023 11:04
@ThePhar ThePhar merged commit dd47790 into ArchipelagoMW:main Nov 25, 2023
12 checks passed
@Br00ty Br00ty deleted the ds3-early-banner branch November 26, 2023 00:14
Jouramie pushed a commit to Jouramie/Archipelago that referenced this pull request Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is: enhancement Issues requesting new features or pull requests implementing new features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants