EventPlayer: Bugfix multiple conditionals #1642

avanwinkle · 2022-06-27T19:15:12Z

SUMMARY

This PR fixes a regression where an EventPlayer configuration meant to post the same event under different conditions would only successfully post when the last condition was true.

DETAILS

Performance optimizations in aa725f2 restructured the parsing of EventPlayer configs to initialization, and uses a dictionary to store the configs for each event to be posted. By moving from an iteration to a dict lookup, runtime performance is improved—however the same event appearing multiple times (with different conditions) would be overwritten because the dictionary key is only the event itself, not the conditions attached.

This PR tweaks the behavior to store the dict value as a list of configs, rather than a single config. When an event is to be posted, the list of configs is iterated and evaluated.

Performance Implications
I did a timeit comparison between (1) always making the dict value a list and iterating, even when there's only one entry, and (2) checking whether the dict value is a list and only iterating if it is. The results were not significantly different, but the all-lists approach performed slightly better.

def test_lists():
    test_var = None
    for event, items in dict_only_lists.items():
        for item in items:
            test_var = item["name"]

def test_mixed():
    test_var = None
    for event, s in dict_mixed.items():
        if isinstance(s, list):
            for item in s:
                test_var = item["name"]
        else:
            test_var = s["name"]

Checking for 10,000 iterations
 - Every value a list: 0.005966
 - Mixed lists and configs: 0.006381
Checking for 50,000 iterations
 - Every value a list: 0.024976
 - Mixed lists and configs: 0.025874
Checking for 100,000 iterations
 - Every value a list: 0.045673
 - Mixed lists and configs: 0.051380

I chose the all-list approach not really because of the performance difference, but because it's easier and more predictable code when all values are the same shape.

TESTING

I wrote some tests to cover the scenario, and verified that they failed in the current dev and passed on this branch. I also ran Mass Effect 2 against this branch and verified that the regressions no longer reproed.