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

Conversation

Putnam3145
Copy link

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.

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.

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) ) {
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

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

@@ -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

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

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

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

@myk002
Copy link
Member

myk002 commented Apr 14, 2022

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

Copy link
Contributor

@cppcooper cppcooper left a comment

Choose a reason for hiding this comment

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

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.

@@ -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

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

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) ) {
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.

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

Comment on lines 1268 to 1280
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);
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.

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

Comment on lines 1275 to 1276
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.

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

@myk002
Copy link
Member

myk002 commented Apr 15, 2022

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

@Putnam3145
Copy link
Author

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

Copy link
Member

@myk002 myk002 left a comment

Choose a reason for hiding this comment

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

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?

@@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@cppcooper cppcooper Apr 22, 2022

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. That would be cleaner.

Copy link
Member

Choose a reason for hiding this comment

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

@Putnam3145 are you ok with this change?

@@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

@Putnam3145 are you ok with this change?

@Putnam3145
Copy link
Author

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
Copy link
Member

ab9rf commented Apr 2, 2023

is this still viable?

@myk002 myk002 removed the eventful label Jun 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants