Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds an onUnitAction event #2094

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions docs/Lua API.rst
Original file line number Diff line number Diff line change
Expand Up @@ -4406,6 +4406,10 @@ These events are straight from EventManager module. Each of them first needs to

Called when a unit uses an interaction on another.

14. ``onUnitAction(unit_id, action, action_id)``

Called when a unit does an action (movement, attacking, talking and so on).

Functions
---------

Expand Down
1 change: 1 addition & 0 deletions docs/changelog.txt
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ changelog.txt uses a syntax similar to RST, with a few special sequences:
- `EventManager`: add new event type ``JOB_STARTED``, triggered when a job first gains a worker
- `EventManager`: add new event type ``NEW_UNIT_ACTIVE``, triggered when a new unit appears on the active list
- `EventManager`: now each registered handler for an event can have its own frequency instead of all handlers using the lowest frequency of all handlers
- `EventManager`: add new event type ``UNIT_ACTION``, triggered when a unit starts an action
- `stocks`: allow search terms to match the full item label, even when the label is truncated for length
- `dfhack-examples-guide`: add mugs to ``basic`` manager orders
- `gui/create-item`: Added "(chain)" annotation text for armours with the [CHAIN_METAL_TEXT] flag set
Expand Down
7 changes: 7 additions & 0 deletions library/include/modules/EventManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ namespace DFHack {
UNIT_ATTACK,
UNLOAD,
INTERACTION,
UNIT_ACTION,
EVENT_MAX
};
}
Expand Down Expand Up @@ -95,6 +96,12 @@ namespace DFHack {
int32_t defendReport;
};

struct ActionData {
int32_t unitId;
df::unit_action* action;
int32_t actionId;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

};

DFHACK_EXPORT void registerListener(EventType::EventType e, EventHandler handler, Plugin* plugin);
DFHACK_EXPORT int32_t registerTick(EventHandler handler, int32_t when, Plugin* plugin, bool absolute=false);
DFHACK_EXPORT void unregister(EventType::EventType e, EventHandler handler, Plugin* plugin);
Expand Down
36 changes: 36 additions & 0 deletions library/modules/EventManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#include "df/report.h"
#include "df/ui.h"
#include "df/unit.h"
#include "df/unit_action.h"
#include "df/unit_flags1.h"
#include "df/unit_inventory_item.h"
#include "df/unit_report_type.h"
Expand Down Expand Up @@ -137,6 +138,7 @@ static void manageReportEvent(color_ostream& out);
static void manageUnitAttackEvent(color_ostream& out);
static void manageUnloadEvent(color_ostream& out){};
static void manageInteractionEvent(color_ostream& out);
static void manageActionEvent(color_ostream& out);

typedef void (*eventManager_t)(color_ostream&);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

};

//job initiated
Expand Down Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@cppcooper cppcooper Apr 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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;
}
};


void DFHack::EventManager::onStateChange(color_ostream& out, state_change_event event) {
static bool doOnce = false;
// const string eventNames[] = {"world loaded", "world unloaded", "map loaded", "map unloaded", "viewscreen changed", "core initialized", "begin unload", "paused", "unpaused"};
Expand All @@ -222,6 +228,7 @@ void DFHack::EventManager::onStateChange(color_ostream& out, state_change_event
buildings.clear();
constructions.clear();
equipmentLog.clear();
unitToKnownActions.clear();

Buildings::clearBuildings(out);
lastReport = -1;
Expand Down Expand Up @@ -1254,3 +1261,32 @@ static void manageInteractionEvent(color_ostream& out) {
//TODO: deduce attacker from latest defend event first
}
}

static void manageActionEvent(color_ostream& out) {
if (!df::global::world)
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];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if ( Units::isActive(unit) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To reduce indentation, how about

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

This way you don't need an else clause

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

auto knownActions = &unitToKnownActions[unit->id];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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);
Copy link
Contributor

@cppcooper cppcooper Apr 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 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.

  2. 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.

Copy link
Contributor

@cppcooper cppcooper Apr 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}
}
} else {
unitToKnownActions.erase(unit->id);
}
}
}
8 changes: 8 additions & 0 deletions plugins/eventful.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include "df/reaction_reagent_itemst.h"
#include "df/reaction_product_itemst.h"
#include "df/unit.h"
#include "df/unit_action.h"
#include "df/unit_inventory_item.h"
#include "df/unit_wound.h"
#include "df/world.h"
Expand Down Expand Up @@ -109,6 +110,7 @@ DEFINE_LUA_EVENT_NH_1(onReport, int32_t);
DEFINE_LUA_EVENT_NH_3(onUnitAttack, int32_t, int32_t, int32_t);
DEFINE_LUA_EVENT_NH_0(onUnload);
DEFINE_LUA_EVENT_NH_6(onInteraction, std::string, std::string, int32_t, int32_t, int32_t, int32_t);
DEFINE_LUA_EVENT_NH_3(onUnitAction, int32_t, df::unit_action*, int32_t);

DFHACK_PLUGIN_LUA_EVENTS {
DFHACK_LUA_EVENT(onWorkshopFillSidebarMenu),
Expand Down Expand Up @@ -136,6 +138,7 @@ DFHACK_PLUGIN_LUA_EVENTS {
DFHACK_LUA_EVENT(onUnitAttack),
DFHACK_LUA_EVENT(onUnload),
DFHACK_LUA_EVENT(onInteraction),
DFHACK_LUA_EVENT(onUnitAction),
DFHACK_LUA_END
};

Expand Down Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest naming this ev_mng_unitAction to match the event type name

EventManager::ActionData* data = (EventManager::ActionData*)ptr;
onUnitAction(out, data->unitId, data->action, data->actionId);
}
std::vector<int> enabledEventManagerEvents(EventManager::EventType::EVENT_MAX,-1);
typedef void (*handler_t) (color_ostream&,void*);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

};
static void enableEvent(int evType,int freq)
{
Expand Down
1 change: 1 addition & 0 deletions plugins/lua/eventful.lua
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,7 @@ eventType=invertTable{
"UNIT_ATTACK",
"UNLOAD",
"INTERACTION",
"UNIT_ACTION",
"EVENT_MAX"
}
return _ENV