Celeste 64: v1.2 Content Update

[PoryGone]

Changelog:

## Features:

- New optional Location Checks
	- Friendsanity
	- Signsanity
	- Carsanity
- Move Shuffle
	- Basic movement abilities can be shuffled into the item pool
		- Ground Dash
		- Air Dash
		- Skid Jump
		- Climb
- Logic Difficulty
	- Completely overhauled logic system
	- Standard or Hard logic difficulty can be chosen
- Badeline Chasers
	- Opt-in options which cause Badelines to start following you as you play, which will kill on contact
	- These can be set to spawn based on either:
		- The number of locations you've checked
		- The number of Strawberry items you've received
	- How fast they follow behind you can be specified

## Quality of Life:

- The maximum number of Strawberries in the item pool can be directly set
	- The required amount of Strawberries is now set via percentage
	- All items beyond the amount placed in the item pool will be `Raspberry` items, which have no effect
- Any unique items placed into the `start_inventory` will not be placed into the item pool

Accompanying game release can be found here.

[Exempt-Medic]

Looked through the code, merged into main and did a few thousand generations without any issues. I don't know enough about the game to review the logic of rules.py. Made a few suggested changes

[PoryGone]

@Exempt-Medic
Friendly reminder bump that I've addressed or responded to all of your feedback.

[Exempt-Medic]

Merged into main, lots of test generations. Didn't look at rules.py much and I'm wondering if location_rule could be reformated to have less checking of the logic difficulty / move shuffling, but otherwise it looks good

[NewSoupVi]

Some earlier evalutions of the conditions inside the location_rule function would be quite important to me personally.

[NewSoupVi]

For performance, I would very much prefer if you moved as many conditions out of the actual access rule evaluation as possible, and did them ahead of time.
These if/else are going to be called each time an item is placed, so 1000s or 10000s of times, when it could be done once

For the record, you can also hope a different core maintainer gets to this who is less biased about performance stuff :)

pulling active_location_mapping assignment out of location_rule

The simplest change that sticks out to me immediately is moving active_location_mapping to something that that you set once.

Version 1 (The sane version)

The most elegant and simplest solution would probably be to make active_location_mapping an attribute of Celeste64World. So, move the if/else block somewhere else and save the attribute to the world instead. (set_rules would make sense to me, but PyCharm might complain about "setting attribute outside of __init__", maybe you can figure something out)

def set_rules(world: Celeste64World):
    if world.options.logic_difficulty == "standard":
        if world.options.move_shuffle:
            world.active_logic_mapping = location_standard_moves_logic
        else:
            world.active_logic_mapping = location_standard_logic
    else:
        if world.options.move_shuffle:
            world.active_logic_mapping = location_hard_moves_logic
        else:
            world.active_logic_mapping = location_hard_logic

    ...

and then in location_rule, you can just have:

def location_rule(state: CollectionState, world: Celeste64World, loc: str) -> bool:
    if loc not in world.active_logic_mapping:
        return True

    for possible_access in world.active_logic_mapping[loc]:
        if state.has_all(possible_access, world.player):
            return True

    return False

Version 2 (The slightly dank but even faster version)

Another alternative to consider would be to make location_rule a sub-function of set_rules, then you don't need the world. at all:

def set_rules(world: Celeste64World):
    ... (active_logic_mapping = stuff) ...
    def location_rule(state: CollectionState, loc: str):  # Don't even need "world" anymore at all
        if loc not in active_logic_mapping:
            return True
        ... (etc.) ...

I can understand if that's a bit too dank though. Either way, I would like to see this change implemented in some capacity.
At that point, I'd be willing to approve the PR (after reviewing everything else as well)

But, there are more optimisations to be made that I would recommend that I will detail here as well, assuming the world.active_location_mapping version but it would work with either.

moving the "in" check out of location_rule

Now that we have the active location mapping at an earlier point, instead of this "in" check inside the location_rule function, you could just do

def set_rules(world: Celeste64World):
    ... (setting world.active_location_mapping) ...

    for location in world.multiworld.get_locations(world.player):
        if location in world.active_location_mapping:
            set_rule(location, lambda state, location=location: location_rule(state, world, location.name))
        else:
            set_rule(location, lambda state: True)

With location_rule now being:

def location_rule(state: CollectionState, world: Celeste64World, loc: str) -> bool:
    for possible_access in world.active_logic_mapping[loc]:
        if state.has_all(possible_access, world.player):
            return True

    return False

any(...) instead of self written loop (but this one is debatable)

One final thing to consider, we could replace this early exiting for loop with an any

def location_rule(state: CollectionState, world: Celeste64World, loc: str) -> bool:
    return any(
        state.has_all(possible_access, world.player)
        for possible_access in world.active_logic_mapping[loc]
   )

However, while I find this more pythonic and prefer it personally, it is actually slower, so on this one I'm not so sure.

[PoryGone]

Is it worth being so particular about minutiae of performance in a world with a maximum of 46 locations?

[NewSoupVi]

I'll give my thoughts on that, sure. Let me put the "size of the game" thing aside for a second, I'll come back to it.

So, I've implemented the changes I suggested and I'm observing about a 30% speedup. It's not crazy good or anything, but considering:

1. These suggestions are fairly simple (in the sense that they don't increase overall line count and still keep all of the same code, just shuffled around)
2. this location_rule function is a change you're making from the original implementation (i.e. it is "in scope" of the PR) - where the previous system was "the fastest it can possibly be" (but also inflexible obviously)
3. "Avoid option checking inside of access rules" is a common piece of advice given by multiple world and core maintainers, like I'm not just pulling this out of my backside, there is some "consensus" around this - I disagree with the wording choice of "minutia" here because of this

It is something I will feel inclined to comment on.

Now, the size of the game might be a good argument, and I think I'm willing to let it go? But please please please consider the first suggestion I made (Setting active_logic_mapping as a world attribute world.active_logic_mapping once at the beginning of set_rules, and referring to that inside location_rule), it is very simple (literally just ctrl+x -> ctrl+v of that if-else block) and already gets 25% of a speedup, and it gets rid of the "option checking inside access rule" paradigm that is generally frowned upon. Everything else I suggested is minor and (while still tangible) gives only 1%-2% speedups ontop of that.

I will however say that I'm not sure I'm generically comfortable with the idea - and you didn't say this, but this would be my takeaway from this conversation - that I should take the size of a game into consideration when reviewing PRs like this (new game or big systems refactor). I'm generally trying to be consistent here, and this problem is something I'd absolutely be calling out and not letting fly for a 500 location game - Is it that fair or reasonable for me to be softer on it when the game is smaller? I'm not sure about that, genuinely I will have to think about it.

[PoryGone]

I truly don't think it would make a difference of more than a couple of seconds of gen in any real scenario, but I moved the if checks outside of the logic rule for you.

[NewSoupVi]

Clicked the wrong damn button again, did not mean to approve

[NewSoupVi]

Code LGTM, very clean

Before I hit the button, I think I would like either a review from someone who knows the game, or a record that playtests were run / the new version has successfully been played in unsupported games.
Mostly to make sure that the logic stuff is correct, as neither Exempt-Medic (I think) nor I know anything about the game and have purely looked at this from a code quality / generation perspective

If playtests have already been run, then we're good obviously, just let me know :)

[PoryGone]

Code LGTM, very clean

Before I hit the button, I think I would like either a review from someone who knows the game, or a record that playtests were run / the new version has successfully been played in unsupported games. Mostly to make sure that the logic stuff is correct, as neither Exempt-Medic (I think) nor I know anything about the game and have purely looked at this from a code quality / generation perspective

If playtests have already been run, then we're good obviously, just let me know :)

I always run public playtests for content PRs, and always run private playtests before the public even hears about the update existing. We had a decent number of players in the public test, and no logic issues came up.
If my word isn't enough, @Silvris participated.

[NewSoupVi]

Good enough for me :)