Core: replace Location.event with advancement property

[alwaysintreble]

What is this fixing or adding?

title. removes a ton of unnecessary event assignments in world implementations too.

How was this tested?

ran --multi 100 --seed 1 --skip_prog_balancing --spoiler 0 --skip_output with effected games and messenger. interestingly i would expect this to run slower but in my tests it seemed to run a bit faster? more testing to confirm this would be good.

If this makes graphical changes, please attach screenshots.

[alwaysintreble]

@Alchav @lordlou @Rosalie-A

[beauxq]

I think this is how I would format this:

            if location.progress_type != LocationProgressType.EXCLUDED and (
                location.player in players["locations"] or location.advancement
            ):

Not saying this is better, just presenting another option in case you were looking for one.

[alwaysintreble]

I think I'd rather leave it how it is due to it being the current style in AP and I don't think we ever decided on which one we should use going forward.

[t3hf1gm3nt]

Ran a few tests gens with a TLOZ yaml and checked the spoiler to make sure the events we have for the playthrough still showed up. all clear there so good in my book.

as for the claim that this change would make things faster, that i found inconclusive. tested with the same yaml with seeds 1, 10, and 100 as well as a few completely random seeds between current main and this PR, and sometimes main was quicker and other times the PR was quicker. only by 1 or 2 tenths of a second, think the only outlier was 5 tenths at most. EDIT: im a dumb dumb and realize that not running at the very least --skip_output might be throwing the times off, so throw my speed testing into the bin. i might look into running proper tests for that claim at a later time but tbh not worried about that personally

[Jarno458]

the change to Timespinner LGTM, i didnt actually test it, but based on the logic the event bool got removed so it makes sense to remove it like you did

[axe-y]

I see 3 names change in DLC Quest, if it work it should be good.

[Ziktofel]

For SC2 it's only removing dead code. See Ziktofel#171

[alwaysintreble]

@jonloveslegos

[Jouramie]

I ran Stardew's performance tests with the 5.x.x branch. I take a little more time than before, but the difference is inside the std, so I would call that insignificant.

However, I ran --skip_output --seed 1 --multi 50 on DLC Quest with Coinsanity, and it seems around 5% slower. Not sure if it's just my low sample of 3 runs or why it would be slower...

[black-sliver]

Checking Location.advancement should be slower than checking the old Location.event, but skips potential writes to Location.event (and fits more in cache), so the impact will depend on "simplicity" of fill.

I tested a couple of things (including 500 DLCQuest+coinsanity without prog balancing) and I get a slowdown of 1-2% across the board, which may or may not be noise. I've never seen a difference as high as 5%.

[Jouramie]

I was testing with prog balancing, I would assume that's why I saw a bigger difference.

[black-sliver]

Prog balancing also had only 2% difference for me

[alwaysintreble]

machine specs could very easily influence it, but somewhere around a 1-5% drop sounds about right to me. i do think the rename is worth it but i'm unsure if swapping this to a property is worth it. i looked a bit into making CollectionState.events a more effective cache that would invalidate the performance loss here (and possibly give us a bit of gain), but that would be more of a rework that's very out of scope for this. if we don't think the property is worth the performance loss, i'm happy to change it back to an attribute since with the cleanup i've already done here it would only be a few lines of code.

[Alchav]

idc

[FlySniper]

Wargroove changes look good to me.

[espeon65536]

oot generates and I didn't find anything that would break with grep, hopefully some obscure setting combo didn't slip by my notice

[jonloveslegos]

Barely anything was changed with Undertale, and what did change looks fine to me.

[nbrochu]

Hmm if this is to be merged for 0.4.5, I think Zork will need a tweak too.

                location.event = isinstance(location_enum_item, ZorkGrandInquisitorEvents)

                if location.event:
                    location.place_locked_item(
                        ZorkGrandInquisitorItem(
                            data.event_item_name,
                            ItemClassification.progression,
                            None,
                            self.player,
                        )
                    )

Would need to become (?)

                if isinstance(location_enum_item, ZorkGrandInquisitorEvents):
                    location.place_locked_item(
                        ZorkGrandInquisitorItem(
                            data.event_item_name,
                            ItemClassification.progression,
                            None,
                            self.player,
                        )
                    )

I guess I should have never been manually setting .event anyway

[nbrochu]

Might be worth checking the other new games too, just in case

[Jouramie]

I looked again and found this. Otherwise, LGTM for Stardew.

[nbrochu]

Zork LGTM

[lordlou]

Didnt test SMZ3 but looks good to me.