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

Core: Region connection helpers #1923

Merged
merged 8 commits into from
Jul 9, 2023

Conversation

alwaysintreble
Copy link
Collaborator

@alwaysintreble alwaysintreble commented Jul 1, 2023

Please format your title with what portion of the project this pull request is
targeting and what it's changing.

ex. "MyGame4: implement new game" or "Docs: add new guide for customizing MyGame3"

What is this fixing or adding?

Region.connect and Region.create_exit. latter returns an entrance that can be used for other things (?) and former allows easy connection between two regions without needing the full structure of add_exits. Also added deprecation warning to creating Entrance elsewhere, since there's a ton of duplicated code from people doing so. Made it a separate commit so it can be easily dropped if desired.

How was this tested?

unit tests and checking that the logs print the warning correctly

If this makes graphical changes, please attach screenshots.

@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 Jul 1, 2023
@el-u
Copy link
Collaborator

el-u commented Jul 1, 2023

  • Is it a good idea to inspect the stack on every entrance cration?
  • How is this gonna work out for users of custom subclasses of Entrance? (Those can't be created using these new methods, so they'd still have to create them manually. And if they follow the recommended practice of calling super().__init__, they will be hit with the deprecation message as well.)
  • Can Region.add_exits be rewritten to use Region.connect inside the for-loop in order to reduce code duplication?
  • I found the argument name exiting_region confusing since this is supposed to be the region that the entrance will lead to, not the one it is exiting from.

@alwaysintreble
Copy link
Collaborator Author

alwaysintreble commented Jul 1, 2023

  • Is it a good idea to inspect the stack on every entrance cration?

Probably not but this was the first thing I thought of. I'm open to a better way to handle this.

  • How is this gonna work out for users of custom subclasses of Entrance? (Those can't be created using these new methods, so they'd still have to create them manually. And if they follow the recommended practice of calling super().__init__, they will be hit with the deprecation message as well.)

Could update create exit so it accepts a class like add_locations or alternatively it could just be subclassed. I feel like if you're subclassing Entrance, you're likely subclassing Region already.

@ThePhar
Copy link
Member

ThePhar commented Jul 6, 2023

I'm not a fan of marking direct Entrance creation as "deprecated". Obviously, want to encourage folks to use the AP helper methods, but if folks know what they're doing and have good reason to, I don't want to nag them with a deprecation warning, implying it's going to be removed.

And that's better than making a monster of a helper method that can handle every possible use case.

BaseClasses.py Outdated Show resolved Hide resolved
BaseClasses.py Outdated Show resolved Hide resolved
BaseClasses.py Outdated Show resolved Hide resolved
@alwaysintreble
Copy link
Collaborator Author

alwaysintreble commented Jul 6, 2023

Screenshot 2023-07-06 053933
I'll drop the warning once a decision is made on how to handle it. I can't think of any reason why someone would still need to create them manually here, though I'm willing to concede it being a possibility. I also know that people won't change or use any of the "proper" API as long as the old stuff exists and works.
Also edit: i realized the assert here didn't work, so I put it at the end of my add_exits after making sure to call super and it passed lmao

BaseClasses.py Outdated Show resolved Hide resolved
BaseClasses.py Outdated Show resolved Hide resolved
BaseClasses.py Outdated Show resolved Hide resolved
BaseClasses.py Outdated
"""Adds locations to the Region object, where location_type is your Location class and locations is a dict of
location names to address."""
def add_locations(self, locations: Dict[str, Optional[int]],
location_type: Optional[typing.Type[Location]] = None) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's a bit of an asymmetry between add_locations, which takes the location_type as an argument, and create_exit, which reads the entrance_type from an attribute.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the asymmetry is ok. There's use cases where we might want multiple Location classes in a Region (Messenger already does this), though very unlikely we have the same for Entrances.

BaseClasses.py Outdated Show resolved Hide resolved
BaseClasses.py Outdated Show resolved Hide resolved
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.

Think this is fine now. Further improvements can be done on top of this without breaking backwards compat.

Adding a warning / deprecating the old way of things should be opened as separate PR and merged once we know for sure this solution is flexible enough to update all worlds.

@black-sliver black-sliver merged commit 07d74ac into ArchipelagoMW:main Jul 9, 2023
@el-u el-u mentioned this pull request Jul 12, 2023
@alwaysintreble alwaysintreble deleted the region_connect branch July 19, 2023 21:46
FlySniper pushed a commit to FlySniper/Archipelago that referenced this pull request Nov 14, 2023
* Region.create_exit and Region.connect helpers

* reduce code duplication and better naming in Region.connect

* thank you tests

* reorder class definition

* define entrance_type on Region

* document helpers

* drop __class_getitem__ for now

* review changes
Jouramie pushed a commit to Jouramie/Archipelago that referenced this pull request Feb 28, 2024
* Region.create_exit and Region.connect helpers

* reduce code duplication and better naming in Region.connect

* thank you tests

* reorder class definition

* define entrance_type on Region

* document helpers

* drop __class_getitem__ for now

* review changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: core Issues/PRs that touch core and may need additional validation. 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.

4 participants