Eventmanager cleanup #3818

master-spike · 2023-09-27T20:16:33Z

Pulled out the event handler calls outside of the iterations over df::global structures so that if any of the calls to handleEvent change those structures those iterators aren't invalidated. Also made a few changes to tidy up some things here and there.

Changed the way manageConstructionEvent() works so that - rather than doing a binary search for every one of the constructions in our memorised set to check if its been destroyed, instead does a linear pass over the game's construction vector (which had to be done regardless to check for new constructions) and moves those constructions which intersect to a new set, and whatever remains in the old set is our destroyed constructions.

Range based for loops over map is now written as a structured binding for clarity. (no more ugly .second).

In manageUnitAttackEvent replaced the weird alreadyDone (it was a map of maps to ints which represented bools?) with a simple hashset of pairs (hash function provided - hash_pair) to check if that interaction was already done.

…esholds for combat skill effectiveness formulas to have a higher 100 cap (more descriptive about very strong warriors).

we, um, had it backwards

library/xml: master scripts: master

…oid potential iterator invalidation by an event callback, and did some tidying

frznd · 2023-09-27T22:30:17Z

library/modules/EventManager.cpp

@@ -902,6 +935,26 @@ static void manageEquipmentEvent(color_ostream& out) {
            equipment.push_back(item);
        }
    }
+
+    // now handle events
+    std::for_each(equipment_pickups.begin(), equipment_drops.end(), [&](InventoryChangeData& data) {


equipment_pickups != equipment_drops.

Is this intentional?

no, it was not

myk002 · 2023-09-28T06:30:24Z

library/modules/EventManager.cpp

+        for (auto &[_,handle] : copy) {
+            // the jobs must have a worker to start
+            DEBUG(log,out).print("calling handler for job started event\n");
+            handle.eventHandler(out, job);


this still suffers from the problem that if a handler cancels the job (which deletes it and removes it from the list), it will crash (probably).

to improve safety, it might be better to move the handler calls back inline and check after each handler call that the previous link still points to this job

myk002 · 2023-09-28T06:37:11Z

library/modules/EventManager.cpp

+    for (df::job* job : new_jobs_completed_repeats) {
+        for (auto &[_,handle] : copy) {
+            DEBUG(log,out).print("calling handler for repeated job completed event\n");
+            handle.eventHandler(out, (void*) job);
+        }


what does moving this logic gain us? the prevjobs structure cannot be modified by the handlers (though the jobs could potentially be deleted, which would cause crashes both in the original code and in the proposed change)

Yeah, there might be no way around that. Somehow I'm able to hold a job open in the gm-editor with auto-update long after its completion though, when it's no longer findable in the linked list? I might do some experiments later to determine the lifetime of a job.

Actually, I think it's fine - the jobs in prevJobs are actually cloned job structures (by Job::cloneJobStruct()) and so won't be deleted by the game.

oh that's good - I thought I remembered something like that, but I didn't see it when I was scanning the code for the review. So, this part can be refactored or not. You can refactor it for consistency with the other handlers or revert this section for fewer local variables. I'm ok with either way.

myk002 · 2023-09-28T06:45:06Z

library/modules/EventManager.cpp

+            handle.eventHandler(out, (void*)intptr_t(building));
+        }
+    });
+
    //now alert people about destroyed buildings


could you take this opportunity to move the destroyed buildings events before the new building events? this would solve a longstanding bug in our building location cache where a new building is added in the spot a removed building just vacated.

thanks! I keep on forgetting to do that. it's been bugging me for more than a year. It wasn't causing errors, just unnecessary O(N) lookups.

myk002 · 2023-09-28T06:51:00Z

could you also make sure to test all these event code changes? you can use devel/eventful-client to register debug listeners and sanity check behavior

master-spike · 2023-09-28T07:41:06Z

Interestingly in testing I've found a bug in the pre-existing code that fires unit death events when units leave the map - probably because the only check it does is whether the unit is active or not

…evious in linked list in case it is removed. fix a bug with item change events and using invalidated stack pointer as item data. swapped order of destroyed and created building event calls, and a couple other improvements

master-spike · 2023-09-28T15:54:24Z

As of the latest changes I have tested the events for the following events (using 2 forts, one with 300 pops and another newly embarked)
UNIT_NEW_ACTIVE - works as expected
UNIT_DEATH - works as expected (note: changed behaviour - now doesn't register units leaving the map as deaths)
JOB_COMPLETED - works as expected. For cancelled and repeated jobs works as expected too.
ITEM_CREATED - works as expected.
JOB_INITIATED - works as expected.
JOB_STARTED - works as expected.
CONSTRUCTION - works as expected.
BUILDING - works as expected.
SYNDROME - works as expected. (i.e. I see when dwarves in the tavern are getting drunk and stuff)
INVENTORY_CHANGE - works as expected (tested equipment use changes by drafting and undrafting squads so they'd strap their gear then put it in their hand and back again)
UNIT_ATTACK - works as expected

All the other event types I didn't test as I didn't change them.

myk002 · 2023-09-28T21:05:28Z

As of the latest changes I have tested the events for the following events

Awesome! Thank you!

Edit: just to check, did you test with more than one registered handler? Whenever we've found bugs in this code the past, it was with that use case.

myk002

please add changelog entries for:

fixing the unit dead event (Fixes)
changing the order of the events in the building handler (Misc Improvements)
protecting against list/vector modification while sending events (Misc Improvements instead of Fixes since this wasn't actively causing reported crashes)

myk002 · 2023-09-28T21:08:25Z

library/modules/EventManager.cpp

    for (auto unit : df::global::world->units.all) {
        //if ( unit->counters.death_id == -1 ) {
        if ( Units::isActive(unit) ) {
            livingUnits.insert(unit->id);
            continue;
        }
+        if (!Units::isDead(unit)) continue; // for units that have left the map but aren't dead


I had to check the difference between isDead() and ! isAlive() -- you chose the correct one.

master-spike · 2023-09-29T09:03:17Z

As of the latest changes I have tested the events for the following events

Awesome! Thank you!

Edit: just to check, did you test with more than one registered handler? Whenever we've found bugs in this code the past, it was with that use case.

Ah, I did not for everything, I'll go ahead and double check that.

master-spike · 2023-09-29T14:38:57Z

I have tested everything again with multiple listeners of each type and also mixed in different types together. Seems to work as expected, and I did stress-test certain conditions and not getting weird behaviour.

myk002 · 2023-09-29T16:37:53Z

fantastic! Thank you!

Mikhail and others added 7 commits September 27, 2023 20:55

Removed redundant uppercase in mental stability formula. Reworked thr…

32a2d9f

…esholds for combat skill effectiveness formulas to have a higher 100 cap (more descriptive about very strong warriors).

Updated changelog.txt

73fed1e

add detailed focus strings for setupdwarfgame

6ad724b

align mouse button semantics to DF

5c670d2

we, um, had it backwards

Auto-update submodules

888c88d

library/xml: master scripts: master

pulled event handler calls out of the loops over global vectors to av…

f5aab7e

…oid potential iterator invalidation by an event callback, and did some tidying

Merge branch 'develop' into eventmanager_cleanup

5f8a68c

frznd reviewed Sep 27, 2023

View reviewed changes

master-spike added 2 commits September 27, 2023 23:34

fixed a typo brain fart moment

78bea2a

Merge branch 'develop' into eventmanager_cleanup

7203e14

myk002 reviewed Sep 28, 2023

View reviewed changes

master-spike added 2 commits September 28, 2023 16:19

Merge branch 'develop' into eventmanager_cleanup

14871c9

myk002 reviewed Sep 28, 2023

View reviewed changes

master-spike added 2 commits September 29, 2023 14:54

Merge branch 'develop' into eventmanager_cleanup

bb9829e

update changelog

feaf0d6

myk002 approved these changes Sep 29, 2023

View reviewed changes

myk002 merged commit 27c85a9 into DFHack:develop Sep 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eventmanager cleanup #3818

Eventmanager cleanup #3818

master-spike commented Sep 27, 2023

frznd Sep 27, 2023

master-spike Sep 27, 2023

myk002 Sep 28, 2023

myk002 Sep 28, 2023

master-spike Sep 28, 2023

master-spike Sep 28, 2023

myk002 Sep 28, 2023

myk002 Sep 28, 2023

master-spike Sep 28, 2023

myk002 Sep 28, 2023

myk002 commented Sep 28, 2023

master-spike commented Sep 28, 2023

master-spike commented Sep 28, 2023

myk002 commented Sep 28, 2023 •

edited

Loading

myk002 left a comment

myk002 Sep 28, 2023

master-spike commented Sep 29, 2023

master-spike commented Sep 29, 2023

myk002 commented Sep 29, 2023

Eventmanager cleanup #3818

Eventmanager cleanup #3818

Conversation

master-spike commented Sep 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myk002 commented Sep 28, 2023

master-spike commented Sep 28, 2023

master-spike commented Sep 28, 2023

myk002 commented Sep 28, 2023 • edited Loading

myk002 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

master-spike commented Sep 29, 2023

master-spike commented Sep 29, 2023

myk002 commented Sep 29, 2023

myk002 commented Sep 28, 2023 •

edited

Loading