core: Region management helpers

[alwaysintreble]

exits is a dict of exit names to access rules. also call's the world's create_location for the location creation if they override it

[Berserker66]

the Locations created aren't the game's Location class if defined and have the wrong .game set.

[Jarno458]

Afaik the .game property on items and locations doesnt really do anything required. Timespinner has not been setting it and been working fine for like half a year now

[black-sliver]

I'm not 100% sure that helper is the best solution, because it lacks flexibility. It also adds a game to each location for no reason - currently we use class MyGameLocation which supplies game as a static member.

If such a helper is useful to multiple games, we could pass the class it should initialize

def create_reagion(..., LocationType: typing.Type[Location], ...):
    location = LocationType(....)

and either supply args or kwargs instead of a single int.

[alwaysintreble]

That was kind of the intent of calling create location which could then just be overridden but this is probably better. Should I use regiontype for an extended region class to be passed in?

[black-sliver]

Below is all my personal opinion. Berserker should maybe chime in and give us his preferred way of things.

1. With what i suggested above create_location would go, and only create_region would stay. Again, this only makes sense if create_region would be used by multiple worlds.

   you then end up with something like this (untested):

    def create_region(self, name: str, player: int, region_type: RegionType = None,
                      LocationType: Optional[typing.Type[Location]] = None,
                      locations: Dict[str, typing.Any] = {}, exits: Dict[str, Optional[Callable]] = {},
                      hint: str = None) -> Region:
        region = Region(name, region_type, hint, player, self)
        for name, args in locations.items():
            if not isinstance(args, list) and not isinstance(args, tuple):
                args = (args,)
            current_loc = LocationType(player, name, *args)
            current_loc.parent_region = region
            region.locations.append(current_loc)
        for name, rule in exits.items():
            region_exit = Entrance(player, name, region)
            if rule:  # has to be callable, don't assign None
                region_exit.access_rule = rule
            region.exits.append(region_exit)
   
        return region
   

2. or if you want to define create_location, i would suggest only having it in the World base class (not in the multiworld object), and have it throw a NotImplementedError, so the worlds have to override it. current_loc = LocationType(... *args) would then become w = self.worlds[player]; current_loc = w.create_location(... *args)

[black-sliver]

Should I use regiontype for an extended region class to be passed in?

Currently there is no reason to ever have a custom region type, I think (unless you want that for Lttp?). So whenever we need it, it could be added as arg to the end; with that kind of arguments we probably want to call it kwargs anyway, so the position of it does not really matter.

[Jarno458]

I dunno this method with alll its properties and those properties themself are collections of settings/rules It all gets quickly very confusing for me, and that comes from a person that does know a thing or two about how those internals work together

To keep it clean it would be much better if we use object orientation, and split things into different methods like region = create_region(self, name: str, player: int, region_type: RegionType = None) region.add_locations(LocationType: Optional[typing.Type[Location]] = None, locations: Dict[str, typing.Any] = {}) region.add_exits(exits: Dict[str, Optional[Callable]] = {})

[alwaysintreble]

I think Jarno's suggestion might be the way to go lol. I was trying to condense everything into one method since that's usually how the worlds do it and it simplifies your function calls but I think having this separated for you can have a cleaner create_regions method is a good way to go.

[black-sliver]

yeah, i thought the goal was

1. feed data
2. ???
3. ~~profit~~ have all locations

from a programming standpoint, using methods on regions is definitely better-looking, but you would still need multiple calls to convert data to locations

[alwaysintreble]

Small problem with putting the add_exits method on regions is that all of the regions need to exist for this to work. this would mean keeping or referencing the region cache in order to do it this way though I'm unsure if there's a better way to do that anyways.

1. feed data
2. ???
3. ~profit~ have all locations

I could create a separate create_regions method that can do this while still leaving everything more cleanly split out and easier to override. then you can just override a single method like the add_locations one and still call all of your data into the existing create_regions

[alwaysintreble]

I've got a mockup of what this generic create_regions function could look like here

def create_regions(self, regions: Dict[str, Optional[typing.Type[RegionType]]], player: int,
                  region_hints: Dict[str, str] = None, locations_per_region: Dict[str, Dict[str, Any]] = None,
                  LocationType: Optional[typing.Type[Location]] = None,
                  exits_per_region: Dict[str, Dict[str, Optional[Callable]]] = {}) -> None:
        for region, reg_type in regions.items():
            current_region = self.create_region(region, player, reg_type, region_hints[region])
            current_region.add_locations(LocationType, locations_per_region[region])
            self.regions.append(current_region)
        for region in exits_per_region:
            self.get_region(region, player).add_exits(exits_per_region[region])

[BootsinSoots]

Coming to this on recommendation, and I do like what's presented here. I notice that the create_region function does need to be updated, since RegionType has been removed