Adds an onUnitAction event #2094

Putnam3145 · 2022-04-13T21:28:34Z

I've had an onUnitAction event that I've wanted to move to C++ since, oh, 2015. I only got DFHack compiled recently, so here it is.

Tested to work with my mod that actually uses this, but that's just on windows.

…action

myk002 · 2022-04-14T03:11:10Z

library/modules/EventManager.cpp

+        return;
+    multimap<Plugin*,EventHandler> copy(handlers[EventType::UNIT_ACTION].begin(), handlers[EventType::UNIT_ACTION].end());
+    for ( size_t a = 0; a < df::global::world->units.all.size(); a++ ) {
+        df::unit* unit = df::global::world->units.all[a];


Since you don't use the index for anything other than getting the unit out of the vector, consider
for (auto unit : df::global::world->units.all)

I agree. Fairly sure I've replaced any that could be via clion's clang-tidy suggestions. So if I end up merging these into PR #2044 I'll be getting this one too.

myk002 · 2022-04-14T03:15:18Z

library/modules/EventManager.cpp

+    multimap<Plugin*,EventHandler> copy(handlers[EventType::UNIT_ACTION].begin(), handlers[EventType::UNIT_ACTION].end());
+    for ( size_t a = 0; a < df::global::world->units.all.size(); a++ ) {
+        df::unit* unit = df::global::world->units.all[a];
+        if ( Units::isActive(unit) ) {


To reduce indentation, how about

if (!Units::isActive(unit)) { unitToKnownActions.erase(unit->id); continue; }

This way you don't need an else clause

I dunno, I wouldn't worry about it until it's not just the indentation but also the length of the block below. This all kinda fits on one screen.

Nah, I generally prefer things be structured in the way mentioned for stuff like this, I just sort of forget to do it

myk002 · 2022-04-14T03:17:23Z

library/modules/EventManager.cpp

+    for ( size_t a = 0; a < df::global::world->units.all.size(); a++ ) {
+        df::unit* unit = df::global::world->units.all[a];
+        if ( Units::isActive(unit) ) {
+            auto knownActions = &unitToKnownActions[unit->id];


consider using a reference instead of a pointer:
auto & knownActions = unitToKnownActions[unit->id];

myk002 · 2022-04-14T03:22:53Z

plugins/eventful.cpp

@@ -220,6 +223,10 @@ static void ev_mng_interaction(color_ostream& out, void* ptr) {
    EventManager::InteractionData* data = (EventManager::InteractionData*)ptr;
    onInteraction(out, data->attackVerb, data->defendVerb, data->attacker, data->defender, data->attackReport, data->defendReport);
 }
+static void ev_mng_action(color_ostream& out, void* ptr) {


I suggest naming this ev_mng_unitAction to match the event type name

myk002 · 2022-04-14T03:28:15Z

library/include/modules/EventManager.h

+        struct ActionData {
+            int32_t unitId;
+            df::unit_action* action;
+            int32_t actionId;


If actionId is a field of action, why pass the id as a separate field to the handler?

Midunderstanding of the comment //it has to keep the id of an item because the item itself may have been deallocated, most likely; I didn't quite put together that this is for inventory_item in particular

myk002 · 2022-04-14T03:34:00Z

library/modules/EventManager.cpp

@@ -200,6 +203,9 @@ static int32_t reportToRelevantUnitsTime = -1;
 //interaction
 static int32_t lastReportInteraction;

+//unit action
+static std::map<int32_t,std::vector<int32_t> > unitToKnownActions;


Since we don't iterate through this structure, an unordered_map might bea better choice here.

Since we don't iterate through this structure, an unordered_map might bea better choice here.

True.

But an even better idea is to add a hash for the data we actually care about, since the EventManager::ActionData are trivial to construct (and the hash trivial to implement), and then change it to std::unordered_set<ActionData>.

This way we avoid whatever kind of search std::find is doing (I would presume linear, since it can't know if it's sorted.. but maybe that's a requirement else UB). It would also remove at least one indent level.

edit: examples

dfhack/library/include/modules/EventManager.h

Lines 107 to 118 in 9bf9a79

namespace std {

template <>

struct hash<df::coord> {

std::size_t operator()(const df::coord& c) const {

size_t r = 17;

const size_t m = 65537;

r = m*(r+c.x);

r = m*(r+c.y);

r = m*(r+c.z);

return r;

}

};

myk002 · 2022-04-14T03:36:51Z

library/modules/EventManager.cpp

+                    }
+                } else {
+                    auto newEnd = std::remove(knownActions->begin(), knownActions->end(), action->id);
+                    knownActions->erase(newEnd, knownActions->end());


Could you explain what is happening here? Is an action type of None invalidating all more recent actions?

It's the erase-removed idiom, though perhaps more verbose than it needs to be

I see. So it's only removing a single element. Does order matter for these actions, or would we be better off using a set instead of a vector?

myk002 · 2022-04-14T03:39:43Z

Also be aware that #2044 is in flight. @cppcooper could you review to see how to merge this new event?

cppcooper

Also be aware that #2044 is in flight. @cppcooper could you review to see how to merge this new event?

So I've reviewed this now.

cppcooper · 2022-04-14T19:35:52Z

library/modules/EventManager.cpp

@@ -157,6 +159,7 @@ static const eventManager_t eventManager[] = {
        manageUnitAttackEvent,
        manageUnloadEvent,
        manageInteractionEvent,
+        manageActionEvent,


This task will need to be moved to a switch when/if #2097 is merged

cppcooper · 2022-04-14T19:43:48Z

library/modules/EventManager.cpp

+        return;
+    multimap<Plugin*,EventHandler> copy(handlers[EventType::UNIT_ACTION].begin(), handlers[EventType::UNIT_ACTION].end());
+    for ( size_t a = 0; a < df::global::world->units.all.size(); a++ ) {
+        df::unit* unit = df::global::world->units.all[a];


I agree. Fairly sure I've replaced any that could be via clion's clang-tidy suggestions. So if I end up merging these into PR #2044 I'll be getting this one too.

cppcooper · 2022-04-14T19:48:52Z

library/modules/EventManager.cpp

+    multimap<Plugin*,EventHandler> copy(handlers[EventType::UNIT_ACTION].begin(), handlers[EventType::UNIT_ACTION].end());
+    for ( size_t a = 0; a < df::global::world->units.all.size(); a++ ) {
+        df::unit* unit = df::global::world->units.all[a];
+        if ( Units::isActive(unit) ) {


I dunno, I wouldn't worry about it until it's not just the indentation but also the length of the block below. This all kinda fits on one screen.

cppcooper · 2022-04-14T19:50:04Z

library/modules/EventManager.cpp

+    for ( size_t a = 0; a < df::global::world->units.all.size(); a++ ) {
+        df::unit* unit = df::global::world->units.all[a];
+        if ( Units::isActive(unit) ) {
+            auto knownActions = &unitToKnownActions[unit->id];


cppcooper · 2022-04-14T19:54:59Z

library/modules/EventManager.cpp

+    multimap<Plugin*,EventHandler> copy(handlers[EventType::UNIT_ACTION].begin(), handlers[EventType::UNIT_ACTION].end());
+    for ( size_t a = 0; a < df::global::world->units.all.size(); a++ ) {
+        df::unit* unit = df::global::world->units.all[a];
+        if ( Units::isActive(unit) ) {
+            auto knownActions = &unitToKnownActions[unit->id];
+            for ( df::unit_action* action : unit->actions ) {
+                if ( action->type != df::unit_action_type::None) {
+                    if ( std::find(knownActions->begin(), knownActions->end(), action->id) == knownActions->end() ) {
+                        knownActions->push_back(action->id);
+                        for ( auto b = copy.begin(); b != copy.end(); b++ ) {
+                            EventHandler handle = (*b).second;
+                            ActionData data = {unit->id, action, action->id};
+                            handle.eventHandler(out, (void*)&data);


So like all the events (that I can think of) in EventManager refactoring #2044 this would need to be split into two parts. Data generation, and event messaging.

Beyond that we'd be looking at knowing the current tick as to log the "time" that data was generated, and then the messaging loop would look the same as all the others.

At a later point, we're probably going to want to merge the data generators cause many iterate the same structures. But that's not part of this discussion.

The only one that might apply to this PR would be 1. With 2, it would just introduce an unused variable.

cppcooper · 2022-04-14T19:57:11Z

plugins/eventful.cpp

@@ -242,6 +249,7 @@ static const handler_t eventHandlers[] = {
 ev_mng_unitAttack,
 ev_mng_unload,
 ev_mng_interaction,
+ ev_mng_action,


This too will get moved to a switch when/if #2097 is merged.

cppcooper · 2022-04-14T20:04:14Z

library/modules/EventManager.cpp

+                    if ( std::find(knownActions->begin(), knownActions->end(), action->id) == knownActions->end() ) {
+                        knownActions->push_back(action->id);


So it seems like the data structure is only being used to keep track of events already seen. event_tracker in #2044 will take care of this. But.. see my suggestion above in the meanwhile. Which will reduce what needs to be done in #2044 when merging these changes into it.

cppcooper · 2022-04-14T20:05:11Z

library/modules/EventManager.cpp

@@ -200,6 +203,9 @@ static int32_t reportToRelevantUnitsTime = -1;
 //interaction
 static int32_t lastReportInteraction;

+//unit action
+static std::map<int32_t,std::vector<int32_t> > unitToKnownActions;


Since we don't iterate through this structure, an unordered_map might bea better choice here.

True.

But an even better idea is to add a hash for the data we actually care about, since the EventManager::ActionData are trivial to construct (and the hash trivial to implement), and then change it to std::unordered_set<ActionData>.

This way we avoid whatever kind of search std::find is doing (I would presume linear, since it can't know if it's sorted.. but maybe that's a requirement else UB). It would also remove at least one indent level.

edit: examples

dfhack/library/include/modules/EventManager.h

Lines 107 to 118 in 9bf9a79

namespace std {

template <>

struct hash<df::coord> {

std::size_t operator()(const df::coord& c) const {

size_t r = 17;

const size_t m = 65537;

r = m*(r+c.x);

r = m*(r+c.y);

r = m*(r+c.z);

return r;

}

};

myk002 · 2022-04-15T04:49:18Z

Could you merge in latest develop and add the event functions to the new arrays added by #2097 ?

Putnam3145 · 2022-04-15T20:43:29Z

Yeah, I was just gonna wait until the rework stuff is merged then adapt, I'll do all the stuff now.

…action

myk002

Apart from the one question on the data structure, looks great! Could you merge latest develop one more time and do a quick verification with devel/eventful-client that the events come out the way you expect?

myk002 · 2022-04-22T02:09:42Z

library/modules/EventManager.cpp

@@ -235,6 +239,9 @@ static int32_t reportToRelevantUnitsTime = -1;
 //interaction
 static int32_t lastReportInteraction;

+//unit action
+static std::unordered_map<int32_t,std::vector<int32_t> > unitToKnownActions;


It looks like this should be a std::unordered_map<int32_t,std::unordered_set<int32_t>> since there doesn't seem to be a reason to pay the cost of iterating through the vector on every lookup, or am I missing something?

Your map<set> is a fine approach, but I still think using the action event data alone would be best. std::unordered_set<ActionData> with a hash function that hashes on the unit id and the action id. Maybe I'm missing something, but that looks like it'll give the same guarantees with the fastest lookup times

Agreed. That would be cleaner.

@Putnam3145 are you ok with this change?

myk002 · 2022-05-13T03:19:13Z

library/modules/EventManager.cpp

@@ -235,6 +239,9 @@ static int32_t reportToRelevantUnitsTime = -1;
 //interaction
 static int32_t lastReportInteraction;

+//unit action
+static std::unordered_map<int32_t,std::vector<int32_t> > unitToKnownActions;


@Putnam3145 are you ok with this change?

Putnam3145 · 2022-10-13T22:27:03Z

Any and all, yes, sorry, uhh, I get distracted by things and I maintain an active-enough open source repository that github notifications are completely useless

ab9rf · 2023-04-02T11:10:56Z

is this still viable?

Putnam3145 added 2 commits April 13, 2022 14:20

Adds an onUnitAction event

d1f933b

Merge branch 'develop' of https://github.com/DFHack/dfhack into unit_…

ec11406

…action

myk002 reviewed Apr 14, 2022

View reviewed changes

myk002 added the eventful label Apr 14, 2022

myk002 added this to In Progress in 0.47.05-r5 via automation Apr 14, 2022

cppcooper reviewed Apr 14, 2022

View reviewed changes

Putnam3145 added 4 commits April 15, 2022 13:51

Merge branch 'develop' of https://github.com/DFHack/dfhack into unit_…

2763e47

…action

Remove actionId from EventManager::ActionData

d96dcc6

Somehow missed the Lua stuff in there

62deece

compiling, that's a neat trick

a244d59

myk002 requested changes Apr 22, 2022

View reviewed changes

myk002 removed this from In Progress in 0.47.05-r5 May 2, 2022

Merge branch 'develop' into unit_action

d199b5b

myk002 requested changes May 13, 2022

View reviewed changes

myk002 removed the eventful label Jun 19, 2023

	namespace std {
	template <>
	struct hash<df::coord> {
	std::size_t operator()(const df::coord& c) const {
	size_t r = 17;
	const size_t m = 65537;
	r = m*(r+c.x);
	r = m*(r+c.y);
	r = m*(r+c.z);
	return r;
	}
	};

		if ( std::find(knownActions->begin(), knownActions->end(), action->id) == knownActions->end() ) {
		knownActions->push_back(action->id);

Adds an onUnitAction event #2094

Are you sure you want to change the base?

Adds an onUnitAction event #2094

Conversation

Putnam3145 commented Apr 13, 2022

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

Choose a reason for hiding this comment

cppcooper Apr 14, 2022 • edited

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 Apr 14, 2022

cppcooper left a 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

cppcooper Apr 14, 2022 • edited

Choose a reason for hiding this comment

cppcooper Apr 15, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cppcooper Apr 14, 2022 • edited

Choose a reason for hiding this comment

myk002 commented Apr 15, 2022

Putnam3145 commented Apr 15, 2022

myk002 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cppcooper Apr 22, 2022 • edited

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

Putnam3145 commented Oct 13, 2022

ab9rf commented Apr 2, 2023

cppcooper Apr 14, 2022 •

edited

cppcooper Apr 14, 2022 •

edited

cppcooper Apr 15, 2022 •

edited

cppcooper Apr 14, 2022 •

edited

cppcooper Apr 22, 2022 •

edited