EventManager refactoring

[cppcooper]

As in the title, this fixes #2031 and also as (currently) mentioned, this is a work in progress. I wouldn't have opened the PR if not for the commits that I also wouldn't have made if not for needing to checkout master and build/install.

One major change so far is that the BUILDING event now sends destroyed buildings as negative id values, meaning they need to be handled appropriately by any users (of which I only found one outside-only.lua via eventful; it doesn't even use the id values). now comes with component events (created/destroyed) that can be used instead of it.

edit: so this PR did fix #2031 orginally, and still does.. but #2031 has "officially" been fixed by reverting #1876 [for the most part]. So this [effectively] re-implements #1876, but with fixes for #2031. As of writing this edit, there are some other nice changes such as to the function lists which needed manual synchronization with the enum event order. Now the order is automatically generated for these function arrays and matches that of the EventType enums. A positive side of that particular change is that the compiler errors when new events are added but not integrated into switches used to generate these arrays (there are 2, one in EventManager.cpp and one in eventful.cpp)

[cppcooper]

So according to the tests I've run, everything I've touched is working as we would hope it to. I haven't performed extensive testing but all the events appear to be firing as they had before from what I can tell, and the backlogs are sent to delayed listeners.

What's left, appear to be spaghetti events and I dunno how long they'll take me just to figure out. So far the toughest has taken maybe a couple hours total.

Here's a list of the remaining

1. updateReportToRelevantUnits
2. manageReportEvent
3. getWound
4. manageUnitAttackEvent
5. getVerb
6. getAttacker
7. gatherRelevantUnits
8. manageInteractionEvent

[cppcooper]

It's a bit unclear how useful this actually is. Users would still need to find the report from the id. So I think this is a bit redundant. Worst case scenario, we've taken away useful information. Best case we've prevented a user from erroneously assuming the id exists

[myk002]

I'm glad this is getting better organization, testing, and data structures, but it's a lot of code to review at once. Is there anything in particular that you'd like feedback on?

one thing I can suggest from personal experience: could you guarantee that the *destroyed events fire before the associated *created events? sometimes the same IDs are reused, and it's better to give callers the opportunity to clear out old data before new data is added. For example, our current building cache in the Buildings module has a bug that is triggered by the current (checked-in) behavior of sending created events before destroyed events.

[cppcooper]

it's a lot of code to review at once.

Indeed it is, sorry. It'd probably be best to checkout the branch and read it in your IDE than suffer through the massive diffs. I did comment most stuff though, so there's that. The last few managers were too complex for me to take a crack at rewriting their update code for optimization.. so hopefully they are already optimized ([X] to doubt).

Is there anything in particular that you'd like feedback on?

Not really no. Just the regular review process. Requested it simply to notify you that this is done.

could you guarantee that the *destroyed events fire before the associated *created events?

What exactly do you mean? Afaik/iirc, manageEvents iterates the events according to the EventType enum order, and I placed the events in created/destroyed order. Though I separated the created/destroyed into new events I retained the old one.. so until they are fully removed it can't be guaranteed for the separated versions.

[myk002]

#2056 has been merged. Could you merge develop into this branch and overwrite the changes in EventManager.h and EventManager.cpp?

[cppcooper]

#2056 has been merged. Could you merge develop into this branch and overwrite the changes in EventManager.h and EventManager.cpp?

As soon as I get the chance

[cppcooper]

As far as I am aware I've done everything necessary to fix #2031 in this PR.

Quite a bit of refactoring, so lots of code to review. If there are comments needed somewhere in the diffs lmk, I have an awful memory so it'll help me too when/if other things are reviewed and I've forgotten everything about the PR.

[myk002]

I think I'll have to review the rest on an actual computer. Phone is not adequate.

[cppcooper]

I was thinking about unit testing, and I had this idea. So I wrote it out.. not sure how well it would work. It would at least simplify the code generation.

/* EVENT_TEST and the following are intended to aid
 * in the creation of a unit test for events...
 * it can't help create testing conditions...
 * but it can help ease testing
 */
#define EVENT_TEST_PTR(ev,type,data_type)                   \
    data_type ev##_q;                                       \
    void on_##ev(color_ostream&, void* d);                  \
    EM::EventHandler ev##_evt(on_##ev,0);                   \
                                                            \
    void on_##ev(color_ostream&, void* d) {                 \
        static bool run_once = false;                       \
        if(!run_once) {                                     \
            run_once = true;                                \
            ev##_q = (data_type)d;                          \
            EM::unregister(type, ev##_evt, plugin_self);    \
        }                                                   \
    }                                                       \
                                                            \
    void on_##ev##2(color_ostream&, void* d);               \
    EM::EventHandler ev##_evt2(on_##ev##2,1200);            \
                                                            \
    void on_##ev##2(color_ostream&, void* d) {              \
        static bool run_once = false;                       \
        if(!run_once) {                                     \
            run_once = true;                                \
            assert( ev##_q == (data_type)d );               \
            EM::unregister(type, ev##_evt2, plugin_self);   \
        }                                                   \
    }                                                       \
                                                            \
    void register_##ev(){                                   \
        EM::registerListener(type, ev##_evt, plugin_self);  \
        EM::registerListener(type, ev##_evt2, plugin_self); \
    }

#define EVENT_TEST_INT(ev,type)                             \
    int32_t ev##_q;                                         \
    void on_##ev(color_ostream&, void* d);                  \
    EM::EventHandler ev##_evt(on_##ev,0);                   \
                                                            \
    void on_##ev(color_ostream&, void* d) {                 \
        static bool run_once = false;                       \
        if(!run_once) {                                     \
            run_once = true;                                \
            ev##_q = (int32_t)intptr_t(d);                  \
            EM::unregister(type, ev##_evt, plugin_self);    \
        }                                                   \
    }                                                       \
                                                            \
    void on_##ev##2(color_ostream&, void* d);               \
    EM::EventHandler ev##_evt2(on_##ev##2,1200);            \
                                                            \
    void on_##ev##2(color_ostream&, void* d) {              \
        static bool run_once = false;                       \
        if(!run_once) {                                     \
            run_once = true;                                \
            assert( ev##_q == (int32_t)intptr_t(d) );       \
            EM::unregister(type, ev##_evt2, plugin_self);   \
        }                                                   \
    }                                                       \
                                                            \
    void register_##ev(){                                   \
        EM::registerListener(type, ev##_evt, plugin_self);  \
        EM::registerListener(type, ev##_evt2, plugin_self); \
    }

EVENT_TEST_INT(unit_new_active, Event::UNIT_NEW_ACTIVE);
EVENT_TEST_INT(unit_death, Event::UNIT_DEATH);
EVENT_TEST_INT(item_created, Event::ITEM_CREATED);
EVENT_TEST_INT(building_destroyed, Event::BUILDING_DESTROYED);
EVENT_TEST_INT(building_created, Event::BUILDING_CREATED);
EVENT_TEST_INT(building, Event::BUILDING);
EVENT_TEST_INT(invasion, Event::INVASION);
EVENT_TEST_INT(report, Event::REPORT);

EVENT_TEST_PTR(job_initiated, Event::JOB_INITIATED, df::job*);
EVENT_TEST_PTR(job_started, Event::JOB_STARTED, df::job*);
EVENT_TEST_PTR(job_completed, Event::JOB_COMPLETED, df::job*);
EVENT_TEST_PTR(construction_removed, Event::CONSTRUCTION_REMOVED, df::construction*);
EVENT_TEST_PTR(construction_added, Event::CONSTRUCTION_ADDED, df::construction*);
EVENT_TEST_PTR(construction, Event::CONSTRUCTION, df::construction*);
EVENT_TEST_PTR(syndrome, Event::SYNDROME, EM::SyndromeData*);
EVENT_TEST_PTR(inventory_change, Event::INVENTORY_CHANGE, EM::InventoryChangeData*);
EVENT_TEST_PTR(unit_attack, Event::UNIT_ATTACK, EM::UnitAttackData*);
EVENT_TEST_PTR(interaction, Event::INTERACTION, EM::InteractionData*);

[myk002]

That would be useful for a C++ unit test, but the problem is that we don't have any infrastructure for running C++ unit tests. All the C++ code is exercised over the Lua interface layer by unit tests written in Lua : /

see: https://github.com/DFHack/dfhack/tree/develop/test
and https://github.com/DFHack/scripts/tree/master/test

[myk002]

Here's a client you can use for eventful testing:

edit: actually, let me just check that in. now available as devel/eventful-client

[myk002]

From playing with the client, it looks like eventful is lowest-frequency based, so if multiple (lua) listeners are registered for an event, they will all fire at the minimum of all requested frequencies for that event.

[cppcooper]

From playing with the client, it looks like eventful is lowest-frequency based, so if multiple (lua) listeners are registered for an event, they will all fire at the minimum of all requested frequencies for that event.

This just furthers my point above:

if we always want events to be exported to lua, then it would make more sense to just do what eventful does in the event manager over having a plugin provide that

[myk002]

also, devel/eventful-client doesn't find the new unit handler function since it is named differently from the related event. I assume you'll want to fix that so I didn't special-case it in the client code. It will pick it up once you rename the function from onUnitNewActive to onNewUnitActive (or was it the other way around... is it the event type name that needs changing? well, anyway, once they line up, the script will find it)

[myk002]

if we always want events to be exported to lua, then it would make more sense to just do what eventful does in the event manager over having a plugin provide that

I'm not so sure -- I think there is value in not having EventManager have to think about Lua. That's too much complexity in one place. There'd still have to be a Lua translation layer that maps EventManager constructs onto Lua constructs. I think that separating that out into a different unit is the right move. Whether that unit is in core or in a plugin is a different discussion, but I'm not sure if it would change the algorithm much. It would have the same API access to EventManager.

edit: I can see why the Lua layer might be frustrating. It does make your vision of per-handler frequency guarantees more difficult to achieve.