Skip to content

Core: create the per world random object in the world constructor #2083

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Mar 10, 2024
Merged

Core: create the per world random object in the world constructor #2083

merged 11 commits into from
Mar 10, 2024

Conversation

@alwaysintreble
Copy link
Collaborator

@alwaysintreble alwaysintreble commented Aug 4, 2023 •
edited
Loading

What is this fixing or adding?

Honestly was stupid to not just do this in the first place when implementing random as an attr on the world itself. This does make seeds generated before this change different, but plan was to eventually drop per_slot_randoms completely anyways, so this just moves forward that particular breakage.
Main reason for doing this is because item link worlds don't have a random object associated so trying to use their random object during filler creation causes a crash. That in particular only affects the messenger and lufia2 but is easy to miss so could affect more on the future. @el-u so you're aware that Lufia currently crashes with item links without this change.

How was this tested?

Unit tests. More practical testing couldn't hurt, but this is relatively low impact.

@el-u
Copy link
Collaborator

el-u commented Aug 5, 2023

Can confirm, Lufia II item links with link_replacement: true are broken on main since 86a55c78 and are fixed by this PR.

@el-u el-u mentioned this pull request Aug 10, 2023
Merged
@ThePhar ThePhar added is: enhancement Issues requesting new features or pull requests implementing new features. affects: core Issues/PRs that touch core and may need additional validation. labels Aug 16, 2023
@alwaysintreble
Copy link
Collaborator Author

This is now reliant on #2290

@el-u el-u mentioned this pull request Oct 28, 2023
Merged
@el-u el-u mentioned this pull request Nov 5, 2023
Merged
@alwaysintreble alwaysintreble added is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. and removed is: enhancement Issues requesting new features or pull requests implementing new features. labels Dec 21, 2023
@alwaysintreble
Merge branch 'main' into random-constructor
52864f6 
# Conflicts:
#	BaseClasses.py
#	Utils.py
#	worlds/AutoWorld.py
@alwaysintreble
move random import and fix conflicts
7787364
@alwaysintreble alwaysintreble added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Feb 14, 2024
BadMagic100
BadMagic100 approved these changes Feb 19, 2024
View reviewed changes
Copy link
Collaborator

@BadMagic100 BadMagic100 left a comment

Choose a reason for hiding this comment

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

Looks good to me. I also wouldn't mind keeping the random assignment in set_seed and just assigning directly to world randoms instead of going via the dict but I don't really have a strong preference

@alwaysintreble alwaysintreble 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 Feb 19, 2024
@black-sliver
Copy link
Member

I also wouldn't mind keeping the random assignment in set_seed

I think having random available asap is a good thing, otherwise people may try to hack their way around it. The only downside i see is that you can not create a world before set_seed() now. I'm wondering If this should be detected - i.e. throw an exception in set_seed() if worlds are already constructed.

alwaysintreble added waiting-on: core-review

Tests are failing.

@black-sliver black-sliver added waiting-on: author Issue/PR is waiting for feedback or changes from its author. and removed waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. labels Feb 19, 2024
@alwaysintreble
Copy link
Collaborator Author

I'm wondering If this should be detected - i.e. throw an exception in set_seed() if worlds are already constructed.

Done.

@black-sliver
Copy link
Member

What do we label "waiting-on: other PR" as?

@black-sliver black-sliver added waiting-on: other Issue/PR is waiting for something else, like another PR. and removed waiting-on: author Issue/PR is waiting for feedback or changes from its author. labels Feb 19, 2024
@BadMagic100
Copy link
Collaborator

Regarding testing (once 2290 is merged) I would actually expect all the relevant code paths to be hit by automated tests here; since the change relates to multiworld instantiation even untested/poorly tested worlds should be ok because they won't be instantiating multiworld instances, or other worlds

@BadMagic100 BadMagic100 mentioned this pull request Mar 3, 2024
Merged
black-sliver
black-sliver reviewed Mar 10, 2024
View reviewed changes
test/general/__init__.py
Comment on lines +10 to +12
def setup_solo_multiworld(
world_type: Type[World], steps: Tuple[str, ...] = gen_steps, seed: Optional[int] = None
) -> MultiWorld:
Copy link
Member

Choose a reason for hiding this comment

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

Did you forget to use seed in set_seed() below?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no i just really hate the way dlcq and sdv do their tests

black-sliver
black-sliver approved these changes Mar 10, 2024
View reviewed changes
Copy link
Member

@black-sliver black-sliver left a comment

Choose a reason for hiding this comment

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

I'm fairly certain this is gonna break tests of worlds that have not been merged (or PR'd) yet, but I don't see a way to make this more back compatible, without adding duplicate world.random init, so we should just merge this.

@black-sliver black-sliver merged commit 2e1a5b0 into ArchipelagoMW:main Mar 10, 2024
@alwaysintreble alwaysintreble deleted the random-constructor branch March 10, 2024 17:48
EmilyV99 pushed a commit to EmilyV99/Archipelago that referenced this pull request Apr 15, 2024
@alwaysintreble @EmilyV99
Core: create the per world random object in the world constructor (Ar…
35bf9c6 
…chipelagoMW#2083)

* Core: create the per world random object in the world constructor

* remove the check that multiworld exists

* add a deprecation warning to per_slot_randoms

* move random import and fix conflicts

* assert worlds don't exist before setting the multiworld seed

* fix the dlcq and sdv tests

* actually use the seed
EmilyV99 pushed a commit to EmilyV99/Archipelago that referenced this pull request Apr 15, 2024
@alwaysintreble @EmilyV99
Core: create the per world random object in the world constructor (Ar…
9b25cc1 
…chipelagoMW#2083)

* Core: create the per world random object in the world constructor

* remove the check that multiworld exists

* add a deprecation warning to per_slot_randoms

* move random import and fix conflicts

* assert worlds don't exist before setting the multiworld seed

* fix the dlcq and sdv tests

* actually use the seed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@black-sliver black-sliver black-sliver approved these changes

+1 more reviewer

@BadMagic100 BadMagic100 BadMagic100 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

affects: core Issues/PRs that touch core and may need additional validation. is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. waiting-on: other Issue/PR is waiting for something else, like another PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@alwaysintreble @el-u @black-sliver @BadMagic100 @ThePhar