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

TUNIC: Better seed groups for Entrance Rando #2998

Merged
merged 34 commits into from
May 3, 2024

Conversation

ScipioWright
Copy link
Collaborator

@ScipioWright ScipioWright commented Mar 20, 2024

What is this fixing or adding?

Makes it so that people who choose the same seed for entrance rando will have the same entrances, even if the settings that alter how entrances are placed (Laurels Location, Logic Rules, and Fewer Shops) are different. It takes the most restrictive of the settings for those in the group.

How was this tested?

Test generations with multiple players, with the above-stated options set to random, and entrance_rando set to a string.
And additional tests with multiple different entrance_rando seed groups, some set to 'true', some with it off, etc.

If this makes graphical changes, please attach screenshots.

N/A

@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Mar 20, 2024
@ScipioWright ScipioWright marked this pull request as draft March 20, 2024 00:58
@ScipioWright ScipioWright added is: enhancement Issues requesting new features or pull requests implementing new features. waiting-on: other Issue/PR is waiting for something else, like another PR. waiting-on: author Issue/PR is waiting for feedback or changes from its author. labels Mar 20, 2024
@ScipioWright ScipioWright marked this pull request as ready for review March 21, 2024 16:34
@ScipioWright ScipioWright removed waiting-on: author Issue/PR is waiting for feedback or changes from its author. waiting-on: other Issue/PR is waiting for something else, like another PR. labels Mar 21, 2024
ScipioWright and others added 2 commits March 24, 2024 11:32
Co-authored-by: Silent <110704408+silent-destroyer@users.noreply.github.com>
@silent-destroyer
Copy link
Contributor

Did a few test gens with different ER settings combinations and everything seemed to work fine

Copy link
Member

@NewSoupVi NewSoupVi left a comment

Choose a reason for hiding this comment

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

Tunc has some pretty deep conditionals sometimes that could probably be cleaned up, like the indentation level on some of these statements is a bit wacky

Other than that, this looks fine

worlds/tunic/__init__.py Outdated Show resolved Hide resolved
worlds/tunic/__init__.py Outdated Show resolved Hide resolved
worlds/tunic/__init__.py Outdated Show resolved Hide resolved
worlds/tunic/__init__.py Outdated Show resolved Hide resolved
worlds/tunic/__init__.py Outdated Show resolved Hide resolved
worlds/tunic/er_scripts.py Outdated Show resolved Hide resolved
@ScipioWright ScipioWright marked this pull request as draft March 29, 2024 13:43
@ScipioWright
Copy link
Collaborator Author

Drafting until I can test it all out, don't need a repeat of what happened before

@ScipioWright ScipioWright marked this pull request as ready for review March 30, 2024 00:44
@ScipioWright ScipioWright marked this pull request as draft April 19, 2024 21:32
@ScipioWright
Copy link
Collaborator Author

An issue was found, coverting to draft while fixing it

worlds/tunic/er_scripts.py Outdated Show resolved Hide resolved
Co-authored-by: Kaito Sinclaire <ks@rosenthalcastle.org>
Copy link
Contributor

@KScl KScl left a comment

Choose a reason for hiding this comment

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

After the latest fix, everything I've thrown at it to test appears to behave correctly:

  • Generates fine with multiple distinct seed groups in the same multiworld, and doesn't change anything for players also in that multiworld who just have ER on normally
  • More restrictive ER rules propagate to other players in the same seed group
  • plando_connections rules propagate to other players in the same seed group, and combine correctly if multiple players have them
  • Conflicting plando_connections rules between multiple players correctly errors out

@ScipioWright ScipioWright marked this pull request as ready for review April 19, 2024 22:56
@ScipioWright ScipioWright added waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. and removed waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Apr 19, 2024
Copy link
Member

@NewSoupVi NewSoupVi left a comment

Choose a reason for hiding this comment

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

Did a quick review of the changes since I last reviewed - Trusting the extensive integration test by KSci that explicitly mentions testing every case I would test as well

@NewSoupVi NewSoupVi merged commit 2618823 into ArchipelagoMW:main May 3, 2024
16 checks passed
@ScipioWright ScipioWright deleted the tunc-seed-groups branch May 3, 2024 13:08
jnschurig pushed a commit to Tranquilite0/Archipelago-SoulBlazer that referenced this pull request Jun 13, 2024
* Update entrance rando description to discuss seed groups

* Starting off, setting up some names

* It lives

* Some preliminary plando connection handling, probably has errors

* Add missed comma

* if -> elif

* I think this is working properly to handle plando connections

* Update comments

* Fix up shop -> shop portal stuff

* Add back comma that got removed for no reason in the ladder PR

* Remove unnecessary if else

* add back the actually necessary if but not the else

* okay they were both necessary

* Update entrance rando description

* blasphemy

Co-authored-by: Silent <110704408+silent-destroyer@users.noreply.github.com>

* Rename other instances of tunc -> tunic

* Update per Vi's review (thank you)

* Fix a not that shouldn't have been

* Rearrange, update per Vi's comments (thank you)

* Fix indent

* Add a .value

* Add .values

* Fix bad comparison

* Add a not that was supposed to be there

* Replace another isinstance

* Revise option description

* Fix per Kaito's comment

Co-authored-by: Kaito Sinclaire <ks@rosenthalcastle.org>

---------

Co-authored-by: Silent <110704408+silent-destroyer@users.noreply.github.com>
Co-authored-by: Kaito Sinclaire <ks@rosenthalcastle.org>
qwint pushed a commit to qwint/Archipelago that referenced this pull request Jun 24, 2024
* Update entrance rando description to discuss seed groups

* Starting off, setting up some names

* It lives

* Some preliminary plando connection handling, probably has errors

* Add missed comma

* if -> elif

* I think this is working properly to handle plando connections

* Update comments

* Fix up shop -> shop portal stuff

* Add back comma that got removed for no reason in the ladder PR

* Remove unnecessary if else

* add back the actually necessary if but not the else

* okay they were both necessary

* Update entrance rando description

* blasphemy

Co-authored-by: Silent <110704408+silent-destroyer@users.noreply.github.com>

* Rename other instances of tunc -> tunic

* Update per Vi's review (thank you)

* Fix a not that shouldn't have been

* Rearrange, update per Vi's comments (thank you)

* Fix indent

* Add a .value

* Add .values

* Fix bad comparison

* Add a not that was supposed to be there

* Replace another isinstance

* Revise option description

* Fix per Kaito's comment

Co-authored-by: Kaito Sinclaire <ks@rosenthalcastle.org>

---------

Co-authored-by: Silent <110704408+silent-destroyer@users.noreply.github.com>
Co-authored-by: Kaito Sinclaire <ks@rosenthalcastle.org>
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. waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants